Site Isolation: RenderView-->RenderFrame for WebContentsVideoCaptureDevice and WebContentsAudioInpu…

Site Isolation: RenderView-->RenderFrame for WebContentsVideoCaptureDevice and WebContentsAudioInputStream.

The tab capture audio/video devices will now track and respond to
RenderFrame tree changes, rather than RenderView swaps, within a
WebContents.  In other words, content change events (navigation,
crashes, etc.) are now followed by observing the render frame tree
associated with a WebContents.

AudioMirroringManager was mostly re-written around the idea that
MirroringDestinations provide a query interface.  The query interface
was needed because AudioMirroringManager runs on the IO thread, while
the question "Does render frame X associate with WebContents Y?" must be
evaluated on the UI thread.

Code de-duplication: Both WebContentsVideoCaptureDevice and
WebContentsAudioInputStream now use the same tracking decision logic
provided by WebContentsTracker.  For the former, WebContentsTracker was
made to detect when hops to the UI thread were not necessary, and will
operate synchronously in that case.

BUG=304341, 392596
Committed: https://crrev.com/451dd9aa93c91692599bd2cc88f61373857d0b16
Cr-Commit-Position: refs/heads/master@{#293888}

[miu, 2014-09-05 02:59:12]

miu@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[miu, 2014-09-05 02:59:13]

Dale,

Here are the changes I'm making to wrap-up the site isolation changes for tab
capture.  As we discussed a month or so back, I've re-written
AudioMirroringManager to use an asynchronous query interface since we have to
hop over to the UI thread to look-up the WebContents for a render frame.

I would have had nick@ look at the WCVideoCaptureDevice and WCTracker stuff, but
he's on leave.  Would you be willing to take a look at them as well?  These
changes are just to de-dupe common logic, so we have the same exact render frame
tracking/switching behavior for both audio and video.

[DaleCurtis, 2014-09-05 22:41:41]

Looks good in general, but my eyes were crossing too much for this on a Friday
afternoon.  I'll give it a fresh look on Monday.  Is there another reviewer you
can add who's familiar with this code?  It's a bit daunting being the only one
and not having seen this code in a year or so :)

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
File content/browser/media/capture/audio_mirroring_manager.cc (right):

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.cc:161: // candidate to be
diverted to another destination.
It seems that the list of candidates can change between when QueryForMatches is
called and when it returns control to here.  Do you need to do any sanity checks
here?

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
File content/browser/media/capture/audio_mirroring_manager.h (right):

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.h:66: // run on the
current thread's task runner to indicate which of them (or
current is unclear in this context.  current as in the calling thread, or
current as in the replying thread?

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.h:132: void
AttemptNewMatches(MirroringDestination* old_destination,
Name could use some work. AttemptNewMatches is kind of vague.

[miu, 2014-09-08 22:41:50]

miu@chromium.org changed reviewers:
+ nick@chromium.org

[miu, 2014-09-08 22:50:54]

On 2014/09/05 22:41:41, DaleCurtis wrote:
> Is there another reviewer you
> can add who's familiar with this code?  It's a bit daunting being the only one
> and not having seen this code in a year or so :)

Yeah, sorry about that.  Unfortunately, you are next in line on the list of
those most familiar with this code: justinlin@ (audio side) is no longer working
on Chrome, and nick@ (video side) is out on leave.  I've added nick@ as TBR, and
will point this change out to him when he gets back.

If you want, I could sit down with you on Thursday to go over the code changes
and explain the things that aren't obvious.

Addressed initial comments:

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
File content/browser/media/capture/audio_mirroring_manager.cc (right):

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.cc:161: // candidate to be
diverted to another destination.
On 2014/09/05 22:41:41, DaleCurtis wrote:
> It seems that the list of candidates can change between when QueryForMatches
is
> called and when it returns control to here.  Do you need to do any sanity
checks
> here?

I do these in the loop.  Line 166 checks that the candidate hasn't been diverted
to another destination in the meantime.  Line 170 checks that the candidate is
actually being diverted to where we thought it was going before we take action
to stop it.

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
File content/browser/media/capture/audio_mirroring_manager.h (right):

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.h:66: // run on the
current thread's task runner to indicate which of them (or
On 2014/09/05 22:41:41, DaleCurtis wrote:
> current is unclear in this context.  current as in the calling thread, or
> current as in the replying thread?

Done.  Clarified comment.

https://codereview.chromium.org/542863004/diff/1/content/browser/media/captur...
content/browser/media/capture/audio_mirroring_manager.h:132: void
AttemptNewMatches(MirroringDestination* old_destination,
On 2014/09/05 22:41:41, DaleCurtis wrote:
> Name could use some work. AttemptNewMatches is kind of vague.

Done.

[DaleCurtis, 2014-09-09 00:39:09]

Spent some time going over this again today, lgtm - the multitude of test cases
were really helpful.

[miu, 2014-09-09 00:43:36]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:45:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/542863004/20001

[miu, 2014-09-09 00:45:53]

On 2014/09/09 00:39:09, DaleCurtis wrote:
> Spent some time going over this again today, lgtm - the multitude of test
cases
> were really helpful.

Thanks again for taking a look!  :)

[miu, 2014-09-09 06:05:44]

The CQ bit was unchecked by miu@chromium.org

[miu, 2014-09-09 06:05:51]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-09-09 06:14:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/miu@chromium.org/542863004/20001

[commit-bot: I haz the power, 2014-09-09 07:39:16]

Committed patchset #2 (id:20001) as 14b986b143e6237965b35cc86ffaa7fd3fc6197b

[miu, 2014-09-09 19:05:29]

miu@chromium.org changed reviewers:
- nick@chromium.org

[commit-bot: I haz the power, 2014-09-10 03:51:45]

Patchset 2 (id:??) landed as
https://crrev.com/451dd9aa93c91692599bd2cc88f61373857d0b16
Cr-Commit-Position: refs/heads/master@{#293888}