[Cross-Site Isolation] Migrate entire MediaStream verticals to be per-RenderFrame.

[Cross-Site Isolation] Migrate entire MediaStream verticals to be per-RenderFrame.

Much of this change is search-and-replace RenderView-->RenderFrame and
render_view_id-->render_frame_id.  However, the following significant
changes have also been made, mainly to simplify/clarify/clean-up the
same LOC that would have had to be changed anyway:

1. Desktop and Tab capture APIs now "register" a WebContents instance,
   rather than a RenderViewHost.  This is a more-appropriate choice than
   registering a RenderFrameHost, and simplifies a lot of code.
2. Removed unnecessary "prepending" and "stripping" of the first part of
   the tab capture device ID in special circumstances.  It is no longer
   necessary.
3. TabCaptureRegistry uses WebContentsObserver to detect all fullscreen
   changes, rather than the deprecated NotificationObserver scheme.
4. Fixes for buggy MediaStreamImpl tear-down: MediaStreamDispatcher is
   now explicitly owned by MediaStreamImpl, to ensure it is destroyed
   only after it is no longer needed.  Also, weak pointers to
   MediaStreamImpl are invalidated before MSI data members are destroyed
   (it was observed during testing that outstanding callbacks were
   firing *after* the data members were destroyed).

Testing:

