Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874943002:
[Chromecast] Add service for "displaying" cast web contents. (Closed)
Description[Chromecast] Add service for "displaying" cast web contents.
Audio-only Android builds should not start an activity, so this change
adds the option of using a service.
Rather than directly starting an activity to display cast apps,
CastContentWindowAndroid uses CastWebContentsComponent to start and
interact with whichever kind of component is appropriate for the build.
This is controlled with the build flag 'display_web_contents_in_service', which
defaults to is_cast_audio_only.
BUG=internal 36596922
Review-Url: https://codereview.chromium.org/2874943002
Cr-Commit-Position: refs/heads/master@{#473670}
Committed: https://chromium.googlesource.com/chromium/src/+/9540c6d6f520ad49921f0dc4e6b925324c623dfc
Patch Set 1 #
Total comments: 22
Patch Set 2 : Add service for "displaying" cast web contents. #Patch Set 3 : Add service for "displaying" cast web contents. #
Total comments: 6
Patch Set 4 : Add service for "displaying" cast web contents. #
Total comments: 9
Patch Set 5 : [Chromecast] Add service for "displaying" cast web contents. #Patch Set 6 : [Chromecast] Add service for "displaying" cast web contents. #Patch Set 7 : [Chromecast] Add service for "displaying" cast web contents. #Messages
Total messages: 49 (32 generated)
Description was changed from ========== Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'cast_android_use_service', which defaults to false for now. BUG=internal 36596922 ========== to ========== Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'cast_android_use_service', which defaults to false for now. BUG=internal 36596922 ==========
thoren@chromium.org changed reviewers: + halliwell@chromium.org
thoren@chromium.org changed required reviewers: + halliwell@chromium.org
thoren@chromium.org changed reviewers: + sanfin@chromium.org, slan@chromium.org
thoren@chromium.org changed required reviewers: + sanfin@chromium.org, slan@chromium.org - halliwell@chromium.org
@slan: Because OWNERS @sanfin: PTAL
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/BUILD.gn (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/BUILD.gn:73: junit_binary("cast_shell_java_junit_tests") { Let's call this "cast_shell_junit_tests" for now, since "java" and "junit" are somewhat redundant, and nothing else in Chromium uses "java_junit" in their target names. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:90: public void onComponentClosed() { nit: Prefer to have Java private methods above native private methods. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:87: // TODO(derekjchow): Remove this call. Do the unittests have a problem if we remove the initializeBrowser() call? If not, how about we remove it here and in the CastWebContentsService? https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:45: class ActivityDelegate implements Delegate { Does this need to be package-private? https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:46: private static final String TAG = CastWebContentsComponent.TAG + ".ActivityDelegate"; We don't want dots in tag names because the log elider for crash reporting interprets it as a web address, and filters it out for privacy reasons. I'm also concerned about the total length of the tag for this and for ServiceDelegate. I forget what the limit is, but can you confirm that the full tag is printed in the logcat? https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:71: class ServiceDelegate implements Delegate { Does this need to be package-private? https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:104: class Receiver extends BroadcastReceiver { Does this need to be package-private? https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:121: private static final boolean DEBUG = true; We might want to make this false in the final patch set, unless we decide that the activity lifecycle logging is useful enough to keep turned on. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:126: "com.google.android.apps.castshell.intent.extra.WEB_CONTENTS"; These are redeclared in CastWebContentsService and CastWebContentsActivity. How about making these package-private, or even moving them to a package-private Constants class? https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni File chromecast/chromecast.gni (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni#n... chromecast/chromecast.gni:29: cast_android_use_service = false Do we want to enable this on audio-only builds yet? https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni#n... chromecast/chromecast.gni:29: cast_android_use_service = false As for naming, how about display_web_contents_in_service? That's a bit more clear about what the service is being used for.
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/BUILD.gn (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/BUILD.gn:73: junit_binary("cast_shell_java_junit_tests") { On 2017/05/15 19:47:24, Simeon wrote: > Let's call this "cast_shell_junit_tests" for now, since "java" and > "junit" are somewhat redundant, and nothing else in Chromium uses "java_junit" > in their target names. Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastContentWindowAndroid.java:90: public void onComponentClosed() { On 2017/05/15 19:47:24, Simeon wrote: > nit: Prefer to have Java private methods above native private methods. Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:87: // TODO(derekjchow): Remove this call. On 2017/05/15 19:47:24, Simeon wrote: > Do the unittests have a problem if we remove the initializeBrowser() call? If > not, how about we remove it here and in the CastWebContentsService? Doesn't seem to break anything. Done https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:45: class ActivityDelegate implements Delegate { On 2017/05/15 19:47:24, Simeon wrote: > Does this need to be package-private? Nope. Went ahead and made it private. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:46: private static final String TAG = CastWebContentsComponent.TAG + ".ActivityDelegate"; On 2017/05/15 19:47:24, Simeon wrote: > We don't want dots in tag names because the log elider for crash reporting > interprets it as a web address, and filters it out for privacy reasons. > > I'm also concerned about the total length of the tag for this and for > ServiceDelegate. I forget what the limit is, but can you confirm that the full > tag is printed in the logcat? It was being printed, but I went ahead and changed it. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:71: class ServiceDelegate implements Delegate { On 2017/05/15 19:47:24, Simeon wrote: > Does this need to be package-private? Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:104: class Receiver extends BroadcastReceiver { On 2017/05/15 19:47:24, Simeon wrote: > Does this need to be package-private? Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:121: private static final boolean DEBUG = true; On 2017/05/15 19:47:24, Simeon wrote: > We might want to make this false in the final patch set, unless we decide that > the activity lifecycle logging is useful enough to keep turned on. Not that useful. Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/browser/android/... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:126: "com.google.android.apps.castshell.intent.extra.WEB_CONTENTS"; On 2017/05/15 19:47:24, Simeon wrote: > These are redeclared in CastWebContentsService and CastWebContentsActivity. How > about making these package-private, or even moving them to a package-private > Constants class? Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni File chromecast/chromecast.gni (right): https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni#n... chromecast/chromecast.gni:29: cast_android_use_service = false On 2017/05/15 19:47:24, Simeon wrote: > As for naming, how about display_web_contents_in_service? That's a bit more > clear about what the service is being used for. Done. https://codereview.chromium.org/2874943002/diff/1/chromecast/chromecast.gni#n... chromecast/chromecast.gni:29: cast_android_use_service = false On 2017/05/15 19:47:24, Simeon wrote: > Do we want to enable this on audio-only builds yet? Might as well I guess. The service-based approach is more stable on Things anyway on account of the GPU issues.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm % a couple nits https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:150: public void setOnComponentClosedHandler(OnComponentClosedHandler onComponentClosedHandler) { If we're only supporting one OnComponentClosedHandler and one OnKeyDownHandler, I'd rather we inject these in the constructor than set them with method calls later. Less mutating state to keep track of that way. https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:188: public static void onKeyDown(Context context, String instanceId, int keyCode) { I'd be interested in putting this logic in a different class, but this is fine for now. https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... File chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java (right): https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java:139: component.onKeyDown(mActivity, INSTANCE_ID, 42); Since this is a static method, we might want to invoke CastWebContentsComponent.onKeyDown() instead of component.onKeyDown() to make that clear.
thoren@chromium.org changed required reviewers: - slan@chromium.org
https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:150: public void setOnComponentClosedHandler(OnComponentClosedHandler onComponentClosedHandler) { On 2017/05/17 00:31:56, Simeon wrote: > If we're only supporting one OnComponentClosedHandler and one OnKeyDownHandler, > I'd rather we inject these in the constructor than set them with method calls > later. Less mutating state to keep track of that way. Done. https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:188: public static void onKeyDown(Context context, String instanceId, int keyCode) { On 2017/05/17 00:31:56, Simeon wrote: > I'd be interested in putting this logic in a different class, but this is fine > for now. Acknowledged. https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... File chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java (right): https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java:139: component.onKeyDown(mActivity, INSTANCE_ID, 42); On 2017/05/17 00:31:56, Simeon wrote: > Since this is a static method, we might want to invoke > CastWebContentsComponent.onKeyDown() instead of component.onKeyDown() to make > that clear. Done. I didn't even realized I did it that way here.
On 2017/05/17 18:20:37, thoren wrote: > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > File > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java > (right): > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:150: > public void setOnComponentClosedHandler(OnComponentClosedHandler > onComponentClosedHandler) { > On 2017/05/17 00:31:56, Simeon wrote: > > If we're only supporting one OnComponentClosedHandler and one > OnKeyDownHandler, > > I'd rather we inject these in the constructor than set them with method calls > > later. Less mutating state to keep track of that way. > > Done. > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:188: > public static void onKeyDown(Context context, String instanceId, int keyCode) { > On 2017/05/17 00:31:56, Simeon wrote: > > I'd be interested in putting this logic in a different class, but this is fine > > for now. > > Acknowledged. > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > File > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java > (right): > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java:139: > component.onKeyDown(mActivity, INSTANCE_ID, 42); > On 2017/05/17 00:31:56, Simeon wrote: > > Since this is a static method, we might want to invoke > > CastWebContentsComponent.onKeyDown() instead of component.onKeyDown() to make > > that clear. > > Done. I didn't even realized I did it that way here. Please put [Chromecast] at start of commit message. And fix that the commit message description of the gn arg is inconsistent with the code.
Description was changed from ========== Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'cast_android_use_service', which defaults to false for now. BUG=internal 36596922 ========== to ========== Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'display_web_contents_in_service', which defaults to is_cast_audio_only. BUG=internal 36596922 ==========
Description was changed from ========== Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'display_web_contents_in_service', which defaults to is_cast_audio_only. BUG=internal 36596922 ========== to ========== [Chromecast] Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'display_web_contents_in_service', which defaults to is_cast_audio_only. BUG=internal 36596922 ==========
On 2017/05/19 16:45:54, halliwell wrote: > On 2017/05/17 18:20:37, thoren wrote: > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > File > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java > > (right): > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:150: > > public void setOnComponentClosedHandler(OnComponentClosedHandler > > onComponentClosedHandler) { > > On 2017/05/17 00:31:56, Simeon wrote: > > > If we're only supporting one OnComponentClosedHandler and one > > OnKeyDownHandler, > > > I'd rather we inject these in the constructor than set them with method > calls > > > later. Less mutating state to keep track of that way. > > > > Done. > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:188: > > public static void onKeyDown(Context context, String instanceId, int keyCode) > { > > On 2017/05/17 00:31:56, Simeon wrote: > > > I'd be interested in putting this logic in a different class, but this is > fine > > > for now. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > File > > > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java > > (right): > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java:139: > > component.onKeyDown(mActivity, INSTANCE_ID, 42); > > On 2017/05/17 00:31:56, Simeon wrote: > > > Since this is a static method, we might want to invoke > > > CastWebContentsComponent.onKeyDown() instead of component.onKeyDown() to > make > > > that clear. > > > > Done. I didn't even realized I did it that way here. > > Please put [Chromecast] at start of commit message. And fix that the commit > message description of the gn arg is inconsistent with the code. Done. Hmm, apparently uploading a commit with a different description doesn't update the issue.
On 2017/05/19 16:50:25, thoren wrote: > On 2017/05/19 16:45:54, halliwell wrote: > > On 2017/05/17 18:20:37, thoren wrote: > > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > File > > > > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:150: > > > public void setOnComponentClosedHandler(OnComponentClosedHandler > > > onComponentClosedHandler) { > > > On 2017/05/17 00:31:56, Simeon wrote: > > > > If we're only supporting one OnComponentClosedHandler and one > > > OnKeyDownHandler, > > > > I'd rather we inject these in the constructor than set them with method > > calls > > > > later. Less mutating state to keep track of that way. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:188: > > > public static void onKeyDown(Context context, String instanceId, int > keyCode) > > { > > > On 2017/05/17 00:31:56, Simeon wrote: > > > > I'd be interested in putting this logic in a different class, but this is > > fine > > > > for now. > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > File > > > > > > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2874943002/diff/40001/chromecast/browser/andr... > > > > > > chromecast/browser/android/junit/src/org/chromium/chromecast/shell/CastWebContentsComponentTest.java:139: > > > component.onKeyDown(mActivity, INSTANCE_ID, 42); > > > On 2017/05/17 00:31:56, Simeon wrote: > > > > Since this is a static method, we might want to invoke > > > > CastWebContentsComponent.onKeyDown() instead of component.onKeyDown() to > > make > > > > that clear. > > > > > > Done. I didn't even realized I did it that way here. > > > > Please put [Chromecast] at start of commit message. And fix that the commit > > message description of the gn arg is inconsistent with the code. > > Done. Hmm, apparently uploading a commit with a different description doesn't > update the issue. Yeah, you have to edit in the web UI here.
https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java (left): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:95: if (!CastBrowserHelper.initializeBrowser(getApplicationContext())) { What's the story behind removing this? https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit, fix date https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:24: * On builds with CAST_ANDROID_USE_SERVICE set to false, it will use CastWebContentsActivity, CAST_ANDROID_USE_SERVICE got renamed? https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsService.java (right): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsService.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. nit, fix date
https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java (left): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:95: if (!CastBrowserHelper.initializeBrowser(getApplicationContext())) { On 2017/05/19 17:04:08, halliwell wrote: > What's the story behind removing this? Mostly because the TODO said to and it didn't seem to break the tests or the internal cast shell (this is called in there). Now that you mention it, though, I think it will break the upstream CastShell since it launches directly into this activity. https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java (right): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/19 17:04:08, halliwell wrote: > nit, fix date Done. https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsComponent.java:24: * On builds with CAST_ANDROID_USE_SERVICE set to false, it will use CastWebContentsActivity, On 2017/05/19 17:04:08, halliwell wrote: > CAST_ANDROID_USE_SERVICE got renamed? Done. https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsService.java (right): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsService.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/05/19 17:04:08, halliwell wrote: > nit, fix date Done.
Also please double check that internal build still works correctly on Fugu with this change. https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... File chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java (left): https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:95: if (!CastBrowserHelper.initializeBrowser(getApplicationContext())) { On 2017/05/19 17:19:49, thoren wrote: > On 2017/05/19 17:04:08, halliwell wrote: > > What's the story behind removing this? > > Mostly because the TODO said to and it didn't seem to break the tests or the > internal cast shell (this is called in there). Now that you mention it, though, > I think it will break the upstream CastShell since it launches directly into > this activity. I see. I think we still want upstream CastShell to work. Although you can't cast to it, you can launch pages from the command line (instructions here https://chromium.googlesource.com/chromium/src/+/master/docs/android_cast_bui... )
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thoren@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/05/19 18:14:06, halliwell wrote: > Also please double check that internal build still works correctly on Fugu with > this change. > > https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... > File > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java > (left): > > https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:95: > if (!CastBrowserHelper.initializeBrowser(getApplicationContext())) { > On 2017/05/19 17:19:49, thoren wrote: > > On 2017/05/19 17:04:08, halliwell wrote: > > > What's the story behind removing this? > > > > Mostly because the TODO said to and it didn't seem to break the tests or the > > internal cast shell (this is called in there). Now that you mention it, > though, > > I think it will break the upstream CastShell since it launches directly into > > this activity. > > I see. I think we still want upstream CastShell to work. Although you can't > cast to it, you can launch pages from the command line (instructions here > https://chromium.googlesource.com/chromium/src/+/master/docs/android_cast_bui... > ) Added it back.
On 2017/05/19 23:50:17, thoren wrote: > On 2017/05/19 18:14:06, halliwell wrote: > > Also please double check that internal build still works correctly on Fugu > with > > this change. > > > > > https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... > > File > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java > > (left): > > > > > https://codereview.chromium.org/2874943002/diff/60001/chromecast/browser/andr... > > > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastWebContentsActivity.java:95: > > if (!CastBrowserHelper.initializeBrowser(getApplicationContext())) { > > On 2017/05/19 17:19:49, thoren wrote: > > > On 2017/05/19 17:04:08, halliwell wrote: > > > > What's the story behind removing this? > > > > > > Mostly because the TODO said to and it didn't seem to break the tests or the > > > internal cast shell (this is called in there). Now that you mention it, > > though, > > > I think it will break the upstream CastShell since it launches directly into > > > this activity. > > > > I see. I think we still want upstream CastShell to work. Although you can't > > cast to it, you can launch pages from the command line (instructions here > > > https://chromium.googlesource.com/chromium/src/+/master/docs/android_cast_bui... > > ) > > Added it back. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by thoren@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sanfin@chromium.org Link to the patchset: https://codereview.chromium.org/2874943002/#ps120001 (title: "[Chromecast] Add service for "displaying" cast web contents.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by thoren@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495479599056230,
"parent_rev": "d570301027a991639be4e1c4ecfc7353e98ad012", "commit_rev":
"9540c6d6f520ad49921f0dc4e6b925324c623dfc"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'display_web_contents_in_service', which defaults to is_cast_audio_only. BUG=internal 36596922 ========== to ========== [Chromecast] Add service for "displaying" cast web contents. Audio-only Android builds should not start an activity, so this change adds the option of using a service. Rather than directly starting an activity to display cast apps, CastContentWindowAndroid uses CastWebContentsComponent to start and interact with whichever kind of component is appropriate for the build. This is controlled with the build flag 'display_web_contents_in_service', which defaults to is_cast_audio_only. BUG=internal 36596922 Review-Url: https://codereview.chromium.org/2874943002 Cr-Commit-Position: refs/heads/master@{#473670} Committed: https://chromium.googlesource.com/chromium/src/+/9540c6d6f520ad49921f0dc4e6b9... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9540c6d6f520ad49921f0dc4e6b9... |
