ResourceScheduler get visual signal from RenderViewHostImpl

content/browser/loader/resource_scheduler.cc

Index: content/browser/loader/resource_scheduler.cc
diff --git a/content/browser/loader/resource_scheduler.cc b/content/browser/loader/resource_scheduler.cc
index 85adbdb30e275631b53af88286e75b658e73a16c..5f3e41eda13041b79bd88e6bf18cc515ebbfc598 100644
--- a/content/browser/loader/resource_scheduler.cc
+++ b/content/browser/loader/resource_scheduler.cc
@@ -9,9 +9,11 @@
 #include "base/stl_util.h"
 #include "content/common/resource_messages.h"
 #include "content/browser/loader/resource_message_delegate.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/resource_throttle.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "ipc/ipc_message_macros.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/load_flags.h"
@@ -227,28 +229,59 @@ void ResourceScheduler::RequestQueue::Insert(
   pointers_[request] = queue_.insert(request);
 }
 
+// VisualObserver lives on UI thread
+class ResourceScheduler::VisualObserver : public content::WebContentsObserver {
+ public:
+  explicit VisualObserver(WebContentsImpl* web_contents);
+  virtual ~VisualObserver() { }
+
+  void SetClient(Client* client);
+  bool IsVisible();
+
+  // WebContentsObserver implementation.
+  virtual void WasShown() OVERRIDE;
+  virtual void WasHidden() OVERRIDE;
+
+ private:
+  Client* client_;
+};
+
 // Each client represents a tab.
 class ResourceScheduler::Client {
  public:
-  explicit Client(ResourceScheduler* scheduler)
+  explicit Client(ResourceScheduler* scheduler,
+                  VisualObserver* visual_observer,
+                  bool is_visible)
       : is_audible_(false),
-        is_visible_(false),
+        is_visible_(is_visible),
         is_loaded_(false),
         is_paused_(false),
         has_body_(false),
         using_spdy_proxy_(false),
         total_delayable_count_(0),
-        throttle_state_(ResourceScheduler::THROTTLED) {
+        throttle_state_(ResourceScheduler::THROTTLED),
+        visual_observer_(visual_observer) {
     scheduler_ = scheduler;
+    if (visual_observer)
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&ResourceScheduler::VisualObserver::SetClient,
+                     base::Unretained(visual_observer_),
+                     base::Unretained(this)));
   }
 
