Initial checkin of chromecast content embedder (cast_shell) and related build scripts.

Initial checkin of chromecast content embedder (cast_shell) and related build scripts.

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

[jam, 2014-04-04 01:01:23]

I haven't looked through this all this change yet, but here are some initial
comments.

My main two comments are:
-make the steps for a random chromium developer to fetch your code and build it
to be as close to other chromium ports as possible. the gold standard is
linux_chromeos where a user just sets "chromeos=1" in their gyp variables and
runs a script once to download any dependencies.

-cc files that implement methods in headers in other directories need to live in
those other directories (unless it's an interface). this is the pattern we use
for every other port.


This will have to be broken up and landed in pieces as currently it's too big to
code review all together.

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README
File chromecast/build/README (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:21: 2. Pick the compielr and build flavor
nit: spelling

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:29: $ setup_chromium_env_x86_chromecast `pwd -P`
<Debug|Release>
hmm, so in general, most devs are used to putting gyp variables in
chromium.gyp_env so that they persist. why is there a wrapper here, instead of
having a line showing which variable to put in that file? and extra points for
having one variable which the build system keys off, i.e. chromecast=1. this is
how the linux_chromeos config works

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:33: $ update_gyp
Why isn't this just build/gyp_chromium? I see the method also fetches gn, but I
thought gclient does that automatically when syncing?

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:35: 4. Buidl
nit: spelling

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:37: $ mymake cast_shell
like above, why is there a wrapper around calling ninja?

it's good to have the developer process be the same across all of our configs.
since the intent is to make this a supported config on the trybots/CQ/buildbot,
we should make it as close to our existing configs as possible. that reduces the
friction when a win/mac/linux developer fetches this code to fix a bug in a
broken test etc

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
File chromecast/build/build_env_arm_chromecast.sh (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
chromecast/build/build_env_arm_chromecast.sh:54: arm_arch=''
i wonder if this stuff better belongs in build/common.gypi, so that when i set
"chromecast=1" in my chromium.gyp_env, all this is setup for free.

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:34: 'target_name': 'cast_gfx',
why bringing in cc files manually instead of depending on the gfx gypi instead?

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:104: 'port/webkit/common/cursors/webcursor_cast.cc',
for all the cc files other than ones that match headers in content/port, i would
think that these files belong beside the other platform implementations. i.e.
the are not "public apis" that these implement. otherwise it'll be confusing to
developers when they change a file in ui/ or webkit/, and then they have to
update a file in another top level directory that they may not even be owners in

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:105:
'../printing/printing_context_no_system_dialog.cc',
again, why are you not including a target from printing/skia?

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
File chromecast/common/cast_paths.cc (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:1: // Copyright (c) 2014 Google Inc. All Rights
Reserved.
btw in this and other files, no "(c)"

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:33: // eliminate this object file if there is no
direct entry point into it.
no need to copy this comment

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/DEPS
File chromecast/port/DEPS (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/DEPS#new...
chromecast/port/DEPS:8: "+content/port",
only content/public and content/port can be included by embedders of content.
the rest are implementation details and as such they are not allowed in any DEPS
files outside of content

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
File chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc
(right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
why does this file exist? are you using plugins? if not, how about setting
enable_plugins=0?

[gunsch-google, 2014-04-04 20:02:33]

Do you want to also immediately populate an OWNERS file?

[gunsch-google, 2014-04-04 20:12:50]

On 2014/04/04 20:02:33, gunsch-google wrote:
> Do you want to also immediately populate an OWNERS file?

Oops, didn't realize we already had an OWNERS file before.

I'm fine with this CL but obviously have looked at this code in pieces outside
of this checkin.

I like John's suggestions to try to simplify our build setup, though obviously
we're still looking to target multiple platforms. How do CrOS/Android handle
that, is it just via setting target_arch locally?

[lcwu1, 2014-04-05 01:17:12]

John, my workstation died this morning so I can only answer some of your
questions. I will address all of your comments and upload new CL(s) next Monday
when I get my new machine.

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README
File chromecast/build/README (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:29: $ setup_chromium_env_x86_chromecast `pwd -P`
<Debug|Release>
On 2014/04/04 01:01:24, jam wrote:
> hmm, so in general, most devs are used to putting gyp variables in
> chromium.gyp_env so that they persist. why is there a wrapper here, instead of
> having a line showing which variable to put in that file? and extra points for
> having one variable which the build system keys off, i.e. chromecast=1. this
is
> how the linux_chromeos config works

Having a wrapper to populate GYP_DEFINES is easier for our internal build
servers to build the whole device firmware (which Chromium is part of), but we
will look into chromium.gyp_env. (We might still need to keep the wrappers but,
if that's the case, will update the document on how to use chromium.gyp_env for
building chromecast.)

We would certainly love to only set one gyp variable and then, in common.gypi,
set all the other gyp variables needed for building chromecast, if that's
preferred.

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:33: $ update_gyp
On 2014/04/04 01:01:24, jam wrote:
> Why isn't this just build/gyp_chromium? I see the method also fetches gn, but
I
> thought gclient does that automatically when syncing?

Yes, gclient would pull in gn. However, in our current source management and
build system, we don't use gclient. (We use repo as we have other components
other than Chromium.) So we would still need the wrapper for our internal build
system. But for users who use gclient to pull in and sync their source tree,
they could certainly use gyp_chromium. I will update the document.

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/README#...
chromecast/build/README:37: $ mymake cast_shell
On 2014/04/04 01:01:24, jam wrote:
> like above, why is there a wrapper around calling ninja?
> 
> it's good to have the developer process be the same across all of our configs.
> since the intent is to make this a supported config on the
trybots/CQ/buildbot,
> we should make it as close to our existing configs as possible. that reduces
the
> friction when a win/mac/linux developer fetches this code to fix a bug in a
> broken test etc

Yes, good point. Will do.

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
File chromecast/build/build_env_arm_chromecast.sh (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
chromecast/build/build_env_arm_chromecast.sh:54: arm_arch=''
On 2014/04/04 01:01:24, jam wrote:
> i wonder if this stuff better belongs in build/common.gypi, so that when i set
> "chromecast=1" in my chromium.gyp_env, all this is setup for free.

Yes, I too prefer what you suggested. (In fact, in our local fork, most of these
gyp variables are set in common.gypi. It's our cleanup for upstreaming that
brought these gyp variables setting here. :-)

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:104: 'port/webkit/common/cursors/webcursor_cast.cc',
On 2014/04/04 01:01:24, jam wrote:
> for all the cc files other than ones that match headers in content/port, i
would
> think that these files belong beside the other platform implementations. i.e.
> the are not "public apis" that these implement. otherwise it'll be confusing
to
> developers when they change a file in ui/ or webkit/, and then they have to
> update a file in another top level directory that they may not even be owners
in

Yes, agreed. All these non-public platform-specific implementation files used to
be in their respective directories with other platform in our fork. And again,
it's our cleanup work that brought them here. I will place them back in their
corresponding directories.

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:105:
'../printing/printing_context_no_system_dialog.cc',
On 2014/04/04 01:01:24, jam wrote:
> again, why are you not including a target from printing/skia?

Chromecast doesn't support printing, but the build would fail if we don't
include a stub implementation like this file. Instead of including this file
directly, I can modify printing.gyp to include this file when chromecast_build
is 1.

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/DEPS
File chromecast/port/DEPS (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/DEPS#new...
chromecast/port/DEPS:8: "+content/port",
On 2014/04/04 01:01:24, jam wrote:
> only content/public and content/port can be included by embedders of content.
> the rest are implementation details and as such they are not allowed in any
DEPS
> files outside of content

Will fix.

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
File chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc
(right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/04/04 01:01:24, jam wrote:
> why does this file exist? are you using plugins? if not, how about setting
> enable_plugins=0?

No, we don't use plugin. I think we encounter some other errors when setting
enable_plugins to 0. Will investigate further.

[jam, 2014-04-07 17:32:00]

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
File chromecast/build/build_env_arm_chromecast.sh (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/build/build_e...
chromecast/build/build_env_arm_chromecast.sh:54: arm_arch=''
On 2014/04/05 01:17:13, lcwu1 wrote:
> On 2014/04/04 01:01:24, jam wrote:
> > i wonder if this stuff better belongs in build/common.gypi, so that when i
set
> > "chromecast=1" in my chromium.gyp_env, all this is setup for free.
> 
> Yes, I too prefer what you suggested. (In fact, in our local fork, most of
these
> gyp variables are set in common.gypi. It's our cleanup for upstreaming that
> brought these gyp variables setting here. :-)
> 

I see. ok btw this is what android does (they set a bunch of flags based on
os==android):
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:105:
'../printing/printing_context_no_system_dialog.cc',
On 2014/04/05 01:17:13, lcwu1 wrote:
> On 2014/04/04 01:01:24, jam wrote:
> > again, why are you not including a target from printing/skia?
> 
> Chromecast doesn't support printing, but the build would fail if we don't
> include a stub implementation like this file. Instead of including this file
> directly, I can modify printing.gyp to include this file when chromecast_build
> is 1.

so the best way to fix this printing case would be to set enable_printing=0 in
common.gypi for chromecast, since you don't want any of printing

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
File chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc
(right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/port/content/...
chromecast/port/content/child/npapi/webplugin_delegate_impl_cast.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/04/05 01:17:13, lcwu1 wrote:
> On 2014/04/04 01:01:24, jam wrote:
> > why does this file exist? are you using plugins? if not, how about setting
> > enable_plugins=0?
> 
> No, we don't use plugin. I think we encounter some other errors when setting
> enable_plugins to 0. Will investigate further.

I see, ok please fix the enable_plugins=0 case then. happy to review brokeness
there

[lcwu1, 2014-04-22 00:59:39]

Path set #3 addresses all the review comments from the previous patch sets. Most
noticeably, we removed the custom build scripts and moved the cast-specific port
implementation to their respective locations. Please take another look. Thanks.

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
File chromecast/common/cast_paths.cc (right):

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:1: // Copyright (c) 2014 Google Inc. All Rights
Reserved.
On 2014/04/04 01:01:24, jam wrote:
> btw in this and other files, no "(c)"

Done.

https://codereview.chromium.org/223143003/diff/20001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:33: // eliminate this object file if there is no
direct entry point into it.
On 2014/04/04 01:01:24, jam wrote:
> no need to copy this comment

Done.

[lcwu1, 2014-06-24 22:28:52]

We have modified the cast shell to enable/use aura and ozone (with their latest
egl x11 backend on x86) and removed all of our custom graphics stack (especially
RWHV_cast) and stub implementation related to event handling. Please take
another look. Thanks.

[lcwu1, 2014-06-27 17:21:39]

On 2014/06/24 22:28:52, lcwu1 wrote:
> We have modified the cast shell to enable/use aura and ozone (with their
latest
> egl x11 backend on x86) and removed all of our custom graphics stack
(especially
> RWHV_cast) and stub implementation related to event handling. Please take
> another look. Thanks.

Ping?

[rjkroege, 2014-06-27 17:31:16]

On 2014/06/27 17:21:39, lcwu1 wrote:
> On 2014/06/24 22:28:52, lcwu1 wrote:
> > We have modified the cast shell to enable/use aura and ozone (with their
> latest
> > egl x11 backend on x86) and removed all of our custom graphics stack
> (especially
> > RWHV_cast) and stub implementation related to event handling. Please take
> > another look. Thanks.
> 
> Ping?


driveby comment: it would be nice if the ui/gfx/screen* stuff was either be
unnecessary or folded into the ongoing refactoring of display and screen that
dnicoara@ has been doing for ozone.

[dcheng, 2014-06-27 17:41:40]

Just a drive by.

https://codereview.chromium.org/223143003/diff/80001/chromecast/shell/browser...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/223143003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/cast_content_browser_client.cc:52: //
URLRequestContextFactory::CreateMainGetter.
Is there a better way to do this? This comment is already stale, I can't seem to
find a reference to CreateMainGetter.

[dnicoara, 2014-06-27 19:43:35]

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:151: '../ui/ozone/ozone.gyp:eglplatform_shim_x11',
drive-by comment: You shouldn't need to depend on a specific Ozone platform.
Normally you'd depend on ui/ozone/ozone.gyp:ozone and either set the default
platform at compile time or specify it as a command line parameter. Is there a
specific reason for depending on the platform directly?

[lcwu1, 2014-06-27 23:01:20]

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:151: '../ui/ozone/ozone.gyp:eglplatform_shim_x11',
On 2014/06/27 19:43:35, dnicoara wrote:
> drive-by comment: You shouldn't need to depend on a specific Ozone platform.
> Normally you'd depend on ui/ozone/ozone.gyp:ozone and either set the default
> platform at compile time or specify it as a command line parameter. Is there a
> specific reason for depending on the platform directly?

We have already enabled ozone_platform_ozonex in common.gypi. What we also like
to do is for libeglplatform_shim.so to be built automatically when we build
cast_shell. However, without explicitly specifying the target
"eglplatform_shim_x11" as a dependency, it won't get built when we do "ninja
cast_shell."

If there is another way for libeglplatform_shim.so to be built as part of
building cast_shell, I would love to know. Thanks.

[lcwu1, 2014-06-27 23:26:35]

On 2014/06/27 17:31:16, rjkroege wrote:
> On 2014/06/27 17:21:39, lcwu1 wrote:
> > On 2014/06/24 22:28:52, lcwu1 wrote:
> > > We have modified the cast shell to enable/use aura and ozone (with their
> > latest
> > > egl x11 backend on x86) and removed all of our custom graphics stack
> > (especially
> > > RWHV_cast) and stub implementation related to event handling. Please take
> > > another look. Thanks.
> > 
> > Ping?
> 
> 
> driveby comment: it would be nice if the ui/gfx/screen* stuff was either be
> unnecessary or folded into the ongoing refactoring of display and screen that
> dnicoara@ has been doing for ozone.

ScreenCast is a minimal implementation of gfx::Screen that's not much different
from aura::TestScreen or ScreenMojo. It was created because I couldn't find a
corresponding implementation in Ozone. If Daniel has been working on this, I
could change the use of ScreenCast to aura::TestScreen for now and then change
it to use Ozone's when it is available.

[lcwu1, 2014-06-28 00:58:17]

https://codereview.chromium.org/223143003/diff/80001/chromecast/shell/browser...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/223143003/diff/80001/chromecast/shell/browser...
chromecast/shell/browser/cast_content_browser_client.cc:52: //
URLRequestContextFactory::CreateMainGetter.
On 2014/06/27 17:41:39, dcheng (OOO) wrote:
> Is there a better way to do this? This comment is already stale, I can't seem
to
> find a reference to CreateMainGetter.

The comment was actually copied from content shell's
ShellContentBrowserClient::IsHandledURL implementation. :-) And it actually
doesn't make sense for us because we only want to allow the schemes listed
below. I will remove the comment.

CreateMainGetter is our own helper function defined in
chromecast/shell/browser/url_request_context_factory.cc. It is called inside
CastContentBrowserClient::CreateRequestContext().

[lcwu1, 2014-06-28 01:24:46]

Patch set #6 removes the ScreenCast implementation (and uses aura's TestScreen
instead for now) as well as the comment that no longer makes sense.

PTAL.

[lcwu1, 2014-07-01 17:34:42]

On 2014/06/28 01:24:46, lcwu1 wrote:
> Patch set #6 removes the ScreenCast implementation (and uses aura's TestScreen
> instead for now) as well as the comment that no longer makes sense.
> 
> PTAL.

Ping? Thanks.

[Ryan Sleevi, 2014-07-01 21:02:58]

On 2014/07/01 17:34:42, lcwu1 wrote:
> On 2014/06/28 01:24:46, lcwu1 wrote:
> > Patch set #6 removes the ScreenCast implementation (and uses aura's
TestScreen
> > instead for now) as well as the comment that no longer makes sense.
> > 
> > PTAL.
> 
> Ping? Thanks.

Please indicate who you are pinging, and what you are pinging for?

This is a resurrection of a CL almost 3 months old, so it's hard to find what
you're asking, and of who.

[Ryan Sleevi, 2014-07-01 21:13:55]

https://codereview.chromium.org/223143003/diff/100001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:12: '..',  # Root of Chromium checkout
This is already handled by the basic build rules.

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
File chromecast/common/cast_paths.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
chromecast/common/cast_paths.cc:17: #if defined(__arm__)
Seems strange to use this directly, when we have build/build_config.h designed
for abstracting out the build config macros.

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
chromecast/common/cast_paths.cc:19: // $HOME for the chrome user.
This comment suggests a layering violation (comments should be like code, and
not refer to implementation details of 'higher' layers).

It also should be capitalized if you're talking about Chrome (which you
shouldn't be, outside of src/chrome. This is Chromium)

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_cast.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:8: #include "base/basictypes.h"
use "base/macros.h" for DISALLOW

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:16: // later CLs after we clean up
the code.
1) Avoid pronouns in comments (
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M
)

2) Reference a bug

// TODO(lcwu): http://crbug.com/foo - Provide an actual implementation; this
// is just a stub to aid upstreaming.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:22: // Implementation of
net::NetworkChangeNotifier.
You use inconsistent comment styles here with this and the factory

Compare with

"// net::NetworkChangeNotifierFactory overrides."

While several styles exist, traditionally it's simply:

// net::NetworkChangeNotifier implementation:

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_factory_cast.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_factory_cast.cc:31: return
g_network_change_notifier_cast;
Use a LazyInstance/LeakyLazyInstance here, not a global static.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_factory_cast.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_factory_cast.h:8: #include
"base/basictypes.h"
use "base/macros.h" instead for DISALLOW

[lcwu1, 2014-07-01 21:27:51]

On 2014/07/01 21:02:58, Ryan Sleevi wrote:
> On 2014/07/01 17:34:42, lcwu1 wrote:
> > On 2014/06/28 01:24:46, lcwu1 wrote:
> > > Patch set #6 removes the ScreenCast implementation (and uses aura's
> TestScreen
> > > instead for now) as well as the comment that no longer makes sense.
> > > 
> > > PTAL.
> > 
> > Ping? Thanks.
> 
> Please indicate who you are pinging, and what you are pinging for?
> 
> This is a resurrection of a CL almost 3 months old, so it's hard to find what
> you're asking, and of who.

Sorry that I didn't make my ping clearer. This CL is the skeleton of our content
embedded implementation (i.e. cast shell). We want to make sure that the way we
structure/architect our content embedder is correct and what's expected. (After
this CL is submitted, we will have many subsequent CLs that are based on this
one.) Basically the main feedback from two months ago was that we should move to
use aura + ozone instead of our own RWHV implementation and graphics stack. So
the patch set #4 basically is a revised cast_shell implementation that's based
on aura/ozone. And then patch set #6 addresses the drive-by comments made
several days ago.

I think the ping is primarily for jam as he was the main reviewer from two
months ago, but also for people who provided feedback to see if I have addressed
their comments properly.

[jam, 2014-07-01 21:56:10]

sorry for the delay, I missed these pings. In the future, please IM me if I
don't respond in half a day or so.

Thanks so much for switching to Ozone, this is much much better. All the issued
I had are addressed. I only have a few tiny nits below.

I defer to others for the usage of Ozone.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.cc:13: #include
"content/public/browser/browser_main_runner.h"
doesn't look like this is used? if so, no need to add "content/public" to
chromecast/shell/app/DEPS

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
File chromecast/shell/app/cast_main_delegate.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.h:30: OVERRIDE;
nit: don't begin a line with OVERRIDE (see cr-dev thread about this, i think
we're waiting for a new clang to fix this)

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:14: }
all these forward declares aren't needed since they're part of the interface

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:16: enum
WindowContainerType;
remove this (can't forward declare an enum anyways)

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:32: 
nit: here and below, no blank lines between overridden methods

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/geolocation/cast_access_token_store.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/geolocation/cast_access_token_store.h:29: 
ditto (no blank line)

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/url_request_context_factory.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.h:24: }  // namespace net
don't need most (or all) of these

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
File chromecast/shell/renderer/cast_content_renderer_client.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
chromecast/shell/renderer/cast_content_renderer_client.h:21: OVERRIDE;
ditto

[dnicoara, 2014-07-02 14:23:52]

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/80001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:151: '../ui/ozone/ozone.gyp:eglplatform_shim_x11',
On 2014/06/27 23:01:20, lcwu1 wrote:
> On 2014/06/27 19:43:35, dnicoara wrote:
> > drive-by comment: You shouldn't need to depend on a specific Ozone platform.
> > Normally you'd depend on ui/ozone/ozone.gyp:ozone and either set the default
> > platform at compile time or specify it as a command line parameter. Is there
a
> > specific reason for depending on the platform directly?
> 
> We have already enabled ozone_platform_ozonex in common.gypi. What we also
like
> to do is for libeglplatform_shim.so to be built automatically when we build
> cast_shell. However, without explicitly specifying the target
> "eglplatform_shim_x11" as a dependency, it won't get built when we do "ninja
> cast_shell."
> 
> If there is another way for libeglplatform_shim.so to be built as part of
> building cast_shell, I would love to know. Thanks.
> 

I chatted with spang@ on this. There's some pending X11 on Ozone work that will
fix this issue. For now we can leave this as is.

[spang, 2014-07-02 15:46:15]

On 2014/07/01 21:56:10, jam wrote:
> sorry for the delay, I missed these pings. In the future, please IM me if I
> don't respond in half a day or so.
> 
> Thanks so much for switching to Ozone, this is much much better. All the
issued
> I had are addressed. I only have a few tiny nits below.
> 
> I defer to others for the usage of Ozone.

Looks like there's nothing to review here. Ozone is used indirectly via aura.

> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
> File chromecast/shell/app/cast_main_delegate.cc (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
> chromecast/shell/app/cast_main_delegate.cc:13: #include
> "content/public/browser/browser_main_runner.h"
> doesn't look like this is used? if so, no need to add "content/public" to
> chromecast/shell/app/DEPS
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
> File chromecast/shell/app/cast_main_delegate.h (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
> chromecast/shell/app/cast_main_delegate.h:30: OVERRIDE;
> nit: don't begin a line with OVERRIDE (see cr-dev thread about this, i think
> we're waiting for a new clang to fix this)
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> File chromecast/shell/browser/cast_content_browser_client.h (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> chromecast/shell/browser/cast_content_browser_client.h:14: }
> all these forward declares aren't needed since they're part of the interface
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> chromecast/shell/browser/cast_content_browser_client.h:16: enum
> WindowContainerType;
> remove this (can't forward declare an enum anyways)
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> chromecast/shell/browser/cast_content_browser_client.h:32: 
> nit: here and below, no blank lines between overridden methods
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> File chromecast/shell/browser/geolocation/cast_access_token_store.h (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> chromecast/shell/browser/geolocation/cast_access_token_store.h:29: 
> ditto (no blank line)
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> File chromecast/shell/browser/url_request_context_factory.h (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
> chromecast/shell/browser/url_request_context_factory.h:24: }  // namespace net
> don't need most (or all) of these
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
> File chromecast/shell/renderer/cast_content_renderer_client.h (right):
> 
>
https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
> chromecast/shell/renderer/cast_content_renderer_client.h:21: OVERRIDE;
> ditto

[spang, 2014-07-02 15:46:30]

https://codereview.chromium.org/223143003/diff/100001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:12: '..',  # Root of Chromium checkout
On 2014/07/01 21:13:54, Ryan Sleevi wrote:
> This is already handled by the basic build rules.

I think it usually comes from dependencies via direct_dependent_settings, not
the build system. Standalone targets require it. It's not a bad idea to be
explicit.

[lcwu1, 2014-07-02 23:30:33]

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
File chromecast/common/cast_paths.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
chromecast/common/cast_paths.cc:17: #if defined(__arm__)
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> Seems strange to use this directly, when we have build/build_config.h designed
> for abstracting out the build config macros.

Changed to use ARCH_CPU_ARMEL defined in build/build_config.h.

https://codereview.chromium.org/223143003/diff/100001/chromecast/common/cast_...
chromecast/common/cast_paths.cc:19: // $HOME for the chrome user.
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> This comment suggests a layering violation (comments should be like code, and
> not refer to implementation details of 'higher' layers).
> 
> It also should be capitalized if you're talking about Chrome (which you
> shouldn't be, outside of src/chrome. This is Chromium)

This comment is really confusing, isn't it? What we really meant here is that
cast_shell runs on the chromecast device as a 'chrome' user. (That is, we have a
user id called 'chrome'.) And $HOME for the 'chrome' user is set to our /data
directory on the device. I will re-word the comment.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_cast.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:8: #include "base/basictypes.h"
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> use "base/macros.h" for DISALLOW

Done.

Also added this include in the other headers that have DISALLOW.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:16: // later CLs after we clean up
the code.
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> 1) Avoid pronouns in comments (
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M
> )
> 
> 2) Reference a bug
> 
> // TODO(lcwu): http://crbug.com/foo - Provide an actual implementation; this
> // is just a stub to aid upstreaming.

Done.

Also fixed the same issue in several other places.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:22: // Implementation of
net::NetworkChangeNotifier.
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> You use inconsistent comment styles here with this and the factory
> 
> Compare with
> 
> "// net::NetworkChangeNotifierFactory overrides."
> 
> While several styles exist, traditionally it's simply:
> 
> // net::NetworkChangeNotifier implementation:

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_factory_cast.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_factory_cast.cc:31: return
g_network_change_notifier_cast;
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> Use a LazyInstance/LeakyLazyInstance here, not a global static.

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
File chromecast/net/network_change_notifier_factory_cast.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/net/network_...
chromecast/net/network_change_notifier_factory_cast.h:8: #include
"base/basictypes.h"
On 2014/07/01 21:13:55, Ryan Sleevi wrote:
> use "base/macros.h" instead for DISALLOW

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.cc:13: #include
"content/public/browser/browser_main_runner.h"
On 2014/07/01 21:56:09, jam wrote:
> doesn't look like this is used? if so, no need to add "content/public" to
> chromecast/shell/app/DEPS

Done. (also changed content/public to content/public/app in
chromecast/shell/app/DEPS.)

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
File chromecast/shell/app/cast_main_delegate.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.h:30: OVERRIDE;
On 2014/07/01 21:56:09, jam wrote:
> nit: don't begin a line with OVERRIDE (see cr-dev thread about this, i think
> we're waiting for a new clang to fix this)

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:14: }
On 2014/07/01 21:56:10, jam wrote:
> all these forward declares aren't needed since they're part of the interface

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:16: enum
WindowContainerType;
On 2014/07/01 21:56:09, jam wrote:
> remove this (can't forward declare an enum anyways)

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.h:32: 
On 2014/07/01 21:56:10, jam wrote:
> nit: here and below, no blank lines between overridden methods

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/geolocation/cast_access_token_store.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/geolocation/cast_access_token_store.h:29: 
On 2014/07/01 21:56:10, jam wrote:
> ditto (no blank line)

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
File chromecast/shell/browser/url_request_context_factory.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.h:24: }  // namespace net
On 2014/07/01 21:56:10, jam wrote:
> don't need most (or all) of these

Done.

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
File chromecast/shell/renderer/cast_content_renderer_client.h (right):

https://codereview.chromium.org/223143003/diff/100001/chromecast/shell/render...
chromecast/shell/renderer/cast_content_renderer_client.h:21: OVERRIDE;
On 2014/07/01 21:56:10, jam wrote:
> ditto

Done.

[lcwu1, 2014-07-02 23:40:40]

Patch set #7 should address all the review comments (so far). PTAL. Thanks.

[Ryan Sleevi, 2014-07-02 23:55:51]

Deferring to mmenke for the URLRequestContextGetter (as a more familiar OWNER),
although the cursory scan is that it LG

The net/ bits LGTM

https://codereview.chromium.org/223143003/diff/120001/chromecast/net/network_...
File chromecast/net/network_change_notifier_cast.h (right):

https://codereview.chromium.org/223143003/diff/120001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:12: namespace net {
These shouldn't be in the net namespace.

[jam, 2014-07-03 01:01:55]

lgtm

[lcwu1, 2014-07-03 01:48:59]

https://codereview.chromium.org/223143003/diff/120001/chromecast/net/network_...
File chromecast/net/network_change_notifier_cast.h (right):

https://codereview.chromium.org/223143003/diff/120001/chromecast/net/network_...
chromecast/net/network_change_notifier_cast.h:12: namespace net {
On 2014/07/02 23:55:51, Ryan Sleevi wrote:
> These shouldn't be in the net namespace.

Done.

[lcwu1, 2014-07-07 18:25:44]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-07-07 18:26:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001

[mmenke, 2014-07-07 18:34:37]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2014-07-07 18:35:09]

On 2014/07/07 18:26:32, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001

I'm going to reviewing context creation, as requested by rsleevi.

[lcwu1, 2014-07-07 18:35:31]

The CQ bit was unchecked by lcwu@chromium.org

[mmenke, 2014-07-07 18:35:48]

On 2014/07/07 18:35:09, mmenke wrote:
> On 2014/07/07 18:26:32, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001
> 
> I'm going to reviewing context creation, as requested by rsleevi.

*going to review, rather

[lcwu1, 2014-07-07 18:36:49]

On 2014/07/07 18:35:09, mmenke wrote:
> On 2014/07/07 18:26:32, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001
> 
> I'm going to reviewing context creation, as requested by rsleevi.

Sure. Unchecked the commit queue. Thanks.

[mmenke, 2014-07-07 18:43:49]

On 2014/07/07 18:36:49, lcwu1 wrote:
> On 2014/07/07 18:35:09, mmenke wrote:
> > On 2014/07/07 18:26:32, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001
> > 
> > I'm going to reviewing context creation, as requested by rsleevi.
> 
> Sure. Unchecked the commit queue. Thanks.

Great!  I'll try to get back to you today, but have a couple other reviews to
do, so may not be until tomorrow morning.

[lcwu1, 2014-07-07 18:52:00]

On 2014/07/07 18:43:49, mmenke wrote:
> On 2014/07/07 18:36:49, lcwu1 wrote:
> > On 2014/07/07 18:35:09, mmenke wrote:
> > > On 2014/07/07 18:26:32, I haz the power (commit-bot) wrote:
> > > > CQ is trying da patch. Follow status at
> > > > 
https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/160001
> > > 
> > > I'm going to reviewing context creation, as requested by rsleevi.
> > 
> > Sure. Unchecked the commit queue. Thanks.
> 
> Great!  I'll try to get back to you today, but have a couple other reviews to
> do, so may not be until tomorrow morning.

Sounds good. Thanks for taking the time to review the code.

[mmenke, 2014-07-08 15:43:59]

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:45: protocol_handlers);
Not passing the interceptors along and hooking them up means that you won't get
ServiceWorker (And fairly soon, AppCache) support.  They should all be hooked
up.

If you don't want them, should have a more principled way of excluding them,
since it's unclear what other interceptors may be added, over time.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:60: if (url.scheme() ==
kProtocolList[i])
optional nit:  While this isn't remotely performance sensitive, GURL::scheme()
creates and returns a new string on every call, so may want to cache it locally.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
File chromecast/shell/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:174: if
(!system_dependencies_initialized_) {
Optional for this and and the next two functions:  There's weak preference in
Chrome to use early returns when there's nothing else to do, unless there's a
specific reason not to.  This reduces the indentation and makes code a little
easier to follow.  Admittedly, this code isn't too complicated, anyways.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:189:
http_server_properties_.reset(new net::HttpServerPropertiesImpl);
For performance reasons, may want an on-disk HttpServerProperties (And for
security, and on-disk ServerBoundCertService), but should be fine without them,
for now.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:250: // by deafult at
the content level.
nit:  deafult -> default

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:279:
http_user_agent_settings_.get());
Should probably set up an in-memory cookie store for the system context as well.

[lcwu1, 2014-07-09 02:16:29]

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
File chromecast/shell/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:45: protocol_handlers);
On 2014/07/08 15:43:59, mmenke wrote:
> Not passing the interceptors along and hooking them up means that you won't
get
> ServiceWorker (And fairly soon, AppCache) support.  They should all be hooked
> up.
> 
> If you don't want them, should have a more principled way of excluding them,
> since it's unclear what other interceptors may be added, over time.

