Whitelist Google Hangouts origins to access Screen Capture API.

Whitelist Google Hangouts origins to access Screen Capture API.

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

[Sergey Ulanov, 2013-03-13 01:45:08]

juberti@ - Please check that I whitelisted correct set of origins.
jschuh@, pkasting@ - Please review.

[Peter Kasting, 2013-03-13 03:43:49]

Why can't testing be done using the flag?  So hangouts will not be WebRTC for
most people in M27.  I fail to see why that is the end of the world.

I wouldn't normally object, but when the API still needs to be "tested and
debugged" and still has "security issues", I'm not terribly comfortable sending
all Chrome users to it, even if we're whitelisting a friendly set of origins. 
There is no need to rush things.

[juberti, 2013-03-13 03:59:54]

On 2013/03/13 03:43:49, Peter Kasting wrote:
> Why can't testing be done using the flag?  So hangouts will not be WebRTC for
> most people in M27.  I fail to see why that is the end of the world.
> 
> I wouldn't normally object, but when the API still needs to be "tested and
> debugged" and still has "security issues", I'm not terribly comfortable
sending
> all Chrome users to it, even if we're whitelisting a friendly set of origins. 
> There is no need to rush things.

I understand your point, but we are opening a pretty small pinhole here, and
this allows us to do a cross-Google dogfood (e.g. including folks who wouldn't
know how to enable a flag) using Hangouts on WebRTC in the M27 timeframe. Which
allows us to kill off NPAPI even sooner.

Maybe I missed something in the CL, but I don't see the dire "security issues"
you mentioned in your comment.

[Sergey Ulanov, 2013-03-13 19:00:18]

On 2013/03/13 03:43:49, Peter Kasting wrote:
> Why can't testing be done using the flag?  So hangouts will not be WebRTC for
> most people in M27.  I fail to see why that is the end of the world.

I completely agree that it's best if we could avoid hacks like this, and I'm not
very comfortable about making this change. But in this specific case I think
it's the right thing to do.
See Justin's reply - it allows to drop NPAPI plugin sooner.
Also note that the flag that we currently have enables the API for all origins -
we don't want to have many users to use Chrome with that flag enabled all the
time. We could have a separate flag just to enable it for hangouts, but that is
the worst of the both worlds.


> I wouldn't normally object, but when the API still needs to be 
> "tested and debugged" 

That's bad wording on my part. The implementation is rather stable and based on
the code that has been used for long time in Chromoting. Main problem is that we
don't know how well it will perform with WebRTC stack - we need to make sure
that it's not downgrade when compared to plugin-based Hangouts. This requires
that it's used in real world on actual networks, so we can get feedback from the
users and properly prioritize possible performance issues (and this applies to
Hangouts as a whole, not only screencast feature).


> and still has "security issues",

The security issues are all around the question of how to open this API to
third-party apps. Security team did agree that whitelisting Hangouts domains is
an acceptable solution for now.


> I'm not terribly comfortable sending
> all Chrome users to it, even if we're whitelisting a friendly set of origins. 

I don't see how it can be bad for the users. Hangouts team obviously will not
start using this API unless they are sure it works properly. 


> There is no need to rush things.

[Peter Kasting, 2013-03-14 04:42:33]

OK.  Thanks for the explanation.  LGTM, though you might want to consider
clarifying some of those code comments similarly to how they were just clarified
in this thread.

[jschuh, 2013-03-15 14:24:49]

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
File chrome/browser/ui/screen_capture_infobar_delegate.cc (right):

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
chrome/browser/ui/screen_capture_infobar_delegate.cc:20: return origin.spec() ==
"https://staging.talkgadget.google.com/" ||
Would it be reasonable to put this in an OFFICIAL_BUILD #ifdef block?

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
chrome/browser/ui/screen_capture_infobar_delegate.cc:32:
switches::kEnableUserMediaScreenCapturing) ||
I thought we were going to require both the switch and the whitelisted origin
for the 27 timeframe, rather than one or the other?

