Wire up GetUserMedia in Chrome Frame.

Wire up GetUserMedia in Chrome Frame.

ExternalTabContainerWin wasn't forwarding requests for media access permission over to Browser's implementation

BUG=134624
TEST=visit http://www.corp.google.com/~xians/webrtc_getusermedia.html, click the "GetUserMedia" button, and notice the infobar.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145893

[grt (UTC plus 2), 2012-07-06 19:43:46]

Tommi: overall review please.
Scott: OWNERS in chrome/browser/ui.  FYI: I followed the existing pattern for
other WebContentsDelegate methods in ExternalTabContainerWin and Browser.

[tommi (sloooow) - chröme, 2012-07-06 22:17:09]

Great catch and thanks for fixing.  I suspect platforms other than windows might
have problems though?  If we continue to use ScopedCOMInitializer albeit in a
somewhat awkward way, we don't need to add the #include and avoid #ifdefs.

http://codereview.chromium.org/10692113/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10692113/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:964: DCHECK(tab);
nit: dcheck not needed since tab is dereferenced anyway just below

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:7: #include
<objbase.h>
won't this cause problems on non windows platforms?

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:67: HRESULT hr =
::CoInitializeEx(NULL, COINIT_MULTITHREADED);
I thought that this file was a cross platform file (no _win suffix).

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.h:56: bool
com_initialized_;
yikes, I see what you mean... should have been caught in a review.
hmm... /me wonders... to kinda force ourselves to use ScopedCOMInitializer as we
do in most other places, should we use scoped_ptr<ScopedCOMInitializer>?  I
realize it's a scoped scopy thing, but it gets rid of the #ifdefs.

[grt (UTC plus 2), 2012-07-06 23:07:24]

you rock.  thanks for the comments.  ptal.

http://codereview.chromium.org/10692113/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10692113/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:964: DCHECK(tab);
On 2012/07/06 22:17:09, tommi wrote:
> nit: dcheck not needed since tab is dereferenced anyway just below

Done.

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.cc:7: #include
<objbase.h>
On 2012/07/06 22:17:09, tommi wrote:
> won't this cause problems on non windows platforms?

what non-windows platforms?  ;-)

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
File content/browser/renderer_host/media/media_stream_manager.h (right):

http://codereview.chromium.org/10692113/diff/1/content/browser/renderer_host/...
content/browser/renderer_host/media/media_stream_manager.h:56: bool
com_initialized_;
On 2012/07/06 22:17:09, tommi wrote:
> yikes, I see what you mean... should have been caught in a review.
> hmm... /me wonders... to kinda force ourselves to use ScopedCOMInitializer as
we
> do in most other places, should we use scoped_ptr<ScopedCOMInitializer>?  I
> realize it's a scoped scopy thing, but it gets rid of the #ifdefs.

awesome idea.  done.

[tommi (sloooow) - chröme, 2012-07-07 07:24:32]

lgtm

[commit-bot: I haz the power, 2012-07-07 07:24:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10692113/10001

[commit-bot: I haz the power, 2012-07-07 07:24:41]

Presubmit check for 10692113-10001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

[tommi (sloooow) - chröme, 2012-07-07 07:25:04]

On 2012/07/07 07:24:41, I haz the power (commit-bot) wrote:
> Presubmit check for 10692113-10001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui

oops, my bad

[grt (UTC plus 2), 2012-07-09 13:36:23]

replacing @sky with @pkasting since the former seems to be away.  Peter: do you
have time for an OWNERS review of these chrome/browser/ui changes?

[tommi (sloooow) - chröme, 2012-07-09 14:39:04]

Hey Greg,

The DeviceThread fix is necessary for not just Chrome Frame but Chrome
proper.
This is a problem on Canary and at least a couple of P1's have been filed
- 136341 and 136238.

So, this is a bit more urgent than I had thought.  Can you add the above
bugs to the description land this as is and TBR for the owner stamp?


On Mon, Jul 9, 2012 at 3:36 PM, <grt@chromium.org> wrote:

> replacing @sky with @pkasting since the former seems to be away.  Peter:
> do you
> have time for an OWNERS review of these chrome/browser/ui changes?
>
>
http://codereview.chromium.**org/10692113/<http://codereview.chromium.org/106...
>

[grt (UTC plus 2), 2012-07-09 15:31:27]

On 2012/07/09 14:39:04, tommi wrote:
> Hey Greg,
> 
> The DeviceThread fix is necessary for not just Chrome Frame but Chrome
> proper.
> This is a problem on Canary and at least a couple of P1's have been filed
> - 136341 and 136238.
> 
> So, this is a bit more urgent than I had thought.  Can you add the above
> bugs to the description land this as is and TBR for the owner stamp?


I moved the COM bits into issue 10744002, which is in the CQ now.  I'm okay
waiting for ui OWNERS approval on the GCF parts.

[Ben Goodger (Google), 2012-07-10 15:35:03]

lgtm

[commit-bot: I haz the power, 2012-07-10 15:35:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/10692113/16001

[commit-bot: I haz the power, 2012-07-10 16:53:42]

Change committed as 145893