Done. Hooked up the interceptors as suggested.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:60: if (url.scheme() ==
kProtocolList[i])
On 2014/07/08 15:43:59, mmenke wrote:
> optional nit:  While this isn't remotely performance sensitive, GURL::scheme()
> creates and returns a new string on every call, so may want to cache it
locally.

Done.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
File chromecast/shell/browser/url_request_context_factory.cc (right):

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:174: if
(!system_dependencies_initialized_) {
On 2014/07/08 15:43:59, mmenke wrote:
> Optional for this and and the next two functions:  There's weak preference in
> Chrome to use early returns when there's nothing else to do, unless there's a
> specific reason not to.  This reduces the indentation and makes code a little
> easier to follow.  Admittedly, this code isn't too complicated, anyways.

Done.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:189:
http_server_properties_.reset(new net::HttpServerPropertiesImpl);
On 2014/07/08 15:43:59, mmenke wrote:
> For performance reasons, may want an on-disk HttpServerProperties (And for
> security, and on-disk ServerBoundCertService), but should be fine without
them,
> for now.

Added a TODO here.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:250: // by deafult at
the content level.
On 2014/07/08 15:43:59, mmenke wrote:
> nit:  deafult -> default

Done.

https://codereview.chromium.org/223143003/diff/160001/chromecast/shell/browse...
chromecast/shell/browser/url_request_context_factory.cc:279:
http_user_agent_settings_.get());
On 2014/07/08 15:43:59, mmenke wrote:
> Should probably set up an in-memory cookie store for the system context as
well.

