Implement all resolution change policies for desktop and tab capture.

content/browser/media/capture/web_contents_tracker.cc

 // Copyright (c) 2013 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 "content/browser/media/capture/web_contents_tracker.h"
 
 #include "base/thread_task_runner_handle.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 26 matching lines @@
         BrowserThread::UI, FROM_HERE,
         base::Bind(&WebContentsTracker::StartObservingWebContents, this,
                    render_process_id, main_render_frame_id));
   }
 }
 
 void WebContentsTracker::Stop() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   callback_.Reset();
+  resize_callback_.Reset();
 
   if (BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     WebContentsObserver::Observe(NULL);
   } else {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&WebContentsTracker::Observe, this,
                    static_cast<WebContents*>(NULL)));
   }
 }
@@ skipping 14 matching lines @@
   if (!rwh) {
     RenderFrameHostImpl* const rfh =
         static_cast<RenderFrameHostImpl*>(wc->GetMainFrame());
     if (rfh)
       rwh = rfh->GetRenderWidgetHost();
   }
 
   return rwh;
 }
 
+void WebContentsTracker::SetResizeChangeCallback(
+    const ChangeCallback& callback) {
+  DCHECK(!task_runner_.get() || task_runner_->BelongsToCurrentThread());
+  resize_callback_ = callback;
+}
+
 void WebContentsTracker::OnPossibleTargetChange(bool force_callback_run) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   RenderWidgetHost* const rwh = GetTargetRenderWidgetHost();
   if (rwh == last_target_ && !force_callback_run)
     return;
   DVLOG(1) << "Will report target change from RenderWidgetHost@" << last_target_
            << " to RenderWidgetHost@" << rwh;
   last_target_ = rwh;
 
   if (task_runner_->BelongsToCurrentThread()) {
     MaybeDoCallback(rwh);
   } else {
     task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&WebContentsTracker::MaybeDoCallback, this, rwh));
   }
 }
 
 void WebContentsTracker::MaybeDoCallback(RenderWidgetHost* rwh) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!callback_.is_null())
     callback_.Run(rwh);
+  if (rwh)
+    MaybeDoResizeCallback(rwh);
+}
+
+void WebContentsTracker::MaybeDoResizeCallback(RenderWidgetHost* rwh) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (!resize_callback_.is_null())
+    resize_callback_.Run(rwh);
 }
 
 void WebContentsTracker::StartObservingWebContents(int render_process_id,
                                                    int main_render_frame_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   Observe(WebContents::FromRenderFrameHost(RenderFrameHost::FromID(
       render_process_id, main_render_frame_id)));
   DVLOG_IF(1, !web_contents())
       << "Could not find WebContents associated with main RenderFrameHost "
       << "referenced by render_process_id=" << render_process_id
       << ", routing_id=" << main_render_frame_id;
 
   OnPossibleTargetChange(true);
 }
 
 void WebContentsTracker::RenderFrameDeleted(
     RenderFrameHost* render_frame_host) {
   OnPossibleTargetChange(false);
 }
 
 void WebContentsTracker::RenderFrameHostChanged(RenderFrameHost* old_host,
                                                 RenderFrameHost* new_host) {
   OnPossibleTargetChange(false);
 }
 
+void WebContentsTracker::MainFrameWasResized(bool width_changed) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  RenderWidgetHost* const rwh = GetTargetRenderWidgetHost();
+  if (!rwh)
+    return;
+
+  if (task_runner_->BelongsToCurrentThread()) {

[Wez, 2015/05/13 19:05:09]

So the task runner is sometimes UI thread and sometimes not...?

[miu, 2015/05/14 01:21:33]

tl;dr: This is the pattern decided upon, based on previous design/impl
decisions.

WebContentsTracker is shared by both WebContentsVideoCaptureDevice and
WebContentsAudioInputStream.  The former uses this class exclusively on the UI
thread, while the latter uses it from a separate media thread.  The "tracking"
and "observing" logic is the same for both.

While, granted, the WCAudioInputStream won't ever use this new "resize
observing" logic, I wanted to remain consistent with the threading model of the
rest of this class.

[Wez, 2015/05/14 01:44:55]

Understood. The BelongsToCurrentThread() test and call is effectively a shortcut
optimization - can we make it an early exit rather than if...else?

[Wez, 2015/05/14 01:44:55]

Also, is there any way that MainFrameWasResized() could be invoked when
task_runner_ is null? e.g. could it still arrive after Stop() has been called?

[miu, 2015/05/14 21:12:26]

This seems like a matter of preference.  I like the way the if...else reads
since the code indentation helps to make it clear there are exactly two paths to
take.  Is there a particular reason you prefer the "early exit" style?

[miu, 2015/05/14 21:12:26]

No.  It is set exactly once, when Start() is called, and before the
WebContentsObserver starts observing the WebContents.  It is never changed
thereafter, even if Stop() is called.

[Wez, 2015/05/15 00:55:42]

I prefer early-exit because you could in principle have this code _always_ use
PostTask() - calling MaybeDoResizeCallback directly is a short-cut optimization,
basically.

[miu, 2015/05/15 21:14:00]

Done.  Feels like we're bike shedding here.  ;-)

+    MaybeDoResizeCallback(rwh);
+  } else {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&WebContentsTracker::MaybeDoResizeCallback, this, rwh));

[Wez, 2015/05/14 01:44:55]

What guarantees that the |rwh| will remain valid by the time this task is
processed? We pass the RWH into the |resize_callback| - are we just using it as
an identifier?

[miu, 2015/05/14 21:12:26]

Excellent point.  I looked again at the other code in WebContentsTracker that
does this.  It turns out that the RWH pointer is only dereferenced by
WCVideoCaptureDevice, which uses the pointer in the same task on the same UI
thread, so it will always be valid.  WCAudioInputStream never dereferences the
pointer: It only checks whether the pointer is null to determine whether
tracking has stopped (i.e., the tab was closed).

So, as is, the code is correct everywhere.  Though, I understand this usage
pattern isn't obvious.  I've done two things to aid future development:

1. Discussed this in the code comments in the header file where
WebContentsTracker::ChangeCallback is declared.

2. Added a InvalidatePointerForDebugBuilds() function in the .cc file that, for
debug builds, will map a non-null pointer to an invalid non-null pointer to
ensure we see a crash when the pointer is misused.

[Wez, 2015/05/15 00:55:42]

Given the usage by the two consumers, could we instead switch to void* and
update the WebContentsVideoCaptureDevice to hold its own copy of the RWH
pointer, so that it's not vulnerable to crashing if someone decides to change
the threading model in future?

AFAICT it only uses the RWH to pull out the process & routing Id, to store in
the ContentCaptureSubscription (see the ctor), so could we pass those in the
callback instead of the RWH itself?

[miu, 2015/05/15 21:14:00]

Done, but I got rid of the pointer altogether.  I realized the client could
always call WebContentsTracker::GetTargetRenderWidgetHost() on the UI thread if
it needed access to the object.

[Wez, 2015/05/15 23:31:31]

Nice!

+  }
+
+}
+
 void WebContentsTracker::WebContentsDestroyed() {
   Observe(NULL);
   OnPossibleTargetChange(false);
 }
 
 void WebContentsTracker::DidShowFullscreenWidget(int routing_id) {
   OnPossibleTargetChange(false);
 }
 
 void WebContentsTracker::DidDestroyFullscreenWidget(int routing_id) {
   OnPossibleTargetChange(false);
 }
 
 }  // namespace content