-  ~Client() {
+  virtual ~Client() {
     // Update to default state and pause to ensure the scheduler has a
     // correct count of relevant types of clients.
     is_visible_ = false;
     is_audible_ = false;
     is_paused_ = true;
     UpdateThrottleState();
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&ResourceScheduler::DeleteVisualObserver,
+                   base::Unretained(scheduler_),
+                   base::Unretained(visual_observer_)));
   }
 
   void ScheduleRequest(
@@ -288,6 +321,8 @@ class ResourceScheduler::Client {
 
   bool is_loaded() const { return is_loaded_; }
 
+  bool IsVisible() const { return is_visible_; }
+
   void OnAudibilityChanged(bool is_audible) {
     if (is_audible == is_audible_) {
       return;
@@ -659,8 +694,47 @@ class ResourceScheduler::Client {
   // The number of delayable in-flight requests.
   size_t total_delayable_count_;
   ResourceScheduler::ClientThrottleState throttle_state_;
+  VisualObserver* visual_observer_;
 };
 
+ResourceScheduler::VisualObserver::VisualObserver(WebContentsImpl* web_contents)
+    : client_(NULL) {
+  Observe(web_contents);
+}
+
+void ResourceScheduler::VisualObserver::SetClient(Client* client) {
+  client_ = client;
+}
+
+// WebContentsObserver implementation.
+void ResourceScheduler::VisualObserver::WasShown() {
+  if (client_)

[mmenke, 2014/08/19 14:38:27]

So we ignore events if the visibility changes if we don't know the client yet?

[Zhen Wang, 2014/08/19 17:53:41]

Yes. It may happen that the Client hasn't been created and associated with the
VisualObserver when the WebContents change its visibility.

The NULL pointer check will also help when the Client is deleted. See below.

On 2014/08/19 14:38:27, mmenke wrote:

+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ResourceScheduler::Client::OnVisibilityChanged,
+                   base::Unretained(client_),
+                   true));

[mmenke, 2014/08/19 14:38:27]

This isn't safe.  The client may already have been deleted, and the message to
destroy the VisualObserver currently on the way to the UI thread.  Note that a
WebContents can change RenderViewHosts.

[Zhen Wang, 2014/08/19 17:53:41]

I think you are right. I override OnRenderViewHostDeleted() in VisualObser now
to reset client_ properly. My reasoning is as follows.

Background:
1. WasShown() is called on UI thread.
2. The Client is deleted when ResourceScheduler::OnClientDeleted() is called,
which is called by ResourceDispatcherHostImpl::OnRenderViewHostDeleted() on IO
thread. And ResourceDispatcherHostImpl::OnRenderViewHostDeleted() is called by
RenderViewHostImpl::~RenderViewHostImpl() on UI thread.

Case 1:
WasShown() is called before RenderViewHostImpl::~RenderViewHostImpl() on UI
thread.

In this case, ResourceScheduler::Client::OnVisibilityChanged() will be executed
before ResourceScheduler::OnClientDeleted() on IO thread. The pointer client_ is
still available. No problem.

Case 2:
RenderViewHostImpl::~RenderViewHostImpl() is called before WasShown() on UI
thread.

The destructor will post ResourceDispatcherHostImpl::OnRenderViewHostDeleted()
to IO thread, which will in turn call ResourceScheduler::OnClientDeleted().
Before switching to IO thread, the destructor will immediately call
WebContentsImpl::RenderViewDeleted(), which will call
VisualObserver::RenderViewDeleted(). Then the pointer client_ will be reset to
NULL. At this point, it is still in the destructor and WasShown() hasn't been
called yet. So later, when WasShown() is called, the NULL pointer check will
work. And eventually, the VisualObserver will be deleted after the Client is
deleted.

On 2014/08/19 14:38:27, mmenke wrote:

+}
+
+// WebContentsObserver implementation.
+void ResourceScheduler::VisualObserver::WasHidden() {
+  if (client_)
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ResourceScheduler::Client::OnVisibilityChanged,
+                   base::Unretained(client_),
+                   false));
+}
+
+ResourceScheduler::VisualObserver* ResourceScheduler::CreateVisualObserver(
+    WebContentsImpl* web_content) {
+  return new ResourceScheduler::VisualObserver(web_content);
+}
+
+void ResourceScheduler::DeleteVisualObserver(VisualObserver* observer) {
+  delete observer;
+}
+
 ResourceScheduler::ResourceScheduler()
     : should_coalesce_(false),
       should_throttle_(false),
@@ -731,12 +805,15 @@ void ResourceScheduler::RemoveRequest(ScheduledResourceRequest* request) {
   client->RemoveRequest(request);
 }
 
-void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
+void ResourceScheduler::OnClientCreated(int child_id,
+                                        int route_id,
+                                        VisualObserver* visual_observer,
+                                        bool is_visible) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
   DCHECK(!ContainsKey(client_map_, client_id));
 
-  Client* client = new Client(this);
+  Client* client = new Client(this, visual_observer, is_visible);
   client_map_[client_id] = client;
 
   // TODO(aiolos): set Client visibility/audibility when signals are added
@@ -747,6 +824,7 @@ void ResourceScheduler::OnClientCreated(int child_id, int route_id) {
 void ResourceScheduler::OnClientDeleted(int child_id, int route_id) {
   DCHECK(CalledOnValidThread());
   ClientId client_id = MakeClientId(child_id, route_id);
+
   DCHECK(ContainsKey(client_map_, client_id));
   ClientMap::iterator it = client_map_.find(client_id);
   if (it == client_map_.end())
@@ -817,14 +895,6 @@ void ResourceScheduler::OnAudibilityChanged(int child_id,
   client->OnAudibilityChanged(is_audible);
 }
 
-void ResourceScheduler::OnVisibilityChanged(int child_id,
-                                            int route_id,
-                                            bool is_visible) {
-  Client* client = GetClient(child_id, route_id);
-  DCHECK(client);
-  client->OnVisibilityChanged(is_visible);
-}
-
 void ResourceScheduler::OnLoadingStateChanged(int child_id,
                                               int route_id,
                                               bool is_loaded) {
@@ -833,6 +903,12 @@ void ResourceScheduler::OnLoadingStateChanged(int child_id,
   client->OnLoadingStateChanged(is_loaded);
 }
 
+bool ResourceScheduler::IsClientVisibleForTesting(int child_id, int route_id) {
+  Client* client = GetClient(child_id, route_id);
+  DCHECK(client);
+  return client->IsVisible();
+}
+
 ResourceScheduler::Client* ResourceScheduler::GetClient(int child_id,
                                                         int route_id) {
   ClientId client_id = MakeClientId(child_id, route_id);