Done.

[mmenke, 2014-07-09 14:30:11]

LGTM!

[lcwu1, 2014-07-09 19:52:21]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-07-09 19:54:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/190001

[lcwu1, 2014-07-09 21:15:55]

@ben and @sky,

this CL requires LGTM from owners of ui/. More specifically, we include/use
aura, gfx, resource_bundle, and l10n headers/APIs in the following files:

chromecast/service/cast_service.cc
chromecast/shell/app/cast_main_delegate.cc
chromecast/shell/common/cast_content_client.cc

Thanks.

[lcwu1, 2014-07-10 21:19:27]

On 2014/07/09 21:15:55, lcwu1 wrote:
> @ben and @sky,
> 
> this CL requires LGTM from owners of ui/. More specifically, we include/use
> aura, gfx, resource_bundle, and l10n headers/APIs in the following files:
> 
> chromecast/service/cast_service.cc
> chromecast/shell/app/cast_main_delegate.cc
> chromecast/shell/common/cast_content_client.cc
> 
> Thanks.

Pinging ben@ and sky@. Could you take a look at uses of ui headers/APIs in this
CL? Thanks.

[sky, 2014-07-10 22:57:21]

DEPS and includes LGTM. In the future pick one reviewer (and not always me)
otherwise reviewers are likely to assume another persons is going to review.

[lcwu1, 2014-07-10 23:19:02]

On 2014/07/10 22:57:21, sky wrote:
> DEPS and includes LGTM. In the future pick one reviewer (and not always me)
> otherwise reviewers are likely to assume another persons is going to review.

Cool. I will keep that in mind. Thanks.

[lcwu1, 2014-07-10 23:31:41]

The CQ bit was checked by lcwu@chromium.org

[commit-bot: I haz the power, 2014-07-10 23:33:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/lcwu@chromium.org/223143003/230001

[commit-bot: I haz the power, 2014-07-11 02:44:38]

Change committed as 282508

[tfarina, 2014-07-22 20:11:50]

https://codereview.chromium.org/223143003/diff/230001/chromecast/shell/app/ca...
File chromecast/shell/app/cast_main_delegate.h (right):

https://codereview.chromium.org/223143003/diff/230001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.h:33: static void
InitializeResourceBundle();
Was this copied from content/shell/app/shell_main_delegate.h or ash/shell or
apps/shell?

I just filed crbug.com/396217 to clean this up.