Chromecast: pages default to having a black background.

Chromecast: pages default to having a black background.

R=byungchul@chromium.org,lcwu@chromium.org,abarth@chromium.org
BUG=400916
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289907

[byungchul, 2014-08-12 20:12:12]

lgtm

[lcwu1, 2014-08-14 13:59:43]

lgtm

[lcwu1, 2014-08-14 14:01:53]

On 2014/08/14 13:59:43, lcwu1 wrote:
> lgtm

Note that you will also need to get owner's approval from third_party/WebKit
given that it is added in the DEPS file in this CL.

[gunsch, 2014-08-14 16:04:29]

On 2014/08/14 14:01:53, lcwu1 wrote:
> On 2014/08/14 13:59:43, lcwu1 wrote:
> > lgtm
> 
> Note that you will also need to get owner's approval from third_party/WebKit
> given that it is added in the DEPS file in this CL.

+abarth, can you take a quick look at our usage of third_party/WebKit?

[gunsch, 2014-08-14 16:27:50]

On 2014/08/14 16:04:29, gunsch wrote:
> On 2014/08/14 14:01:53, lcwu1 wrote:
> > On 2014/08/14 13:59:43, lcwu1 wrote:
> > > lgtm
> > 
> > Note that you will also need to get owner's approval from third_party/WebKit
> > given that it is added in the DEPS file in this CL.
> 
> +abarth, can you take a quick look at our usage of third_party/WebKit?

[actually adding abarth]

[abarth-chromium, 2014-08-14 20:34:54]

I'm not a good reviewer for this change.  You want someone like jamesr.

[jamesr, 2014-08-14 20:37:13]

What do files in the chromecast/ target end up linking in to?

[gunsch, 2014-08-14 20:47:34]

On 2014/08/14 20:37:13, jamesr wrote:
> What do files in the chromecast/ target end up linking in to?

chromecast/ has its own "cast_shell" target, see crbug/336640. It is treated as
a "top-level" directory, like chrome/---it does not depend on chrome/, and
chrome/ does not depend on it. No nothing from chromecast/ is linked into the
"chrome" binary, only into "cast_shell" and Chromecast test targets.

[jamesr, 2014-08-15 00:52:54]

https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS
File chromecast/DEPS (right):

https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS#newcode9
chromecast/DEPS:9: # Allow inclusion of WebKit API files.
this should have the same restrictions as whatever allows
content/public/renderer/

[gunsch, 2014-08-15 00:58:07]

https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS
File chromecast/DEPS (right):

https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS#newcode9
chromecast/DEPS:9: # Allow inclusion of WebKit API files.
On 2014/08/15 00:52:54, jamesr wrote:
> this should have the same restrictions as whatever allows
> content/public/renderer/

Hm, should it? I see the same include_rules in chrome/ and content/.

Presubmit doesn't seem to pass unless I have both rules. I guess I'm not sure
what you're suggesting?

[jamesr, 2014-08-15 01:01:17]

On 2014/08/15 00:58:07, gunsch wrote:
> https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS
> File chromecast/DEPS (right):
> 
> https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS#newcode9
> chromecast/DEPS:9: # Allow inclusion of WebKit API files.
> On 2014/08/15 00:52:54, jamesr wrote:
> > this should have the same restrictions as whatever allows
> > content/public/renderer/
> 
> Hm, should it? I see the same include_rules in chrome/ and content/.
> 
> Presubmit doesn't seem to pass unless I have both rules. I guess I'm not sure
> what you're suggesting?

You're only using the blink apis from places that depend on
content/public/renderer/, so you should put the DEPS rules in the same place
that allows content/public/renderer/.  It appears not all of chromecast/ is
allowed to depend on the content renderer API, so you shouldn't allow all of
chromecast/ to depend on blink either

[gunsch, 2014-08-15 04:10:59]

On 2014/08/15 01:01:17, jamesr wrote:
> On 2014/08/15 00:58:07, gunsch wrote:
> > https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS
> > File chromecast/DEPS (right):
> > 
> > https://codereview.chromium.org/454113003/diff/1/chromecast/DEPS#newcode9
> > chromecast/DEPS:9: # Allow inclusion of WebKit API files.
> > On 2014/08/15 00:52:54, jamesr wrote:
> > > this should have the same restrictions as whatever allows
> > > content/public/renderer/
> > 
> > Hm, should it? I see the same include_rules in chrome/ and content/.
> > 
> > Presubmit doesn't seem to pass unless I have both rules. I guess I'm not
sure
> > what you're suggesting?
> 
> You're only using the blink apis from places that depend on
> content/public/renderer/, so you should put the DEPS rules in the same place
> that allows content/public/renderer/.  It appears not all of chromecast/ is
> allowed to depend on the content renderer API, so you shouldn't allow all of
> chromecast/ to depend on blink either

Ah, hadn't noticed that we had smaller-scoped DEPS files in chromecast/; thanks
for clarifying.

Reduced the DEPS scope, PTAL.

[jamesr, 2014-08-15 04:23:24]

lgtm

[gunsch, 2014-08-15 05:03:25]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-08-15 05:06:18]

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

[commit-bot: I haz the power, 2014-08-15 16:30:12]

Committed patchset #2 (20001) as 289907