1. All relevant content_unittests and browser_tests run.
2. Manually engaged tab capture and desktop capture using Google Cast
   extension and "Desktop Capture Example"
   (https://developer.chrome.com/extensions/samples) to test screen
   picker UI.
3. Ran WebRTC demo: http://apprtc.appspot.com
4. Confirmed Flash webcam and microphone input.
5. Checked tab capture/recording/audio indicators.
6. Confirmed HTML5 and Flash fullscreen-within-tab functionality during
   tab capture.

BUG=304341, 323223, 320200, 163100, 338445
TEST=See "Testing" above.
TBR=kenrb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283702

[miu, 2014-07-08 23:16:23]

Please review a subset of the files:

jam:
 chrome/browser/ui/ash/media_delegate_chromeos.*
 content/browser/frame_host/render_frame_host_delegate.*
 content/browser/renderer_host/render_view_host_delegate.h
 content/public/*
 content/renderer/render_frame_impl.*
 content/renderer/render_view_impl.*
 content/shell/renderer/test_runner/web_test_proxy.h

nick:
 chrome/browser/extensions/api/tab_capture/*
 content/browser/media/capture/*

sergeyu:
 chrome/browser/extensions/api/desktop_capture/desktop_capture_api.*
 chrome/browser/media/desktop_streams_registry.*
 chrome/browser/media/media_capture_devices_dispatcher.*

tommi:
 content/common/media/*
 content/browser/renderer_host/media/*
 content/browser/speech/*
 content/renderer/media/*

yzshen:
 content/renderer/pepper/*

[tommi (sloooow) - chröme, 2014-07-09 07:46:55]

Nice!!! Thanks for doing this. Just a couple of questions.

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:290:
WebContentsCaptureUtil::StripWebContentsDeviceScheme(
I'm not familiar with what this does/did.  Are there no side effects of
removing?

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:14: // migrated for the
Cross-Site Isolation project.
is there a bug to track these todos?

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:55: return
media_stream_dispatcher_.get();
add a thread check?

[ncarter (slow), 2014-07-09 17:56:02]

+site-istolation-reviews

[Sergey Ulanov, 2014-07-09 18:06:25]

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:96:
content::WebContents* web_contents_for_picker = NULL;
The same WebContents should be used for both picker and for the stream, i.e.
picker should be shown in the context of the tab/window that will be using the
stream.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142:
web_contents_for_picker = GetAssociatedWebContents();
I think this will always return the same WebContents that you get in line 140
(WebContents this call came from).

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.h
(right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.h:61: // URL
of page that used the API to initiate desktop capture.
This comment is not correct. Desktop Capture API allows an extension to request
destkop capture stream for another origin. This should be, e.g. "URL of the page
that desktop capture was requested for"

[miu, 2014-07-09 19:48:07]

Responses for tommi (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:290:
WebContentsCaptureUtil::StripWebContentsDeviceScheme(
On 2014/07/09 07:46:55, tommi wrote:
> I'm not familiar with what this does/did.  Are there no side effects of
> removing?

A couple years ago, MediaStreamImpl/Dispatcher/Manager were in an ugly state
where we had to hack the device IDs for some reason.  The person who would know
exactly why is no longer with us, and there don't seem to be any comments
anywhere explaining the reason for it all.

I looked through the code paths and didn't see any reason for the
stripping/unstripping, so I removed it.  Also, be assured, I have tested that
tab capture was not broken by this.

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_audio_source.cc:14: // migrated for the
Cross-Site Isolation project.
On 2014/07/09 07:46:55, tommi wrote:
> is there a bug to track these todos?

Done.  crbug.com/392596

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/364123002/diff/100001/content/renderer/media/...
content/renderer/media/media_stream_impl.h:55: return
media_stream_dispatcher_.get();
On 2014/07/09 07:46:55, tommi wrote:
> add a thread check?

A valid concern, but I think it's unnecessary: 1) MSD lives as long as
MediaStreamImpl, since the latter now owns it.  2) All MSD methods have thread
checking, guaranteeing any use of the instance is on the right thread.

[miu, 2014-07-09 23:28:05]

Responses for sergeyu (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:96:
content::WebContents* web_contents_for_picker = NULL;
On 2014/07/09 18:06:24, Sergey Ulanov wrote:
> The same WebContents should be used for both picker and for the stream, i.e.
> picker should be shown in the context of the tab/window that will be using the
> stream.

Done.  However, I should point out I wasn't actually changing the prior behavior
here.  Please see next comment response for more details...

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:142:
web_contents_for_picker = GetAssociatedWebContents();
On 2014/07/09 18:06:24, Sergey Ulanov wrote:
> I think this will always return the same WebContents that you get in line 140
> (WebContents this call came from).

It doesn't.  The DesktopCaptureApiTest.ChooseDesktopMedia test was failing for
the chooseMediaAndGetStream() step.  The root cause was that
GetAssociatedWebContents() was returning the WebContents for the currently
active tab, rather than the one requesting the stream.  See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

I've fixed everything to use the WebContents for the current JS context (the
consumer of the stream).  Let me know if I should revert.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.h
(right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.h:61: // URL
of page that used the API to initiate desktop capture.
On 2014/07/09 18:06:25, Sergey Ulanov wrote:
> This comment is not correct. Desktop Capture API allows an extension to
request
> destkop capture stream for another origin. This should be, e.g. "URL of the
page
> that desktop capture was requested for"

Done.

[ncarter (slow), 2014-07-10 01:17:52]

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:61: if
(render_view_host()) {
As you are probably aware, these would ideally be render_frame_host()'s, but the
ExtensionFunctionDispatcher isn't quite wired up to let that happen yet.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:134:
"web-contents-media-stream://%i:%i",
Does introducing the scheme here fix a bug? How is this different from the
scheme applied by web_contents_capture_util?

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:27: // is a
valid pointer (i.e., it will be dereferenced).
Just say: "... is not NULL". The rest is implicit.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:172: //
deleted.  Probably the root cause of http://crbug.com/338445.
If the entries of |requests_| are not properly cleaned up, then the resulting
behavior is more of a problem after this CL. This is because an invalid
WebContents* pointer value in |request_| is unsafe in some situations where a
routing pair would be safe. Since they're never reused, routing pairs can be
reasoned about even after their referand is destroyed, but pointers carry no
such guarantee.

One way to possibly fix this, would be to have the FullscreenObserver override
WebContentsDestroyed, and pass the event on to the TabCaptureRegistry. This
might require installing the observer earlier, from the moment the request is
created, since there doesn't seem to be anything preventing a WebContents
disappearing between TabCaptureRegistry::AddRequest and
content::MEDIA_REQUEST_STATE_DONE . And if you're then always observing the
WebContents, there's nothing stopping you from merging TabCaptureRequest and
FullscreenObserver into a single class.

An alternative approach for handling this, would be to look up the WebContents
as needed, based on the tab_id.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (left):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:35: public
content::NotificationObserver,
You are a saint.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:52: bool
AddRequest(content::WebContents* target_contents,
Could you document the parameters here? In particular, could make it 100%
unambiguous that |target_contents| and |tab_id| represent the tab being
captured, and not the tab requesting the capture?

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:54: int tab_id,
If we have the |target_contents|, do we need the tab_id as well?

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:58: // the
stream by checking the registry here.
What about the request, exactly, is verified? It might be helpful to give an
example of when this verification might fail.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.cc:69: web_contents !=
it->second.web_contents ||
You can only match against a pointer value like this if you can guarantee that
the value in the map is not stored past the lifetime of the WebContents --
otherwise the memory, once freed, could be reallocated as a different
WebContents. Routing pairs were not unsafe in that way, since they are never
reused.

I think this file should probably go back to using a process_id / routing_id
pair, with the routing_id becoming a RFH routing ID instead of an RVH one.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.h (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.h:32: std::string
RegisterStream(const content::WebContents* web_contents,
My expectation of which types to use where goes something like:
 - WebContents* to be used when we want to identify a tab whose output is being
captured.
 - A RenderFrameHost (or routing pair identifying a rfh) to be used to identify
the initiator of the capture. Because it's always going to be a particular
document that requests a capture stream.

A WebContents, over its lifetime, may navigate from a chrome-extension:// page
to a http://evil.com page and back. Our design must ensure that extensionlike
abilities aren't granted to evil.com; we do that by scoping this kind of
permission to a RenderFrameHost.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.h:53: const content::WebContents*
web_contents;  // Never dereference this!
Because of the possibility of address reuse, pointers that are unsafe to
dereference are also unsafe to use as keys.

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_input_stream.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_input_stream.cc:28: Impl(int
render_process_id, int main_render_frame_id,
Interesting, this points to a possible hurdle (which in no way needs to be
addressed now). Do you think it will be feasible, in oopif, to capture audio
from all the (cross-process) frames of a page?

Situations like a youtube player inside of an iframe could be imagined to be
fairly common.

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_input_stream.cc:124: //
isolation).
So -- it seems like this workaround is needed because this object is created on
the IO thread to target an initial frame routing_id, but some of the code hasn't
been switched over to use frame routing_ids -- and because we're not on the UI
thread, we can't easily get from the frame to the view.

I'd consider using a bool parameter (target_identified_ ?) on the side, instead
of using kintmax (which usually means MSG_ROUTING_CONTROL). With the boolean you
probably wouldn't need quite as long a comment.

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/DEPS (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/DEPS:7: # friends to somewhere more
appropriate (content/browser/frame_host/media?).
I'm not opposed to content/browser/frame_host/media, but I also haven't given it
too much thought.

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:290:
WebContentsCaptureUtil::StripWebContentsDeviceScheme(
On 2014/07/09 19:48:07, miu wrote:
> On 2014/07/09 07:46:55, tommi wrote:
> > I'm not familiar with what this does/did.  Are there no side effects of
> > removing?
> 
> A couple years ago, MediaStreamImpl/Dispatcher/Manager were in an ugly state
> where we had to hack the device IDs for some reason.  The person who would
know
> exactly why is no longer with us, and there don't seem to be any comments
> anywhere explaining the reason for it all.
> 
> I looked through the code paths and didn't see any reason for the
> stripping/unstripping, so I removed it.  Also, be assured, I have tested that
> tab capture was not broken by this.

FWIW, jam@ and I were chatting about this a few weeks back and came to the same
conclusion, that it seemed to be a kind of no-op.

I always thought the scheme was there to prevent possible collisions between
local video capture device IDs, and the virtual ones we were adding, but the
details are fairly fuzzy and I have nothing to compare it to now.

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.cc:115:
render_view_host->GetMainFrame()->GetRoutingID() : MSG_ROUTING_NONE;
render_view_host->GetMainFrame() is pretty awful but it's there for hacks like
this.

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.h:71: int
params_render_frame_id,
does the "params_" in this name make sense?

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:197:
DCHECK_NE(context.render_frame_id, 0);
Do you want to check "!= 0" or do you want to check "> 0"? Are you okay if it's
MSG_ROUTING_NONE?

[tommi (sloooow) - chröme, 2014-07-10 13:12:55]

lgtm

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
File content/browser/renderer_host/media/media_stream_manager.cc (left):

https://codereview.chromium.org/364123002/diff/100001/content/browser/rendere...
content/browser/renderer_host/media/media_stream_manager.cc:290:
WebContentsCaptureUtil::StripWebContentsDeviceScheme(
On 2014/07/10 01:17:51, ncarter wrote:
> On 2014/07/09 19:48:07, miu wrote:
> > On 2014/07/09 07:46:55, tommi wrote:
> > > I'm not familiar with what this does/did.  Are there no side effects of
> > > removing?
> > 
> > A couple years ago, MediaStreamImpl/Dispatcher/Manager were in an ugly state
> > where we had to hack the device IDs for some reason.  The person who would
> know
> > exactly why is no longer with us, and there don't seem to be any comments
> > anywhere explaining the reason for it all.
> > 
> > I looked through the code paths and didn't see any reason for the
> > stripping/unstripping, so I removed it.  Also, be assured, I have tested
that
> > tab capture was not broken by this.
> 
> FWIW, jam@ and I were chatting about this a few weeks back and came to the
same
> conclusion, that it seemed to be a kind of no-op.
> 
> I always thought the scheme was there to prevent possible collisions between
> local video capture device IDs, and the virtual ones we were adding, but the
> details are fairly fuzzy and I have nothing to compare it to now.

Great. The fewer mysteries the better.

[miu, 2014-07-10 22:16:12]

Responses for nick (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:134:
"web-contents-media-stream://%i:%i",
On 2014/07/10 01:17:51, ncarter wrote:
> Does introducing the scheme here fix a bug? How is this different from the
> scheme applied by web_contents_capture_util?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

It addresses crbug.com/163100.  There isn't really a difference, except that the
code in WCCaptureUtil to "append" and "strip" the scheme was deemed unnecessary
due to the major refactoring work done in MediaStreamManager.  In short, the
device ID string can just be generated here, and then remain unmodified
throughout the media request pipeline.

Also, per the TODO comment here, I'd like to see the tab capture API employ the
secure mechanism used by the desktop capture API, where random device ID strings
are created and then later verified.  Thus, the direction I'm starting to take
here is to move device ID string generation into the chrome/ layer; and
ultimately I'd like to eliminate the parsing of the device ID string in
libcontent entirely (just like desktop capture).

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:27: // is a
valid pointer (i.e., it will be dereferenced).
On 2014/07/10 01:17:51, ncarter wrote:
> Just say: "... is not NULL". The rest is implicit.

I think this was left over from earlier prototyping on my desktop.  Instead of
the comment, I've made the intention clear/documented in the form of a
DCHECK(web_contents()) in the ctor body.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:172: //
deleted.  Probably the root cause of http://crbug.com/338445.
On 2014/07/10 01:17:51, ncarter wrote:
> If the entries of |requests_| are not properly cleaned up, then the resulting
> behavior is more of a problem after this CL. This is because an invalid
> WebContents* pointer value in |request_| is unsafe in some situations where a
> routing pair would be safe. Since they're never reused, routing pairs can be
> reasoned about even after their referand is destroyed, but pointers carry no
> such guarantee.
> 
> One way to possibly fix this, would be to have the FullscreenObserver override
> WebContentsDestroyed, and pass the event on to the TabCaptureRegistry. This
> might require installing the observer earlier, from the moment the request is
> created, since there doesn't seem to be anything preventing a WebContents
> disappearing between TabCaptureRegistry::AddRequest and
> content::MEDIA_REQUEST_STATE_DONE . And if you're then always observing the
> WebContents, there's nothing stopping you from merging TabCaptureRequest and
> FullscreenObserver into a single class.
> 
> An alternative approach for handling this, would be to look up the WebContents
> as needed, based on the tab_id.

Done.

I used exactly your suggested approach of merging TabCaptureRequest with
FullscreenObserver into a new class called LiveRequest.  Then, there was a ton
of obvious clean-up exposed, which I took care of.  Also, I accidentally fixed a
number of bugs.  ;-)  Sorry about the huge patch delta, but I think the
improvement seems to warrant it.

Also, updated chrome/test/data/extensions/api_tests/tab_capture/api_test.js to
test that getCapturedTabs() works correctly since I fixed the "stale infos" bug
(crbug.com/338445).

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (left):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:35: public
content::NotificationObserver,
On 2014/07/10 01:17:51, ncarter wrote:
> You are a saint.

:)

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.h (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:52: bool
AddRequest(content::WebContents* target_contents,
On 2014/07/10 01:17:51, ncarter wrote:
> Could you document the parameters here? In particular, could make it 100%
> unambiguous that |target_contents| and |tab_id| represent the tab being
> captured, and not the tab requesting the capture?

Done.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:54: int tab_id,
On 2014/07/10 01:17:51, ncarter wrote:
> If we have the |target_contents|, do we need the tab_id as well?

Excellent point.  In fact, I was able to remove both this and the last argument.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.h:58: // the
stream by checking the registry here.
On 2014/07/10 01:17:51, ncarter wrote:
> What about the request, exactly, is verified? It might be helpful to give an
> example of when this verification might fail.

Done.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.cc:69: web_contents !=
it->second.web_contents ||
On 2014/07/10 01:17:51, ncarter wrote:
> You can only match against a pointer value like this if you can guarantee that
> the value in the map is not stored past the lifetime of the WebContents --
> otherwise the memory, once freed, could be reallocated as a different
> WebContents. Routing pairs were not unsafe in that way, since they are never
> reused.
> 
> I think this file should probably go back to using a process_id / routing_id
> pair, with the routing_id becoming a RFH routing ID instead of an RVH one.

Done.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.h (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.h:53: const content::WebContents*
web_contents;  // Never dereference this!
On 2014/07/10 01:17:51, ncarter wrote:
> Because of the possibility of address reuse, pointers that are unsafe to
> dereference are also unsafe to use as keys.

Good insight.  Fixed.

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_input_stream.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_input_stream.cc:28: Impl(int
render_process_id, int main_render_frame_id,
On 2014/07/10 01:17:51, ncarter wrote:
> Interesting, this points to a possible hurdle (which in no way needs to be
> addressed now). Do you think it will be feasible, in oopif, to capture audio
> from all the (cross-process) frames of a page?
> 
> Situations like a youtube player inside of an iframe could be imagined to be
> fairly common.

Yes.  I've prototyped a solution that seems to work well, with some unfortunate
added complexity: The problem at-hand is that AudioMirroringManager is trying to
match audio streams, associated by render frame routing IDs, against
MirroringDestinations that want the streams from multiple render frames (for a
tab).  To accomplish this, I've prototyped an asynchronous "query" API where
AudioMirroringManager will ask a MirroringDestination whether it wants to
consume audio for a specific render frame.  The API has to be asynchronous
because we have to hop from the IO thread to the UI thread to execute such a
query.

Once this change lands, I'll rebase my prototype, determine whether it still
looks like the right solution, and clean it up for review.

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_input_stream.cc:124: //
isolation).
On 2014/07/10 01:17:51, ncarter wrote:
> So -- it seems like this workaround is needed because this object is created
on
> the IO thread to target an initial frame routing_id, but some of the code
hasn't
> been switched over to use frame routing_ids -- and because we're not on the UI
> thread, we can't easily get from the frame to the view.
> 
> I'd consider using a bool parameter (target_identified_ ?) on the side,
instead
> of using kintmax (which usually means MSG_ROUTING_CONTROL). With the boolean
you
> probably wouldn't need quite as long a comment.

Done.

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
File content/browser/speech/speech_recognition_dispatcher_host.h (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
content/browser/speech/speech_recognition_dispatcher_host.h:71: int
params_render_frame_id,
On 2014/07/10 01:17:52, ncarter wrote:
> does the "params_" in this name make sense?

After looking at this code again, I reverted all changes to
speech_recognition_dispatcher_host.*, and instead put in a
RenderView-->RenderFrame hack in speech_recognition_manager_impl.cc.

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:197:
DCHECK_NE(context.render_frame_id, 0);
On 2014/07/10 01:17:52, ncarter wrote:
> Do you want to check "!= 0" or do you want to check "> 0"? Are you okay if
it's
> MSG_ROUTING_NONE?

My intention was to confirm the value was being set, but I think I should
probably remove this DCHECK and guard the whole call to MakeMediaAccessRequest()
with a sanity-check.  In other words, we shouldn't be making the request unless
the RP ID and RF ID are both valid.

[miu, 2014-07-10 22:17:55]

kenrb: Need OWNERS approval for content/common/media/media_stream_messages.h

[ncarter (slow), 2014-07-11 22:32:25]

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_api.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_api.cc:134:
"web-contents-media-stream://%i:%i",
On 2014/07/10 22:16:11, miu wrote:
> On 2014/07/10 01:17:51, ncarter wrote:
> > Does introducing the scheme here fix a bug? How is this different from the
> > scheme applied by web_contents_capture_util?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...
> 
> It addresses crbug.com/163100.  There isn't really a difference, except that
the
> code in WCCaptureUtil to "append" and "strip" the scheme was deemed
unnecessary
> due to the major refactoring work done in MediaStreamManager.  In short, the
> device ID string can just be generated here, and then remain unmodified
> throughout the media request pipeline.
> 
> Also, per the TODO comment here, I'd like to see the tab capture API employ
the
> secure mechanism used by the desktop capture API, where random device ID
strings
> are created and then later verified.  Thus, the direction I'm starting to take
> here is to move device ID string generation into the chrome/ layer; and
> ultimately I'd like to eliminate the parsing of the device ID string in
> libcontent entirely (just like desktop capture).

Acknowledged.

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/364123002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:172: //
deleted.  Probably the root cause of http://crbug.com/338445.
On 2014/07/10 22:16:11, miu wrote:
> On 2014/07/10 01:17:51, ncarter wrote:
> > If the entries of |requests_| are not properly cleaned up, then the
resulting
> > behavior is more of a problem after this CL. This is because an invalid
> > WebContents* pointer value in |request_| is unsafe in some situations where
a
> > routing pair would be safe. Since they're never reused, routing pairs can be
> > reasoned about even after their referand is destroyed, but pointers carry no
> > such guarantee.
> > 
> > One way to possibly fix this, would be to have the FullscreenObserver
override
> > WebContentsDestroyed, and pass the event on to the TabCaptureRegistry. This
> > might require installing the observer earlier, from the moment the request
is
> > created, since there doesn't seem to be anything preventing a WebContents
> > disappearing between TabCaptureRegistry::AddRequest and
> > content::MEDIA_REQUEST_STATE_DONE . And if you're then always observing the
> > WebContents, there's nothing stopping you from merging TabCaptureRequest and
> > FullscreenObserver into a single class.
> > 
> > An alternative approach for handling this, would be to look up the
WebContents
> > as needed, based on the tab_id.
> 
> Done.
> 
> I used exactly your suggested approach of merging TabCaptureRequest with
> FullscreenObserver into a new class called LiveRequest.  Then, there was a ton
> of obvious clean-up exposed, which I took care of.  Also, I accidentally fixed
a
> number of bugs.  ;-)  Sorry about the huge patch delta, but I think the
> improvement seems to warrant it.
> 
> Also, updated chrome/test/data/extensions/api_tests/tab_capture/api_test.js to
> test that getCapturedTabs() works correctly since I fixed the "stale infos"
bug
> (crbug.com/338445).

Wonderful, glad it worked out.

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
File content/browser/media/capture/web_contents_audio_input_stream.cc (right):

https://codereview.chromium.org/364123002/diff/100001/content/browser/media/c...
content/browser/media/capture/web_contents_audio_input_stream.cc:28: Impl(int
render_process_id, int main_render_frame_id,
On 2014/07/10 22:16:12, miu wrote:
> On 2014/07/10 01:17:51, ncarter wrote:
> > Interesting, this points to a possible hurdle (which in no way needs to be
> > addressed now). Do you think it will be feasible, in oopif, to capture audio
> > from all the (cross-process) frames of a page?
> > 
> > Situations like a youtube player inside of an iframe could be imagined to be
> > fairly common.
> 
> Yes.  I've prototyped a solution that seems to work well, with some
unfortunate
> added complexity: The problem at-hand is that AudioMirroringManager is trying
to
> match audio streams, associated by render frame routing IDs, against
> MirroringDestinations that want the streams from multiple render frames (for a
> tab).  To accomplish this, I've prototyped an asynchronous "query" API where
> AudioMirroringManager will ask a MirroringDestination whether it wants to
> consume audio for a specific render frame.  The API has to be asynchronous
> because we have to hop from the IO thread to the UI thread to execute such a
> query.
> 
> Once this change lands, I'll rebase my prototype, determine whether it still
> looks like the right solution, and clean it up for review.

Pretty cool. Glad to hear you're on top of it.

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:207:
render_process_id, render_frame_id)));
Before this change, it seems that the original ID pair used to open the stream
would be a reliable identifier throughout the life of the the capture request.
Even if the underlying RVH/RFH went away.

But after this change, if the original view goes away (due to navigation, say),
it seems like we'll no longer be able to update the request, because the
RenderFrameHost will have gone away.

Since CLOSE is one of the supported update operations here -- which presumably
can happen a long time after the stream is created, allowing plenty of time for
navigation to occur  -- I'm wondering if there's a regression in behavior here.
Will we now orphan a stream after the target WebContents does a cross-process
navigation? Navigating by typing a URL should usually trigger a process swap
(clicked links sometimes have reasons to stay in process). You can double-check
by looking at the process id for the tab in the TaskManager.

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:419:
main_frame->GetRoutingID(),
This ultimately should be request.render_process_id/request.render_frame ID,
right?

Why wouldn't that work right now?

https://codereview.chromium.org/364123002/diff/160001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/364123002/diff/160001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:204:
context.render_process_id, context.render_view_id);
This code seems misplaced; you can't get an RVH on the IO thread. Should you
just go back to how it was in patch set 1?

[miu, 2014-07-12 03:16:29]

nick: PTAL (PS4-->PS6).

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc (right):

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_registry.cc:207:
render_process_id, render_frame_id)));
On 2014/07/11 22:32:24, ncarter wrote:
> Before this change, it seems that the original ID pair used to open the stream
> would be a reliable identifier throughout the life of the the capture request.
> Even if the underlying RVH/RFH went away.
> 
> But after this change, if the original view goes away

I was able to confirm, as you feared, that this is a problem.  I came up with a
solution that I'll let the delta between PS4 and PS6 explain for itself (code
comments explain the problem, temporary solution, and permanent solution).

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:419:
main_frame->GetRoutingID(),
On 2014/07/11 22:32:24, ncarter wrote:
> This ultimately should be request.render_process_id/request.render_frame ID,
> right?
> 
> Why wouldn't that work right now?

See TODO(miu) comment about 10 lines above.  ;-)

The problem is due to ExtensionFunction currently only supporting use of
render_view_host() and not render_frame_host().  The desktop capture API impl
has no way of knowing the exact RF that requested the stream, so it registers
the request with DesktopStreamsRegistry using the main RF's ID.  Therefore,
here, we have no choice but to request the media stream by the main RF ID.

https://codereview.chromium.org/364123002/diff/160001/content/browser/speech/...
File content/browser/speech/speech_recognition_manager_impl.cc (right):

https://codereview.chromium.org/364123002/diff/160001/content/browser/speech/...
content/browser/speech/speech_recognition_manager_impl.cc:204:
context.render_process_id, context.render_view_id);
On 2014/07/11 22:32:25, ncarter wrote:
> This code seems misplaced; you can't get an RVH on the IO thread. Should you
> just go back to how it was in patch set 1?

Done.  Yeah, I oops'ed; and also noticed the trybots caught it.  I put it back
the way it was.

[yzshen1, 2014-07-14 17:19:55]

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:75:
GetMediaStreamDispatcher()->StopEnumerateDevices(request_id, AsWeakPtr());
Is it safe to remove this PostTask()?

If I understand the code correctly:
This method may be called while MediaStreamDispatcher::OnDevicesEnumerated() is
on the stack, without PostTask() it mutates the |requests_| list directly, and
causes the |it| iterator in OnDeviceEnumerated() to be invalidated before it
does ++it.

Right?

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:108: succeeded &=
!!device_manager;
nit: it seems more natural to use logical AND instead of bitwise AND.

[ncarter (slow), 2014-07-14 17:51:05]

lgtm

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
File chrome/browser/media/media_capture_devices_dispatcher.cc (right):

https://codereview.chromium.org/364123002/diff/160001/chrome/browser/media/me...
chrome/browser/media/media_capture_devices_dispatcher.cc:419:
main_frame->GetRoutingID(),
On 2014/07/12 03:16:29, miu wrote:
> On 2014/07/11 22:32:24, ncarter wrote:
> > This ultimately should be request.render_process_id/request.render_frame ID,
> > right?
> > 
> > Why wouldn't that work right now?
> 
> See TODO(miu) comment about 10 lines above.  ;-)
> 
> The problem is due to ExtensionFunction currently only supporting use of
> render_view_host() and not render_frame_host().  The desktop capture API impl
> has no way of knowing the exact RF that requested the stream, so it registers
> the request with DesktopStreamsRegistry using the main RF's ID.  Therefore,
> here, we have no choice but to request the media stream by the main RF ID.

Ah got it now. I'd seen the TODO but it hadn't quite clicked. So it's not that
request.render_frame_id isn't a valid frame (it is), but rather that the ID we
use here has to match code elsewhere.

[miu, 2014-07-14 19:41:28]

Responses for yzshen (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:75:
GetMediaStreamDispatcher()->StopEnumerateDevices(request_id, AsWeakPtr());
On 2014/07/14 17:19:55, yzshen1 wrote:
> Is it safe to remove this PostTask()?
> 
> If I understand the code correctly:
> This method may be called while MediaStreamDispatcher::OnDevicesEnumerated()
is
> on the stack, without PostTask() it mutates the |requests_| list directly, and
> causes the |it| iterator in OnDeviceEnumerated() to be invalidated before it
> does ++it.
> 
> Right?

Done.

I removed it because, at one point, I managed to convince myself the "mechanism"
here avoided the re-entrancy issue:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

However, there are a lot of complex interactions at-play here, so I've decided
to take the safer route and put back the PostTask().  Note, the code here still
had to change because MediaStreamDispatcher no longer provides WeakPtrs to
non-private code.

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:87: if (render_frame())
FYI--Checking that render_frame() is not NULL to guarantee
GetMediaStreamDispatcher() won't return NULL.  (This method is being invoked by
the message loop at some unknown point-in-time after StopEnumerateDevices() was
called.)

[yzshen1, 2014-07-14 21:15:33]

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:75:
GetMediaStreamDispatcher()->StopEnumerateDevices(request_id, AsWeakPtr());
> I removed it because, at one point, I managed to convince myself the
"mechanism"
> here avoided the re-entrancy issue:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...
> 
I don't think the mechanism can avoid the re-entrancy issue. Even if the
enumeration results are returned asynchronously, 
MediaStreamDispatcher::OnDevicesEnumerated() will be on the stack.

> However, there are a lot of complex interactions at-play here, so I've decided
> to take the safer route and put back the PostTask().  Note, the code here
still
> had to change because MediaStreamDispatcher no longer provides WeakPtrs to
> non-private code.

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:108: succeeded &=
!!device_manager;
On 2014/07/14 17:19:55, yzshen1 wrote:
> nit: it seems more natural to use logical AND instead of bitwise AND.

It seems you haven't replied to this comment.

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:87: if (render_frame())
On 2014/07/14 19:41:28, miu wrote:
> FYI--Checking that render_frame() is not NULL to guarantee
> GetMediaStreamDispatcher() won't return NULL. 

It seems good to make this a comment?

 (This method is being invoked by
> the message loop at some unknown point-in-time after StopEnumerateDevices()
was
> called.)

[ncarter (slow), 2014-07-14 21:24:11]

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:108: succeeded &=
!!device_manager;
On 2014/07/14 21:15:32, yzshen1 wrote:
> On 2014/07/14 17:19:55, yzshen1 wrote:
> > nit: it seems more natural to use logical AND instead of bitwise AND.
> 
> It seems you haven't replied to this comment.

it's a matter of style, but I agree with yzshen. I think it the original form
using && reads like more natural C++.

[ncarter (slow), 2014-07-14 21:24:11]

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:108: succeeded &=
!!device_manager;
On 2014/07/14 21:15:32, yzshen1 wrote:
> On 2014/07/14 17:19:55, yzshen1 wrote:
> > nit: it seems more natural to use logical AND instead of bitwise AND.
> 
> It seems you haven't replied to this comment.

it's a matter of style, but I agree with yzshen. I think it the original form
using && reads like more natural C++.

[miu, 2014-07-15 00:09:14]

Responses for yzshen (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
File content/renderer/pepper/pepper_platform_video_capture.cc (right):

https://codereview.chromium.org/364123002/diff/200001/content/renderer/pepper...
content/renderer/pepper/pepper_platform_video_capture.cc:108: succeeded &=
!!device_manager;
On 2014/07/14 21:15:32, yzshen1 wrote:
> On 2014/07/14 17:19:55, yzshen1 wrote:
> > nit: it seems more natural to use logical AND instead of bitwise AND.
> 
> It seems you haven't replied to this comment.

Done.  No disagreement here, I just missed the comment.  ;-)

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
File content/renderer/pepper/pepper_media_device_manager.cc (right):

https://codereview.chromium.org/364123002/diff/240001/content/renderer/pepper...
content/renderer/pepper/pepper_media_device_manager.cc:87: if (render_frame())
On 2014/07/14 21:15:33, yzshen1 wrote:
> On 2014/07/14 19:41:28, miu wrote:
> > FYI--Checking that render_frame() is not NULL to guarantee
> > GetMediaStreamDispatcher() won't return NULL. 
> 
> It seems good to make this a comment?
> 
>  (This method is being invoked by
> > the message loop at some unknown point-in-time after StopEnumerateDevices()
> was
> > called.)
> 

Done.

[yzshen1, 2014-07-15 00:27:28]

.*pepper.* LGTM

Thanks!

[jam, 2014-07-15 02:04:08]

lgtm, HUGE THANKS for converting this code!

https://codereview.chromium.org/364123002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/364123002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:131:
web_contents = content::WebContents::FromRenderViewHost(render_view_host());
i'm curious why you changed this from "web_contents =
GetAssociatedWebContents();"?

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/media/DEPS (right):

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
content/browser/renderer_host/media/DEPS:7: # friends to somewhere more
appropriate (content/browser/frame_host/media?).
or move this code to content/browser/media?

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:16: #include
"content/public/common/media_stream_request.h"
nit: not needed anymore?

https://codereview.chromium.org/364123002/diff/280001/content/public/browser/...
File content/public/browser/speech_recognition_session_context.h (right):

https://codereview.chromium.org/364123002/diff/280001/content/public/browser/...
content/public/browser/speech_recognition_session_context.h:29: int
render_frame_id;
it's redundant to have render_view_id and render_frame_id, since
RenderFrameHost::FromID(render_process_id, render_frame_id)->GetRenderViewHost()
provides the RVH. it's problematic to have both RF and RV ID since it could
result in an inconsistent state (where the RenderFrameHost's WC isn't the same
as the RenderViewHost's WC).

can you remove render_view_id and call
RenderFrameHost::FromID(render_process_id, render_frame_id)->GetRenderViewHost()
everywhere that we temporarily need the render view id for now?

[miu, 2014-07-15 20:37:55]

Responses for jam (addressing other reviewer's comments separately):

https://codereview.chromium.org/364123002/diff/280001/chrome/browser/extensio...
File chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc
(right):

https://codereview.chromium.org/364123002/diff/280001/chrome/browser/extensio...
chrome/browser/extensions/api/desktop_capture/desktop_capture_api.cc:131:
web_contents = content::WebContents::FromRenderViewHost(render_view_host());
On 2014/07/15 02:04:08, jam wrote:
> i'm curious why you changed this from "web_contents =
> GetAssociatedWebContents();"?

I had a discussion with sergeyu (original author of this code), and when he saw
how I was doing things in PS1, he said the picker should always be
shown/anchored to the browser owning the WebContents that consumes the stream. 
Thus, the logic change.

(See previous discussion in code review comments for more details.  tl;dr: A
unit test revealed a faulty assumption in the original implementation.)

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/media/DEPS (right):

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
content/browser/renderer_host/media/DEPS:7: # friends to somewhere more
appropriate (content/browser/frame_host/media?).
On 2014/07/15 02:04:08, jam wrote:
> or move this code to content/browser/media?

Changed comment.  I'll poke around to see who is willing to take this on, since
it's mainly one/more of the Chrome Media team's ownership concern.

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/364123002/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_view_host_delegate.h:16: #include
"content/public/common/media_stream_request.h"
On 2014/07/15 02:04:08, jam wrote:
> nit: not needed anymore?

Done.

https://codereview.chromium.org/364123002/diff/280001/content/public/browser/...
File content/public/browser/speech_recognition_session_context.h (right):

https://codereview.chromium.org/364123002/diff/280001/content/public/browser/...
content/public/browser/speech_recognition_session_context.h:29: int
render_frame_id;
On 2014/07/15 02:04:08, jam wrote:
> it's redundant to have render_view_id and render_frame_id, since
> RenderFrameHost::FromID(render_process_id,
render_frame_id)->GetRenderViewHost()
> provides the RVH. it's problematic to have both RF and RV ID since it could
> result in an inconsistent state (where the RenderFrameHost's WC isn't the same
> as the RenderViewHost's WC).
> 
> can you remove render_view_id and call
> RenderFrameHost::FromID(render_process_id,
render_frame_id)->GetRenderViewHost()
> everywhere that we temporarily need the render view id for now?

I can't do the conversion in this CL.  It's not as simple as calling
RenderFrameHost::FromID() because the render_view_id is used in a lot of the 
"control" methods that run on the IO thread.  There's a lot of work, and it
could easily be its own 200+ LOC change.

I think this is safe right now, since the Speech Recognition implementation is
mostly unchanged (i.e., RenderView based).  The only reason I added
render_frame_id is so the code could make use of
MediaStreamManager::MakeMediaAccessRequest().  Actually, the Speech Recognition
impl isn't really using MSManager like all the other media code.  It's just
trying to utilize the media permissions UI to request access from the user, then
doing it's own thing separately.  There's a TODO comment in
MediaStreamManager::MakeMediaAccessRequest that hints at a design problem with
this.

[jam, 2014-07-16 18:28:39]

slgtm

[Sergey Ulanov, 2014-07-16 19:29:47]

lgtm

https://codereview.chromium.org/364123002/diff/300001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.cc (right):

https://codereview.chromium.org/364123002/diff/300001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.cc:44: } while
(approved_streams_.find(id) != approved_streams_.end());
nit: This isn't really necessary. There are 128 bits of randomness - so
probability of collision is negligible, even if you generate 1M streams here.

[miu, 2014-07-16 20:43:29]

https://codereview.chromium.org/364123002/diff/300001/chrome/browser/media/de...
File chrome/browser/media/desktop_streams_registry.cc (right):

https://codereview.chromium.org/364123002/diff/300001/chrome/browser/media/de...
chrome/browser/media/desktop_streams_registry.cc:44: } while
(approved_streams_.find(id) != approved_streams_.end());
On 2014/07/16 19:29:47, Sergey Ulanov wrote:
> nit: This isn't really necessary. There are 128 bits of randomness - so
> probability of collision is negligible, even if you generate 1M streams here.

Okay, reverted.  I did leave in a DCHECK as a sanity-check to confirm the
GenerateRandomStreamId() function isn't being non-random in debug builds.

[miu, 2014-07-16 20:44:50]

TBR'ing kenrb@ since the changes to content/common/media/media_stream_messages.h
are only comment changes, and multiple other reviewers have looked at the file.

[miu, 2014-07-16 20:44:59]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:48:14]

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

[commit-bot: I haz the power, 2014-07-17 01:50:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-17 08:04:44]

Change committed as 283702

[kenrb, 2014-07-17 20:42:09]

ipc lgtm