[juberti, 2013-03-15 16:23:51]

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
> chrome/browser/ui/screen_capture_infobar_delegate.cc:32:
> switches::kEnableUserMediaScreenCapturing) ||
> I thought we were going to require both the switch and the whitelisted origin
> for the 27 timeframe, rather than one or the other?

Not sure I understand. That would seem to be the worst of both worlds - Goog
users need to enable a flag, external sites can't use this API at all.

[jschuh, 2013-03-15 18:18:56]

On 2013/03/15 16:23:51, juberti wrote:
>
https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
> > chrome/browser/ui/screen_capture_infobar_delegate.cc:32:
> > switches::kEnableUserMediaScreenCapturing) ||
> > I thought we were going to require both the switch and the whitelisted
origin
> > for the 27 timeframe, rather than one or the other?
> 
> Not sure I understand. That would seem to be the worst of both worlds - Goog
> users need to enable a flag, external sites can't use this API at all.

There are no plans to expose this to the open Web directly (i.e. outside of apps
and extensions). So, I suppose it's okay to keep the switch, but in that case do
we even need whitelisted URLs for dogfooding purposes?

[Wez, 2013-03-15 18:31:18]

jschuh@: The purpose of whitelisting these apps is specifically so they can
use the API without the command-line flag, so we can dogfood using it and
iron out any kinks before removing the flag.

We could have the API permission gated now, but only let these apps be
granted the permission, and have the command-line flag open the permission
up to be available to all apps/extensions.  WDYT?


On 15 March 2013 11:18, <jschuh@chromium.org> wrote:

> On 2013/03/15 16:23:51, juberti wrote:
>
> https://codereview.chromium.**org/12596011/diff/1/chrome/**
>
browser/ui/screen_capture_**infobar_delegate.cc#newcode32<https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capture_infobar_delegate.cc#newcode32>
>
>> > chrome/browser/ui/screen_**capture_infobar_delegate.cc:**32:
>> > switches::**kEnableUserMediaScreenCapturin**g) ||
>> > I thought we were going to require both the switch and the whitelisted
>>
> origin
>
>> > for the 27 timeframe, rather than one or the other?
>>
>
>  Not sure I understand. That would seem to be the worst of both worlds -
>> Goog
>> users need to enable a flag, external sites can't use this API at all.
>>
>
> There are no plans to expose this to the open Web directly (i.e. outside
> of apps
> and extensions). So, I suppose it's okay to keep the switch, but in that
> case do
> we even need whitelisted URLs for dogfooding purposes?
>
>
https://codereview.chromium.**org/12596011/<https://codereview.chromium.org/1...
>

[jschuh, 2013-03-15 19:16:44]

I guess there was a minor disconnect. I worry about this hanging around, but
lgtm on the strict condition that this gets removed soon, and nothing depending
on it can gets released on the web at large.

[Sergey Ulanov, 2013-03-15 22:30:57]

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
File chrome/browser/ui/screen_capture_infobar_delegate.cc (right):

https://codereview.chromium.org/12596011/diff/1/chrome/browser/ui/screen_capt...
chrome/browser/ui/screen_capture_infobar_delegate.cc:20: return origin.spec() ==
"https://staging.talkgadget.google.com/" ||
On 2013/03/15 14:24:50, Justin Schuh wrote:
> Would it be reasonable to put this in an OFFICIAL_BUILD #ifdef block?

Done.

[commit-bot: I haz the power, 2013-03-15 22:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12596011/14003

[commit-bot: I haz the power, 2013-03-15 22:35:50]

Presubmit check for 12596011-14003 failed and returned exit status 1.

INFO:root:Found 2 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/browser/renderer_host/media/video_capture_manager.cc

[vrk (LEFT CHROMIUM), 2013-03-15 22:41:15]

rubberstamp lgtm

[commit-bot: I haz the power, 2013-03-15 22:43:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/12596011/14003

[commit-bot: I haz the power, 2013-03-16 09:56:50]

Change committed as 188575