Skip to content

Commit

Permalink
[GRC] Decouple PageSignalReceiver from TabManager.
Browse files Browse the repository at this point in the history
Previously PageSignalReceiver depends on TabManager because it calls directly to
TabManager::WebContentsData and TabManager::TabStatsCollector. TabManager also
depends on PageSignalReceiver directly or indirectly. To make PageSignalReceiver
decoupled from TabManger, we need to introduce a layering that
PageSignalReceiver calls into but not part of TabManager.

Thus, this patch:

1. Decoupled PageSignalReceiver from TabManager by adding PageSignalObserver
API;
2. Add [Add|Remove]Observer APIs in PageSignalReceiver;
3. Implemented a TabManager::ResourceCoordinatorSignalObserver which implements
PageSignalObserver APIs.

BUG=775691

Change-Id: I8c111ed36bbdb39c69d3568e5f7396dcd01c4ad7
Reviewed-on: https://chromium-review.googlesource.com/752506
Commit-Queue: lpy <[email protected]>
Reviewed-by: Zhen Wang <[email protected]>
Cr-Commit-Position: refs/heads/master@{#514308}
  • Loading branch information
lpy authored and Commit Bot committed Nov 7, 2017
1 parent b34d3fb commit 8e18bee
Show file tree
Hide file tree
Showing 9 changed files with 161 additions and 52 deletions.
6 changes: 4 additions & 2 deletions chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,8 @@ split_static_library("browser") {
"resource_coordinator/browser_child_process_watcher.h",
"resource_coordinator/chrome_browser_main_extra_parts_resource_coordinator.cc",
"resource_coordinator/chrome_browser_main_extra_parts_resource_coordinator.h",
"resource_coordinator/page_signal_receiver.cc",
"resource_coordinator/page_signal_receiver.h",
"resource_coordinator/resource_coordinator_render_process_probe.cc",
"resource_coordinator/resource_coordinator_render_process_probe.h",
"resource_coordinator/resource_coordinator_web_contents_observer.cc",
Expand Down Expand Up @@ -2524,14 +2526,14 @@ split_static_library("browser") {
"repost_form_warning_controller.h",
"resource_coordinator/background_tab_navigation_throttle.cc",
"resource_coordinator/background_tab_navigation_throttle.h",
"resource_coordinator/page_signal_receiver.cc",
"resource_coordinator/page_signal_receiver.h",
"resource_coordinator/tab_manager.cc",
"resource_coordinator/tab_manager.h",
"resource_coordinator/tab_manager_delegate_chromeos.cc",
"resource_coordinator/tab_manager_delegate_chromeos.h",
"resource_coordinator/tab_manager_observer.cc",
"resource_coordinator/tab_manager_observer.h",
"resource_coordinator/tab_manager_resource_coordinator_signal_observer.cc",
"resource_coordinator/tab_manager_resource_coordinator_signal_observer.h",
"resource_coordinator/tab_manager_stats_collector.cc",
"resource_coordinator/tab_manager_stats_collector.h",
"resource_coordinator/tab_manager_web_contents_data.cc",
Expand Down
76 changes: 41 additions & 35 deletions chrome/browser/resource_coordinator/page_signal_receiver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@

#include "base/lazy_instance.h"
#include "base/time/time.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/resource_coordinator/tab_manager_stats_collector.h"
#include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
#include "content/public/common/service_manager_connection.h"
#include "services/resource_coordinator/public/cpp/coordination_unit_id.h"
#include "services/resource_coordinator/public/cpp/resource_coordinator_features.h"
Expand All @@ -18,70 +15,79 @@
namespace resource_coordinator {

namespace {
base::LazyInstance<TabManager::PageSignalReceiver>::Leaky
g_page_signal_receiver;
base::LazyInstance<PageSignalReceiver>::Leaky g_page_signal_receiver;
}

// static
bool TabManager::PageSignalReceiver::IsEnabled() {
// Check that service_manager is active and GRC is enabled.
bool PageSignalReceiver::IsEnabled() {
// Check that service_manager is active and Resource Coordinator is enabled.
return content::ServiceManagerConnection::GetForProcess() != nullptr &&
resource_coordinator::IsResourceCoordinatorEnabled();
}

// static
TabManager::PageSignalReceiver* TabManager::PageSignalReceiver::GetInstance() {
PageSignalReceiver* PageSignalReceiver::GetInstance() {
if (!IsEnabled())
return nullptr;
return g_page_signal_receiver.Pointer();
}

TabManager::PageSignalReceiver::PageSignalReceiver() : binding_(this) {
content::ServiceManagerConnection* service_manager_connection =
content::ServiceManagerConnection::GetForProcess();
// Ensure service_manager is active before trying to connect to it.
if (service_manager_connection) {
service_manager::Connector* connector =
service_manager_connection->GetConnector();
mojom::PageSignalGeneratorPtr page_signal_generator_ptr;
connector->BindInterface(mojom::kServiceName,
mojo::MakeRequest(&page_signal_generator_ptr));
mojom::PageSignalReceiverPtr page_signal_receiver_ptr;
binding_.Bind(mojo::MakeRequest(&page_signal_receiver_ptr));
page_signal_generator_ptr->AddReceiver(std::move(page_signal_receiver_ptr));
}
}
PageSignalReceiver::PageSignalReceiver() : binding_(this) {}

TabManager::PageSignalReceiver::~PageSignalReceiver() = default;
PageSignalReceiver::~PageSignalReceiver() = default;

void TabManager::PageSignalReceiver::NotifyPageAlmostIdle(
const CoordinationUnitID& cu_id) {
void PageSignalReceiver::NotifyPageAlmostIdle(const CoordinationUnitID& cu_id) {
auto web_contents_iter = cu_id_web_contents_map_.find(cu_id);
if (web_contents_iter == cu_id_web_contents_map_.end())
return;
auto* web_contents_data =
TabManager::WebContentsData::FromWebContents(web_contents_iter->second);
web_contents_data->NotifyAlmostIdle();
for (auto& observer : observers_)
observer.NotifyPageAlmostIdle(web_contents_iter->second);
}

void TabManager::PageSignalReceiver::SetExpectedTaskQueueingDuration(
void PageSignalReceiver::SetExpectedTaskQueueingDuration(
const CoordinationUnitID& cu_id,
base::TimeDelta duration) {
auto web_contents_iter = cu_id_web_contents_map_.find(cu_id);
if (web_contents_iter == cu_id_web_contents_map_.end())
return;
g_browser_process->GetTabManager()
->stats_collector()
->RecordExpectedTaskQueueingDuration(web_contents_iter->second, duration);
for (auto& observer : observers_)
observer.OnExpectedTaskQueueingDurationSet(web_contents_iter->second,
duration);
}

void PageSignalReceiver::AddObserver(PageSignalObserver* observer) {
// When PageSignalReceiver starts to have observer, construct the mojo
// channel.
if (!observers_.might_have_observers()) {
content::ServiceManagerConnection* service_manager_connection =
content::ServiceManagerConnection::GetForProcess();
// Ensure service_manager is active before trying to connect to it.
if (service_manager_connection) {
service_manager::Connector* connector =
service_manager_connection->GetConnector();
mojom::PageSignalGeneratorPtr page_signal_generator_ptr;
connector->BindInterface(mojom::kServiceName,
mojo::MakeRequest(&page_signal_generator_ptr));
mojom::PageSignalReceiverPtr page_signal_receiver_ptr;
binding_.Bind(mojo::MakeRequest(&page_signal_receiver_ptr));
page_signal_generator_ptr->AddReceiver(
std::move(page_signal_receiver_ptr));
}
}
observers_.AddObserver(observer);
}

void PageSignalReceiver::RemoveObserver(PageSignalObserver* observer) {
observers_.RemoveObserver(observer);
}

void TabManager::PageSignalReceiver::AssociateCoordinationUnitIDWithWebContents(
void PageSignalReceiver::AssociateCoordinationUnitIDWithWebContents(
const CoordinationUnitID& cu_id,
content::WebContents* web_contents) {
cu_id_web_contents_map_[cu_id] = web_contents;
}

void TabManager::PageSignalReceiver::RemoveCoordinationUnitID(
void PageSignalReceiver::RemoveCoordinationUnitID(
const CoordinationUnitID& cu_id) {
cu_id_web_contents_map_.erase(cu_id);
}
Expand Down
29 changes: 23 additions & 6 deletions chrome/browser/resource_coordinator/page_signal_receiver.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#define CHROME_BROWSER_RESOURCE_COORDINATOR_PAGE_SIGNAL_RECEIVER_H_

#include "base/macros.h"
#include "chrome/browser/resource_coordinator/tab_manager.h"
#include "base/observer_list.h"
#include "mojo/public/cpp/bindings/binding.h"
#include "services/resource_coordinator/public/interfaces/page_signal.mojom.h"

Expand All @@ -16,11 +16,25 @@ class WebContents;

namespace resource_coordinator {

// Implementation of resource_coordinator::mojom::PageSignalReceiver,
// observer constructs a mojo channel to PageSignalGenerator in GRC, passes an
// interface pointer to PageSignalGenerator, and receives page scoped signals
// from PageSignalGenerator once a signal is generated.
class TabManager::PageSignalReceiver : public mojom::PageSignalReceiver {
// A PageSignalObserver is implemented to receive notifications from
// PageSignalReceiver by adding itself to PageSignalReceiver.
class PageSignalObserver {
public:
virtual ~PageSignalObserver() = default;
virtual void NotifyPageAlmostIdle(content::WebContents* web_contents) {}
virtual void OnExpectedTaskQueueingDurationSet(
content::WebContents* web_contents,
base::TimeDelta duration) {}
};

// Implementation of resource_coordinator::mojom::PageSignalReceiver.
// PageSignalReceiver constructs a mojo channel to PageSignalGenerator in
// resource coordinator, passes an interface pointer to PageSignalGenerator,
// receives page scoped signals from PageSignalGenerator, and dispatches them
// with WebContents to PageSignalObservers.
// The mojo channel won't be constructed until PageSignalReceiver has the first
// observer.
class PageSignalReceiver : public mojom::PageSignalReceiver {
public:
PageSignalReceiver();
~PageSignalReceiver() override;
Expand All @@ -34,6 +48,8 @@ class TabManager::PageSignalReceiver : public mojom::PageSignalReceiver {
void SetExpectedTaskQueueingDuration(const CoordinationUnitID& cu_id,
base::TimeDelta duration) override;

void AddObserver(PageSignalObserver* observer);
void RemoveObserver(PageSignalObserver* observer);
void AssociateCoordinationUnitIDWithWebContents(
const CoordinationUnitID& cu_id,
content::WebContents* web_contents);
Expand All @@ -43,6 +59,7 @@ class TabManager::PageSignalReceiver : public mojom::PageSignalReceiver {
private:
mojo::Binding<mojom::PageSignalReceiver> binding_;
std::map<CoordinationUnitID, content::WebContents*> cu_id_web_contents_map_;
base::ObserverList<PageSignalObserver> observers_;
DISALLOW_COPY_AND_ASSIGN(PageSignalReceiver);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,13 @@ ResourceCoordinatorWebContentsObserver::ResourceCoordinatorWebContentsObserver(
EnsureUkmRecorderInterface();
}

#if !defined(OS_ANDROID)
if (auto* page_signal_receiver =
resource_coordinator::TabManager::PageSignalReceiver::GetInstance()) {
resource_coordinator::PageSignalReceiver::GetInstance()) {
// Gets CoordinationUnitID for this WebContents and adds it to
// PageSignalReceiver.
page_signal_receiver->AssociateCoordinationUnitIDWithWebContents(
page_resource_coordinator_->id(), web_contents);
}
#endif
}

// TODO(matthalp) integrate into ResourceCoordinatorService once the UKM mojo
Expand Down Expand Up @@ -122,15 +120,13 @@ void ResourceCoordinatorWebContentsObserver::WasHidden() {
}

void ResourceCoordinatorWebContentsObserver::WebContentsDestroyed() {
#if !defined(OS_ANDROID)
if (auto* page_signal_receiver =
resource_coordinator::TabManager::PageSignalReceiver::GetInstance()) {
resource_coordinator::PageSignalReceiver::GetInstance()) {
// Gets CoordinationUnitID for this WebContents and removes it from
// PageSignalReceiver.
page_signal_receiver->RemoveCoordinationUnitID(
page_resource_coordinator_->id());
}
#endif
}

void ResourceCoordinatorWebContentsObserver::DidFinishNavigation(
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/resource_coordinator/tab_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "chrome/browser/resource_coordinator/background_tab_navigation_throttle.h"
#include "chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h"
#include "chrome/browser/resource_coordinator/tab_manager_observer.h"
#include "chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.h"
#include "chrome/browser/resource_coordinator/tab_manager_stats_collector.h"
#include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
#include "chrome/browser/resource_coordinator/time.h"
Expand Down Expand Up @@ -240,10 +241,15 @@ TabManager::TabManager()
#endif
browser_tab_strip_tracker_.Init();
session_restore_observer_.reset(new TabManagerSessionRestoreObserver(this));
if (PageSignalReceiver::IsEnabled()) {
resource_coordinator_signal_observer_.reset(
new ResourceCoordinatorSignalObserver());
}
stats_collector_.reset(new TabManagerStatsCollector());
}

TabManager::~TabManager() {
resource_coordinator_signal_observer_.reset();
Stop();
}

Expand Down
10 changes: 8 additions & 2 deletions chrome/browser/resource_coordinator/tab_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,9 @@ struct BrowserInfo {
class TabManager : public TabStripModelObserver,
public chrome::BrowserListObserver {
public:
// Forward declaration of page signal observer.
class PageSignalReceiver;
// Forward declaration of resource coordinator signal observer.
class ResourceCoordinatorSignalObserver;

// Needs to be public for DEFINE_WEB_CONTENTS_USER_DATA_KEY.
class WebContentsData;

Expand Down Expand Up @@ -565,6 +566,11 @@ class TabManager : public TabStripModelObserver,
// in parallel.
size_t loading_slots_;

// |resource_coordinator_signal_observer_| is owned by TabManager and is used
// to receive various signals from ResourceCoordinator.
std::unique_ptr<ResourceCoordinatorSignalObserver>
resource_coordinator_signal_observer_;

// Records UMAs for tab and system-related events and properties during
// session restore.
std::unique_ptr<TabManagerStatsCollector> stats_collector_;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.h"

#include "base/time/time.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/resource_coordinator/tab_manager_stats_collector.h"
#include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"

namespace resource_coordinator {

TabManager::ResourceCoordinatorSignalObserver::
ResourceCoordinatorSignalObserver() {
if (auto* page_signal_receiver = PageSignalReceiver::GetInstance())
page_signal_receiver->AddObserver(this);
}

TabManager::ResourceCoordinatorSignalObserver::
~ResourceCoordinatorSignalObserver() {
if (auto* page_signal_receiver = PageSignalReceiver::GetInstance())
page_signal_receiver->RemoveObserver(this);
}

void TabManager::ResourceCoordinatorSignalObserver::NotifyPageAlmostIdle(
content::WebContents* web_contents) {
auto* web_contents_data =
TabManager::WebContentsData::FromWebContents(web_contents);
web_contents_data->NotifyAlmostIdle();
}

void TabManager::ResourceCoordinatorSignalObserver::
OnExpectedTaskQueueingDurationSet(content::WebContents* web_contents,
base::TimeDelta duration) {
g_browser_process->GetTabManager()
->stats_collector()
->RecordExpectedTaskQueueingDuration(web_contents, duration);
}

} // namespace resource_coordinator
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// Copyright 2017 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_RESOURCE_COORDINATOR_SIGNAL_OBSERVER_H_
#define CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_RESOURCE_COORDINATOR_SIGNAL_OBSERVER_H_

#include "base/macros.h"
#include "chrome/browser/resource_coordinator/page_signal_receiver.h"
#include "chrome/browser/resource_coordinator/tab_manager.h"

namespace resource_coordinator {

// TabManager::ResourceCoordinatorSignalObserver implements some observer
// interfaces, for example it currently implements PageSignalObserver but can
// implement more; and receives signals from resource coordinator through those
// interfaces.
class TabManager::ResourceCoordinatorSignalObserver
: public PageSignalObserver {
public:
ResourceCoordinatorSignalObserver();
~ResourceCoordinatorSignalObserver() override;

// PageSignalObserver implementation.
void NotifyPageAlmostIdle(content::WebContents* web_contents) override;
void OnExpectedTaskQueueingDurationSet(content::WebContents* web_contents,
base::TimeDelta duration) override;

private:
DISALLOW_COPY_AND_ASSIGN(ResourceCoordinatorSignalObserver);
};

} // namespace resource_coordinator

#endif // CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_RESOURCE_COORDINATOR_SIGNAL_OBSERVER_H_
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class PageSignalGeneratorImpl : public CoordinationUnitGraphObserver,
PageSignalGeneratorImpl();
~PageSignalGeneratorImpl() override;

// mojom::SignalGenerator implementation.
// mojom::PageSignalGenerator implementation.
void AddReceiver(mojom::PageSignalReceiverPtr receiver) override;

// CoordinationUnitGraphObserver implementation.
Expand Down

0 comments on commit 8e18bee

Please sign in to comment.