Chromecast: initial checkin of Android-based cast shell.

Chromecast: initial checkin of Android-based cast shell.

R=byungchul@chromium.org,lcwu@chromium.org,yfriedman@chromium.org
BUG=400876
Committed: https://crrev.com/407189f61ab162096ea98c40b2ecd85875bd6049
Cr-Commit-Position: refs/heads/master@{#294476}

[gunsch, 2014-08-19 17:50:25]

On 2014/08/19 17:46:14, gunsch wrote:

jam@/igsolla@: Please add whomever you think would be best on the Clank side to
review our use of the content/ Java APIs as well. Thanks.

[jam, 2014-08-19 19:49:15]

On 2014/08/19 17:50:25, gunsch wrote:
> On 2014/08/19 17:46:14, gunsch wrote:
> 
> jam@/igsolla@: Please add whomever you think would be best on the Clank side
to
> review our use of the content/ Java APIs as well. Thanks.

I don't know who would be best to review; check with Android chrome team
directly.

btw i'm very overloaded with CY so please get reviewer stamp from another
reviewer, thanks.

[gunsch, 2014-08-19 21:39:28]

On 2014/08/19 19:49:15, jam wrote:
> On 2014/08/19 17:50:25, gunsch wrote:
> > On 2014/08/19 17:46:14, gunsch wrote:
> > 
> > jam@/igsolla@: Please add whomever you think would be best on the Clank side
> to
> > review our use of the content/ Java APIs as well. Thanks.
> 
> I don't know who would be best to review; check with Android chrome team
> directly.
> 
> btw i'm very overloaded with CY so please get reviewer stamp from another
> reviewer, thanks.

-jam, +yfriedman. Thanks for volunteering some time, Yaron.

[Yaron, 2014-08-20 18:11:02]

https://codereview.chromium.org/490603002/diff/1/chromecast/android/cast_jni_...
File chromecast/android/cast_jni_registrar.h (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/cast_jni_...
chromecast/android/cast_jni_registrar.h:13: // Register all JNI bindings
necessary for media shell.
update

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android.cc:13: ChromecastConfigAndroid*
g_instance = NULL;
Why not use LazyInstance?

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android.h:23: bool CanSendUsageStats();
Should this be handled as part of the MetricsService?

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android_simple.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android_simple.cc:10: bool
ChromecastConfigAndroid::RegisterJni(JNIEnv* env) {
So there will be a private/internal that does real work?

https://codereview.chromium.org/490603002/diff/1/chromecast/android/platform_...
File chromecast/android/platform_jni_loader_simple.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/platform_...
chromecast/android/platform_jni_loader_simple.cc:10: void
PlatformRegisterJni(JNIEnv* env) {
Again, not super excited about these stubs. Will these be used? Do you have more
details about upstreaming plan somewher?

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:48: 'net/network_change_notifier_cast.cc',
Why not include in the existing "net" target? Is this really just a stub and
there's no NetworkCHangeNotifier in cast?

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:239: 'target_name': 'libcast_shell_android',
any reason this needs "android" in the name?

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:288:
'../tools/android/forwarder/forwarder.gyp:forwarder',
Why does the java target depend on forwarder and image_diff?

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:314: # if the actual Java path is used.
what's the sync.stamp file for? is it part of this?

Don't you at least need an Application/Activity? Why is this empty?

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:48:
ResourceExtractor.setMandatoryPaksToExtract(MANDATORY_PAK_FILES);
Aren't both of these done in CastApplication.initializeApplicationParameters ?

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:106:
private static void initializePlatformKeySystem() {
?

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:99:
public static String sanitizeUrl(String url) {
This looks to be copied from ContentShell. I think we were ok with it there
cause it's meant for testing. If this is used in user-facing builds, we probably
want to use Url classes.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/app/android...
File chromecast/shell/app/android/cast_jni_loader.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/app/android...
chromecast/shell/app/android/cast_jni_loader.cc:33:
chromecast::android::PlatformRegisterJni(env);
how does this differ from the above registration?

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.cc:62:
Java_CastWindowAndroid_initFromNativeTabContents(
initFromNativeWebContents

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_manager.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_manager.cc:32:
base::LazyInstance<GlobalState> g_global_state = LAZY_INSTANCE_INITIALIZER;
why the extra wrapper struct?

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/dev...
File chromecast/shell/browser/devtools/remote_debugging_server.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/remote_debugging_server.cc:47:
UnixDomainListenSocketWithAbstractNamespaceFactory(
This is being changed in https://codereview.chromium.org/296053012/

[gunsch, 2014-08-21 22:31:28]

Thanks for the thorough review. Some comments inline.

If these answers aren't detailed enough, particularly about our overall code
structure, let me know and we can set up a meeting to chat more (with lcwu@ and
myself, at least).

https://codereview.chromium.org/490603002/diff/1/chromecast/android/cast_jni_...
File chromecast/android/cast_jni_registrar.h (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/cast_jni_...
chromecast/android/cast_jni_registrar.h:13: // Register all JNI bindings
necessary for media shell.
On 2014/08/20 18:11:01, Yaron wrote:
> update

Done.

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android.cc:13: ChromecastConfigAndroid*
g_instance = NULL;
On 2014/08/20 18:11:01, Yaron wrote:
> Why not use LazyInstance?

Done.

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android.h:23: bool CanSendUsageStats();
On 2014/08/20 18:11:01, Yaron wrote:
> Should this be handled as part of the MetricsService?

We do use MetricsService, and provide our own MetricsServiceClient (see:
https://codereview.chromium.org/482813004/), but it's not yet (as of either of
these CLs) hooked up to Android.

That said, I took a second look and it looks like this method isn't actually
necessary in the public code. I'll remove this method at least.

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
File chromecast/android/chromecast_config_android_simple.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/chromecas...
chromecast/android/chromecast_config_android_simple.cc:10: bool
ChromecastConfigAndroid::RegisterJni(JNIEnv* env) {
On 2014/08/20 18:11:01, Yaron wrote:
> So there will be a private/internal that does real work?

This stub in particular is silly, since I have a PlatformRegisterJni to cover
this. Removed.

https://codereview.chromium.org/490603002/diff/1/chromecast/android/platform_...
File chromecast/android/platform_jni_loader_simple.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/android/platform_...
chromecast/android/platform_jni_loader_simple.cc:10: void
PlatformRegisterJni(JNIEnv* env) {
On 2014/08/20 18:11:01, Yaron wrote:
> Again, not super excited about these stubs. Will these be used? Do you have
more
> details about upstreaming plan somewher?

I don't think we have a doc for public consumption. Le-Chun Wu (lcwu@) said that
he spoke with you about our high-level upstreaming plans a few months ago.

Our high-level plan is to upstream as much as possible, minus several pieces
that we're not ready or able to make public (our home screen UIs, partner code,
etc.). To continue compiling publicly, yes, we do have these stubs, and the
convention is to put them in their own file and methods named with "Platform" as
the differentiating keywords.

In terms of code organization, most of our private code is in an internal git
repo that gets mapped to |src/chromecast/internal/|. The functionality change
occurs largely by stubbing out a handful of methods in the public code (so that
one can build a working cast_shell from upstream chromium), but that get swapped
out with private implementations when building with the GYP flag
|chromecast_branding=="Chrome"|.

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:48: 'net/network_change_notifier_cast.cc',
On 2014/08/20 18:11:02, Yaron wrote:
> Why not include in the existing "net" target? Is this really just a stub and
> there's no NetworkCHangeNotifier in cast?

What do you mean by the existing "net" target? |src/net/net.gyp:net| ?

Chromium has overall expressed that they would prefer Chromecast to keep all of
our code in |src/chromecast/|, rather than scattering throughout the Chromium
codebase|. We've been explicitly disallowed a C-define such as |OS_CHROMECAST|,
so we intend to be a standalone content-embedder implementation.

This specific change in this CL separates out a |cast_net| target for ourselves,
because it is Linux-specific (for the Chromecast product) and not appropriate
when |OS=="android"|. In this case, cast_shell uses Android's
NetworkChangeNotifier.

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:239: 'target_name': 'libcast_shell_android',
On 2014/08/20 18:11:02, Yaron wrote:
> any reason this needs "android" in the name?

Since this generates a shared library |libcast_shell_android.so| that is one of
our primary outputs, we find it helpful to distinguish from a less clearly named
|libcast_shell.so|.

But if you feel strongly, I can change to simply libcast_shell.

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:288:
'../tools/android/forwarder/forwarder.gyp:forwarder',
On 2014/08/20 18:11:01, Yaron wrote:
> Why does the java target depend on forwarder and image_diff?

Removed.

https://codereview.chromium.org/490603002/diff/1/chromecast/chromecast.gyp#ne...
chromecast/chromecast.gyp:314: # if the actual Java path is used.
On 2014/08/20 18:11:02, Yaron wrote:
> what's the sync.stamp file for? is it part of this?
> 
> Don't you at least need an Application/Activity? Why is this empty?

|sync.stamp|: didn't mean to check it in. We had used it locally as a way to
force rebuilds of a generated version file. Removed, thanks for catching.

I have an |Application| and |Activity|. They're specified within the
|cast_shell_java| target above. All code in that target is shared by our
internal APK target, which builds the entire "Chromecast for Android TV" APK.

The difference between the two (from our perspective) should be conceptually
similar to "ContentShell.apk vs. Chrome on Android", with the caveat that in our
case the final "Chromecast for Android TV" product includes all of the public
code.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:48:
ResourceExtractor.setMandatoryPaksToExtract(MANDATORY_PAK_FILES);
On 2014/08/20 18:11:02, Yaron wrote:
> Aren't both of these done in CastApplication.initializeApplicationParameters ?

Done.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:106:
private static void initializePlatformKeySystem() {
On 2014/08/20 18:11:02, Yaron wrote:
> ?

Was left as a known TODO for more code to be added, but removed for the time
being.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_manager.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_manager.cc:32:
base::LazyInstance<GlobalState> g_global_state = LAZY_INSTANCE_INITIALIZER;
On 2014/08/20 18:11:02, Yaron wrote:
> why the extra wrapper struct?

Done.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/dev...
File chromecast/shell/browser/devtools/remote_debugging_server.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/dev...
chromecast/shell/browser/devtools/remote_debugging_server.cc:47:
UnixDomainListenSocketWithAbstractNamespaceFactory(
On 2014/08/20 18:11:02, Yaron wrote:
> This is being changed in https://codereview.chromium.org/296053012/

Acknowledged. Reviewed that CL for Byungchul but expecting that it will be
submitted before this, at which point I'll rebase those changes in. For the
meantime, leaving this as-is so this compiles.

[gunsch, 2014-08-22 22:44:31]

oops, missed a couple comments last round. addressed.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java
(right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/android/apk...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowAndroid.java:99:
public static String sanitizeUrl(String url) {
On 2014/08/20 18:11:02, Yaron wrote:
> This looks to be copied from ContentShell. I think we were ok with it there
> cause it's meant for testing. If this is used in user-facing builds, we
probably
> want to use Url classes.

Done.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/app/android...
File chromecast/shell/app/android/cast_jni_loader.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/app/android...
chromecast/shell/app/android/cast_jni_loader.cc:33:
chromecast::android::PlatformRegisterJni(env);
On 2014/08/20 18:11:02, Yaron wrote:
> how does this differ from the above registration?

Moved closer to above, explained.

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
File chromecast/shell/browser/android/cast_window_android.cc (right):

https://codereview.chromium.org/490603002/diff/1/chromecast/shell/browser/and...
chromecast/shell/browser/android/cast_window_android.cc:62:
Java_CastWindowAndroid_initFromNativeTabContents(
On 2014/08/20 18:11:02, Yaron wrote:
> initFromNativeWebContents

Done.

[lcwu1, 2014-08-27 02:33:03]

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:40: }
nit: add a comment "// namespace android"

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:244: 'cast_jni_headers',
Not int alphabetical order.

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:342: # This target contains all of the primary code of
|cast_shell|.
This line of comment doesn't look right in this place. It appears to be meant
for a target like cast_shell_core.

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service_android.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service_android.h:15:
CastServiceAndroid(content::BrowserContext* browser_context);
Add 'explicit'.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/DEPS
File chromecast/shell/app/DEPS (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/DEP...
chromecast/shell/app/DEPS:3: "+content/public/browser",
If this additional dep is for the browser_main_runner, you could add only
"+content/public/browser/browser_main_runner.h" here.

[lcwu1, 2014-08-27 16:47:47]

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:54: base::CPU cpu_info;
This variable doesn't appear to be used.

[lcwu1, 2014-08-27 16:53:45]

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/devtools/remote_debugging_server.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/devtools/remote_debugging_server.cc:48: socket_name,
"", base::Bind(&content::CanUserConnectToDevTools));
It looks like you will need to rebase this code to
https://codereview.chromium.org/296053012/

[byungchul, 2014-08-27 18:54:28]

A partial review. More comments will go.

https://codereview.chromium.org/490603002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/490603002/diff/40001/build/common.gypi#newcod...
build/common.gypi:100: 'embedded%': 1,
Why not for android?

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:22: void
SetSendUsageStatsChangedCallback(
set_send_usage_stats_chcnaged_callback

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:25: const
base::Callback<void(bool)>& send_usage_stats_changed_callback() {
const function.

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:340: }, {
Please add a comment saying that # OS!="android". It's a long condition.

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

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:54: case FILE_CAST_ANDROID_LOG: {
alphabetic order?

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
File chromecast/common/cast_paths.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
chromecast/common/cast_paths.h:24: FILE_CAST_ANDROID_LOG,
alphabetic order?

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service_android.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service_android.cc:30: base::Callback<void(bool)>());
Not necessary to set null function because the default callback is already null.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
File chromecast/shell/android/apk/res/values/strings.xml (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/res/values/strings.xml:9: <string
name="app_name">Chromecast Android Shell</string>
You may want to sync with Tomer for public apk name.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:91:
if (CommandLine.getInstance().hasSwitch(BaseSwitches.WAIT_FOR_JAVA_DEBUGGER)) {
Fast return on false condition.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:97:

Remove extra blank line.

[byungchul, 2014-08-27 23:19:17]

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:76: #if !defined(OS_ANDROID)
Don't you have to guard it in .h as well?

[gunsch, 2014-08-28 00:12:09]

https://codereview.chromium.org/490603002/diff/40001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/490603002/diff/40001/build/common.gypi#newcod...
build/common.gypi:100: 'embedded%': 1,
On 2014/08/27 18:54:27, byungchul wrote:
> Why not for android?

Embedded implies a lot of things we don't want (in particular, implies enabling
Ozone/Aura). Note that Clank is not built with embedded=1.

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:22: void
SetSendUsageStatsChangedCallback(
On 2014/08/27 18:54:27, byungchul wrote:
> set_send_usage_stats_chcnaged_callback

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:25: const
base::Callback<void(bool)>& send_usage_stats_changed_callback() {
On 2014/08/27 18:54:27, byungchul wrote:
> const function.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:40: }
On 2014/08/27 02:33:02, lcwu1 wrote:
> nit: add a comment "// namespace android"

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:244: 'cast_jni_headers',
On 2014/08/27 02:33:02, lcwu1 wrote:
> Not int alphabetical order.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:340: }, {
On 2014/08/27 18:54:27, byungchul wrote:
> Please add a comment saying that # OS!="android". It's a long condition.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:342: # This target contains all of the primary code of
|cast_shell|.
On 2014/08/27 02:33:02, lcwu1 wrote:
> This line of comment doesn't look right in this place. It appears to be meant
> for a target like cast_shell_core.

Done.

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

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
chromecast/common/cast_paths.cc:54: case FILE_CAST_ANDROID_LOG: {
On 2014/08/27 18:54:27, byungchul wrote:
> alphabetic order?

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
File chromecast/common/cast_paths.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/common/cast_p...
chromecast/common/cast_paths.h:24: FILE_CAST_ANDROID_LOG,
On 2014/08/27 18:54:28, byungchul wrote:
> alphabetic order?

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service_android.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service_android.cc:30: base::Callback<void(bool)>());
On 2014/08/27 18:54:28, byungchul wrote:
> Not necessary to set null function because the default callback is already
null.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
File chromecast/service/cast_service_android.h (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/service/cast_...
chromecast/service/cast_service_android.h:15:
CastServiceAndroid(content::BrowserContext* browser_context);
On 2014/08/27 02:33:02, lcwu1 wrote:
> Add 'explicit'.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
File chromecast/shell/android/apk/res/values/strings.xml (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/res/values/strings.xml:9: <string
name="app_name">Chromecast Android Shell</string>
On 2014/08/27 18:54:28, byungchul wrote:
> You may want to sync with Tomer for public apk name.

Not necessary, there is no product here.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:91:
if (CommandLine.getInstance().hasSwitch(BaseSwitches.WAIT_FOR_JAVA_DEBUGGER)) {
On 2014/08/27 18:54:28, byungchul wrote:
> Fast return on false condition.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/android...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:97:

On 2014/08/27 18:54:28, byungchul wrote:
> Remove extra blank line.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/DEPS
File chromecast/shell/app/DEPS (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/DEP...
chromecast/shell/app/DEPS:3: "+content/public/browser",
On 2014/08/27 02:33:02, lcwu1 wrote:
> If this additional dep is for the browser_main_runner, you could add only
> "+content/public/browser/browser_main_runner.h" here.

Done.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:54: base::CPU cpu_info;
On 2014/08/27 16:47:46, lcwu1 wrote:
> This variable doesn't appear to be used.

True, it is not used---but instantiating it has many side effects.

Not having this was the cause of the "AllowScopedIO" hack we had locally inside
UrlRequestContextGetter. This constructor needs to be invoked once during
startup to allow base::CPU to read /proc/cpuinfo, then cache the value.

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:76: #if !defined(OS_ANDROID)
On 2014/08/27 23:19:17, byungchul wrote:
> Don't you have to guard it in .h as well?

It is already?

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/browser...
File chromecast/shell/browser/devtools/remote_debugging_server.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/browser...
chromecast/shell/browser/devtools/remote_debugging_server.cc:48: socket_name,
"", base::Bind(&content::CanUserConnectToDevTools));
On 2014/08/27 16:53:45, lcwu1 wrote:
> It looks like you will need to rebase this code to
> https://codereview.chromium.org/296053012/

Done.

[byungchul, 2014-08-28 01:34:39]

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:23: const
base::Callback<void(bool)>& callback);
Why not a inline function?

https://codereview.chromium.org/490603002/diff/60001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/60001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:368: }, { # OS != "android"
2 whitespaces between '{' and '#'

[lcwu1, 2014-08-28 02:40:29]

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:54: base::CPU cpu_info;
On 2014/08/28 00:12:09, gunsch wrote:
> On 2014/08/27 16:47:46, lcwu1 wrote:
> > This variable doesn't appear to be used.
> 
> True, it is not used---but instantiating it has many side effects.
> 
> Not having this was the cause of the "AllowScopedIO" hack we had locally
inside
> UrlRequestContextGetter. This constructor needs to be invoked once during
> startup to allow base::CPU to read /proc/cpuinfo, then cache the value.

I see. In that case I think we should add more comments here explaining why it
needs to happen here.

[gunsch, 2014-08-28 02:59:27]

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
File chromecast/shell/app/cast_main_delegate.cc (right):

https://codereview.chromium.org/490603002/diff/40001/chromecast/shell/app/cas...
chromecast/shell/app/cast_main_delegate.cc:54: base::CPU cpu_info;
On 2014/08/28 02:40:29, lcwu1 wrote:
> On 2014/08/28 00:12:09, gunsch wrote:
> > On 2014/08/27 16:47:46, lcwu1 wrote:
> > > This variable doesn't appear to be used.
> > 
> > True, it is not used---but instantiating it has many side effects.
> > 
> > Not having this was the cause of the "AllowScopedIO" hack we had locally
> inside
> > UrlRequestContextGetter. This constructor needs to be invoked once during
> > startup to allow base::CPU to read /proc/cpuinfo, then cache the value.
> 
> I see. In that case I think we should add more comments here explaining why it
> needs to happen here.
> 

Done.

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:23: const
base::Callback<void(bool)>& callback);
On 2014/08/28 01:34:39, byungchul wrote:
> Why not a inline function?

Chromium style guide: "Simple accessors should generally be the only inline
functions. These should be named unix_hacker_style().  Virtual functions should
never be declared this way."

https://codereview.chromium.org/490603002/diff/60001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/490603002/diff/60001/chromecast/chromecast.gy...
chromecast/chromecast.gyp:368: }, { # OS != "android"
On 2014/08/28 01:34:39, byungchul wrote:
> 2 whitespaces between '{' and '#'

Done.

[lcwu1, 2014-08-28 05:07:07]

lgtm

[byungchul, 2014-08-28 05:49:49]

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
File chromecast/android/chromecast_config_android.h (right):

https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
chromecast/android/chromecast_config_android.h:23: const
base::Callback<void(bool)>& callback);
On 2014/08/28 02:59:26, gunsch wrote:
> On 2014/08/28 01:34:39, byungchul wrote:
> > Why not a inline function?
> 
> Chromium style guide: "Simple accessors should generally be the only inline
> functions. These should be named unix_hacker_style().  Virtual functions
should
> never be declared this way."

Do you mean set_send_usage_stats_changed_callback() is not a simple accessor?

[gunsch, 2014-08-28 17:07:49]

On 2014/08/28 05:49:49, byungchul wrote:
>
https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
> File chromecast/android/chromecast_config_android.h (right):
> 
>
https://codereview.chromium.org/490603002/diff/60001/chromecast/android/chrom...
> chromecast/android/chromecast_config_android.h:23: const
> base::Callback<void(bool)>& callback);
> On 2014/08/28 02:59:26, gunsch wrote:
> > On 2014/08/28 01:34:39, byungchul wrote:
> > > Why not a inline function?
> > 
> > Chromium style guide: "Simple accessors should generally be the only inline
> > functions. These should be named unix_hacker_style().  Virtual functions
> should
> > never be declared this way."
> 
> Do you mean set_send_usage_stats_changed_callback() is not a simple accessor?

Discussed offline. It is not an acccessor :) changed back to original naming per
Byungchul's request

[byungchul, 2014-08-28 17:11:45]

lgtm

[Yaron, 2014-09-06 01:10:14]

https://codereview.chromium.org/490603002/diff/100001/chromecast/android/plat...
File chromecast/android/platform_jni_loader_simple.cc (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/android/plat...
chromecast/android/platform_jni_loader_simple.cc:2: // Use of this source code
is governed by a BSD-style license that can be
I'd rename this platform_jni_loader_stub.cc (although I see it's consistent with
other cast examples so fine in a separate change)

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:297:
'<(cast_internal_gyp):cast_shell_android_internal'
So this setup is a little different than android. The way we've handled it is to
instead have a variable "extension hook" of sorts. Then you can have a
supplement.gypi. So you can depend on something like "cast_shell_external_deps"
and then have that variable populated by a supplement.gypi in your private repo.
supplement.gypi is automatically loaded by gyp_chromium so the presence of a
private repo automatically adjusts your build. It's arguably cleaner from a
chromium repo point of view but I'm not specifically opposed to what you have.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File chromecast/shell/android/apk/AndroidManifest.xml (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/AndroidManifest.xml:94: <service
android:name="org.chromium.content.app.SandboxedProcessService12"
These were upped to 19 as part of not re-using renderers ever on android. Can
you do the same?

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:62:
CommandLine.getInstance().appendSwitchWithValue("force-device-scale-factor",
"1");
use constant from ContentSwitches

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:63:
CastBrowserHelper.initializeBrowser(getApplicationContext());
if this returns false, you should probably Toast and finish() since you're about
to crash

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:35:
public static class Delegate {
Why isn't this an interface? Without multiple inheritance, if you aren't going
to actually provide abstract functionality I think it's worse to provide default
impls

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/DEPS
File chromecast/shell/app/DEPS (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/DE...
chromecast/shell/app/DEPS:3: "+content/public/browser/android/compositor.h",
I think simplifying to content/public/browser is fine

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.cc:52: // Create an instance of the CPU
class to parse /proc/cpuinfo and cache the
why is LINUX included here?

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:151:
CHECK(PathService::Get(FILE_CAST_PAK, &pak_file));
Where is the child process accessing the passed fd?

[gunsch, 2014-09-08 19:42:23]

https://codereview.chromium.org/490603002/diff/100001/chromecast/android/plat...
File chromecast/android/platform_jni_loader_simple.cc (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/android/plat...
chromecast/android/platform_jni_loader_simple.cc:2: // Use of this source code
is governed by a BSD-style license that can be
On 2014/09/06 01:10:13, Yaron wrote:
> I'd rename this platform_jni_loader_stub.cc (although I see it's consistent
with
> other cast examples so fine in a separate change)

Done.

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:297:
'<(cast_internal_gyp):cast_shell_android_internal'
On 2014/09/06 01:10:13, Yaron wrote:
> So this setup is a little different than android. The way we've handled it is
to
> instead have a variable "extension hook" of sorts. Then you can have a
> supplement.gypi. So you can depend on something like
"cast_shell_external_deps"
> and then have that variable populated by a supplement.gypi in your private
repo.
> supplement.gypi is automatically loaded by gyp_chromium so the presence of a
> private repo automatically adjusts your build. It's arguably cleaner from a
> chromium repo point of view but I'm not specifically opposed to what you have.

Interesting. So if you have the private repo checked out (and your
supplement.gypi is always picked up), how do you build the public version?

One feature of our repo setup is we have is that we are generally building
several variations (public/internal, Android/Linux, x86 for desktop, ARM for the
dongle) from the same checkout, as opposed to the chromium setup where it seems
you can generally build Android or Linux but not both. To accomplish this, we
have scripts that set up various environment variables/GYP defines before
running a given build.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File chromecast/shell/android/apk/AndroidManifest.xml (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/AndroidManifest.xml:94: <service
android:name="org.chromium.content.app.SandboxedProcessService12"
On 2014/09/06 01:10:13, Yaron wrote:
> These were upped to 19 as part of not re-using renderers ever on android. Can
> you do the same?

Done.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java:62:
CommandLine.getInstance().appendSwitchWithValue("force-device-scale-factor",
"1");
On 2014/09/06 01:10:14, Yaron wrote:
> use constant from ContentSwitches

Done.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:63:
CastBrowserHelper.initializeBrowser(getApplicationContext());
On 2014/09/06 01:10:14, Yaron wrote:
> if this returns false, you should probably Toast and finish() since you're
about
> to crash

Done.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastWindowManager.java:35:
public static class Delegate {
On 2014/09/06 01:10:14, Yaron wrote:
> Why isn't this an interface? Without multiple inheritance, if you aren't going
> to actually provide abstract functionality I think it's worse to provide
default
> impls

Done.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/DEPS
File chromecast/shell/app/DEPS (right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/DE...
chromecast/shell/app/DEPS:3: "+content/public/browser/android/compositor.h",
On 2014/09/06 01:10:14, Yaron wrote:
> I think simplifying to content/public/browser is fine

Done.

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/app/ca...
chromecast/shell/app/cast_main_delegate.cc:52: // Create an instance of the CPU
class to parse /proc/cpuinfo and cache the
On 2014/09/06 01:10:14, Yaron wrote:
> why is LINUX included here?

Not sure, but ShellMainDelegate and ChromeMainDelegate have the exact same
condition.

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:151:
CHECK(PathService::Get(FILE_CAST_PAK, &pak_file));
On 2014/09/06 01:10:14, Yaron wrote:
> Where is the child process accessing the passed fd?

cast_main_delegate.cc, in CastMainDelegate::InitializeResourceBundle.

[gunsch, 2014-09-10 16:22:06]

gunsch@chromium.org changed reviewers:
+ jam@chromium.org

[gunsch, 2014-09-10 16:24:00]

+jam, can you take a quick look for the DEPS change in
chromecast/shell/app/DEPS? We added another "content/public/browser" dependency
since our ContentMainDelegate implementation needs to create a
BrowserMainRunner.

[Yaron, 2014-09-11 06:45:17]

lgtm

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:297:
'<(cast_internal_gyp):cast_shell_android_internal'
On 2014/09/08 19:42:23, gunsch wrote:
> On 2014/09/06 01:10:13, Yaron wrote:
> > So this setup is a little different than android. The way we've handled it
is
> to
> > instead have a variable "extension hook" of sorts. Then you can have a
> > supplement.gypi. So you can depend on something like
> "cast_shell_external_deps"
> > and then have that variable populated by a supplement.gypi in your private
> repo.
> > supplement.gypi is automatically loaded by gyp_chromium so the presence of a
> > private repo automatically adjusts your build. It's arguably cleaner from a
> > chromium repo point of view but I'm not specifically opposed to what you
have.
> 
> Interesting. So if you have the private repo checked out (and your
> supplement.gypi is always picked up), how do you build the public version?
> 
You could still have a gyp define control it but presumably using the internal
repo is the common operatoin if you have access, right? So something like:
use_public_build and default it off.

> One feature of our repo setup is we have is that we are generally building
> several variations (public/internal, Android/Linux, x86 for desktop, ARM for
the
> dongle) from the same checkout, as opposed to the chromium setup where it
seems
> you can generally build Android or Linux but not both. To accomplish this, we
> have scripts that set up various environment variables/GYP defines before
> running a given build.

sure. You can do that with the "cr" tool as well. Essentially you run gyp for a
configuration with a specific out dir and then you can have multiple builds
simultaneously.

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:63:
CastBrowserHelper.initializeBrowser(getApplicationContext());
On 2014/09/08 19:42:23, gunsch wrote:
> On 2014/09/06 01:10:14, Yaron wrote:
> > if this returns false, you should probably Toast and finish() since you're
> about
> > to crash
> 
> Done.

I don't see a call to finish(). Is that not an option in onCreate?

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/browse...
chromecast/shell/browser/cast_content_browser_client.cc:151:
CHECK(PathService::Get(FILE_CAST_PAK, &pak_file));
On 2014/09/08 19:42:23, gunsch wrote:
> On 2014/09/06 01:10:14, Yaron wrote:
> > Where is the child process accessing the passed fd?
> 
> cast_main_delegate.cc, in CastMainDelegate::InitializeResourceBundle.

right, thanks

[gunsch, 2014-09-11 16:19:07]

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

https://codereview.chromium.org/490603002/diff/100001/chromecast/chromecast.g...
chromecast/chromecast.gyp:297:
'<(cast_internal_gyp):cast_shell_android_internal'
On 2014/09/11 06:45:16, Yaron_OOO_until_9-22 wrote:
> On 2014/09/08 19:42:23, gunsch wrote:
> > On 2014/09/06 01:10:13, Yaron wrote:
> > > So this setup is a little different than android. The way we've handled it
> is
> > to
> > > instead have a variable "extension hook" of sorts. Then you can have a
> > > supplement.gypi. So you can depend on something like
> > "cast_shell_external_deps"
> > > and then have that variable populated by a supplement.gypi in your private
> > repo.
> > > supplement.gypi is automatically loaded by gyp_chromium so the presence of
a
> > > private repo automatically adjusts your build. It's arguably cleaner from
a
> > > chromium repo point of view but I'm not specifically opposed to what you
> have.
> > 
> > Interesting. So if you have the private repo checked out (and your
> > supplement.gypi is always picked up), how do you build the public version?
> > 
> You could still have a gyp define control it but presumably using the internal
> repo is the common operatoin if you have access, right? So something like:
> use_public_build and default it off.
> 
> > One feature of our repo setup is we have is that we are generally building
> > several variations (public/internal, Android/Linux, x86 for desktop, ARM for
> the
> > dongle) from the same checkout, as opposed to the chromium setup where it
> seems
> > you can generally build Android or Linux but not both. To accomplish this,
we
> > have scripts that set up various environment variables/GYP defines before
> > running a given build.
> 
> sure. You can do that with the "cr" tool as well. Essentially you run gyp for
a
> configuration with a specific out dir and then you can have multiple builds
> simultaneously.

Sure, we build to different out directories as well, largely controlled by
GYP_GENERATOR_FLAGS. How does syncing all the repositories work, since DEPS has
different conditions for 'android' and 'linux' w/r/t which repositories it pulls
down?

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
File
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java
(right):

https://codereview.chromium.org/490603002/diff/100001/chromecast/shell/androi...
chromecast/shell/android/apk/src/org/chromium/chromecast/shell/CastShellActivity.java:63:
CastBrowserHelper.initializeBrowser(getApplicationContext());
On 2014/09/11 06:45:16, Yaron_OOO_until_9-22 wrote:
> On 2014/09/08 19:42:23, gunsch wrote:
> > On 2014/09/06 01:10:14, Yaron wrote:
> > > if this returns false, you should probably Toast and finish() since you're
> > about
> > > to crash
> > 
> > Done.
> 
> I don't see a call to finish(). Is that not an option in onCreate?

Done.

[jam, 2014-09-11 16:35:57]

lgtm for DEPS

[Yaron, 2014-09-11 16:37:03]

On Thu, Sep 11, 2014 at 9:19 AM, <gunsch@chromium.org> wrote:

>
> https://codereview.chromium.org/490603002/diff/100001/
> chromecast/chromecast.gyp
> File chromecast/chromecast.gyp (right):
>
> https://codereview.chromium.org/490603002/diff/100001/
> chromecast/chromecast.gyp#newcode297
> chromecast/chromecast.gyp:297:
> '<(cast_internal_gyp):cast_shell_android_internal'
> On 2014/09/11 06:45:16, Yaron_OOO_until_9-22 wrote:
>
>> On 2014/09/08 19:42:23, gunsch wrote:
>> > On 2014/09/06 01:10:13, Yaron wrote:
>> > > So this setup is a little different than android. The way we've
>>
> handled it
>
>> is
>> > to
>> > > instead have a variable "extension hook" of sorts. Then you can
>>
> have a
>
>> > > supplement.gypi. So you can depend on something like
>> > "cast_shell_external_deps"
>> > > and then have that variable populated by a supplement.gypi in your
>>
> private
>
>> > repo.
>> > > supplement.gypi is automatically loaded by gyp_chromium so the
>>
> presence of a
>
>> > > private repo automatically adjusts your build. It's arguably
>>
> cleaner from a
>
>> > > chromium repo point of view but I'm not specifically opposed to
>>
> what you
>
>> have.
>> >
>> > Interesting. So if you have the private repo checked out (and your
>> > supplement.gypi is always picked up), how do you build the public
>>
> version?
>
>> >
>> You could still have a gyp define control it but presumably using the
>>
> internal
>
>> repo is the common operatoin if you have access, right? So something
>>
> like:
>
>> use_public_build and default it off.
>>
>
>  > One feature of our repo setup is we have is that we are generally
>>
> building
>
>> > several variations (public/internal, Android/Linux, x86 for desktop,
>>
> ARM for
>
>> the
>> > dongle) from the same checkout, as opposed to the chromium setup
>>
> where it
>
>> seems
>> > you can generally build Android or Linux but not both. To accomplish
>>
> this, we
>
>> > have scripts that set up various environment variables/GYP defines
>>
> before
>
>> > running a given build.
>>
>
>  sure. You can do that with the "cr" tool as well. Essentially you run
>>
> gyp for a
>
>> configuration with a specific out dir and then you can have multiple
>>
> builds
>
>> simultaneously.
>>
>
> Sure, we build to different out directories as well, largely controlled
> by GYP_GENERATOR_FLAGS. How does syncing all the repositories work,
> since DEPS has different conditions for 'android' and 'linux' w/r/t
> which repositories it pulls down?


android just adds additional repos on top of linux and everyone builds from
a linux host so gclient picks up the linux repos automaticalyl

>
>
> https://codereview.chromium.org/490603002/diff/100001/
> chromecast/shell/android/apk/src/org/chromium/chromecast/
> shell/CastShellActivity.java
> File
> chromecast/shell/android/apk/src/org/chromium/chromecast/
> shell/CastShellActivity.java
> (right):
>
> https://codereview.chromium.org/490603002/diff/100001/
> chromecast/shell/android/apk/src/org/chromium/chromecast/
> shell/CastShellActivity.java#newcode63
> chromecast/shell/android/apk/src/org/chromium/chromecast/
> shell/CastShellActivity.java:63:
> CastBrowserHelper.initializeBrowser(getApplicationContext());
> On 2014/09/11 06:45:16, Yaron_OOO_until_9-22 wrote:
>
>> On 2014/09/08 19:42:23, gunsch wrote:
>> > On 2014/09/06 01:10:14, Yaron wrote:
>> > > if this returns false, you should probably Toast and finish()
>>
> since you're
>
>> > about
>> > > to crash
>> >
>> > Done.
>>
>
>  I don't see a call to finish(). Is that not an option in onCreate?
>>
>
> Done.
>
> https://codereview.chromium.org/490603002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gunsch, 2014-09-11 17:06:56]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:13:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/490603002/200001

[gunsch, 2014-09-11 17:46:08]

The CQ bit was unchecked by gunsch@chromium.org

[gunsch, 2014-09-11 17:48:31]

The CQ bit was checked by gunsch@chromium.org

[gunsch, 2014-09-11 18:05:00]

The CQ bit was unchecked by gunsch@chromium.org

[gunsch, 2014-09-11 18:05:24]

The CQ bit was checked by gunsch@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:08:07]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[commit-bot: I haz the power, 2014-09-11 18:09:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/490603002/220001

[commit-bot: I haz the power, 2014-09-11 22:14:35]

Committed patchset #12 (id:220001) as 84621418425a1f7249d480809c36a7a4858aab8d

[commit-bot: I haz the power, 2014-09-11 22:31:03]

Patchset 12 (id:??) landed as
https://crrev.com/407189f61ab162096ea98c40b2ecd85875bd6049
Cr-Commit-Position: refs/heads/master@{#294476}