|
|
Created:
3 years, 8 months ago by PEConn Modified:
3 years, 7 months ago CC:
agrieve+watch_chromium.org, chromium-reviews, dominickn+watch_chromium.org, feature-media-reviews_chromium.org, pkotwicz+watch_chromium.org, zpeng+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove fullscreen web content to a new Activity.
Introduce the FullscreenMediaActivity and move Tabs to it when entering
fullscreen and away from it when leaving fullscreen.
BUG=709042
Review-Url: https://codereview.chromium.org/2807663002
Cr-Commit-Position: refs/heads/master@{#467705}
Committed: https://chromium.googlesource.com/chromium/src/+/b71e98cdf14f18cb967a73857826f6e8c568cea0
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed comments. #Patch Set 3 : Updated Manifest with Activity rename. #Patch Set 4 : Considered other ChromeActivity subclasses. #
Total comments: 9
Patch Set 5 : Added Test. #Patch Set 6 : Added Test. #Patch Set 7 : Fixed Hardware Acceleration Test. #Patch Set 8 : Got code into committable state. #
Total comments: 9
Patch Set 9 : Cleaned manifest and made Intent logic clearer. #Patch Set 10 : Fix test. #Patch Set 11 : Fix test. #Messages
Total messages: 52 (34 generated)
peconn@chromium.org changed reviewers: + bauerb@chromium.org, mlamouri@chromium.org, twellington@chromium.org, yusufo@chromium.org
Please take a look, this is somewhere between rough and polished! I'm around in MTV if you'd like to chat in person.
https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java:17: * An Activity used to display fullscreen web contents. …that is playing media? Or if we want this to be a generic activity for any fullscreen WebContents, we should find a name that is less media-specific. (Also, I would probably use `WebContents` here as a singular term, because there is a single object.) https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:291: * @param targetActivity The Activity to move the Tab to. It's the Activity Class, no? You could also wrap the class names in {@link […]}.
Exciting to see this :) Let me know if there are specific areas you want me to look at. It's not my area of expertise so I can't help much without digging. https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java:17: * An Activity used to display fullscreen web contents. On 2017/04/07 at 20:08:59, Bernhard Bauer wrote: > …that is playing media? Or if we want this to be a generic activity for any fullscreen WebContents, we should find a name that is less media-specific. > > (Also, I would probably use `WebContents` here as a singular term, because there is a single object.) We don't know if an activity is playing media while fullscreen until it is fullscreen so we will need to do this all the time.
https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java:31: mTab.attachAndFinishReparenting(this, createTabDelegateFactory(), params); I don't think we need to cache this here since it gets added to the model. https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1461: if (mIsPendingFullscreen != null) { see the finalizeCallback in detach.. That is a runnable that carries out any activity to activity specific logic at the end of tab reparenting. If you look up, the line: reparentingParams.finalizeTabReparenting(); runs that callback. I think this logic inside the if, is a good candidate for that. You can just pass it in moveToOtherWindow as a runnable and then we can keep detach-attach mostly UX flow agnostic.
Description was changed from ========== Move fullscreen web content to a new Activity. Introduce the FullscreenMediaActivity and move Tabs to it when entering fullscreen and away from it when leaving fullscreen. BUG=709042 ========== to ========== Move fullscreen web content to a new Activity. Introduce the FullscreenMediaActivity and move Tabs to it when entering fullscreen and away from it when leaving fullscreen. BUG=709042 ==========
https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java:17: * An Activity used to display fullscreen web contents. On 2017/04/07 20:08:59, Bernhard Bauer wrote: > …that is playing media? Or if we want this to be a generic activity for any > fullscreen WebContents, we should find a name that is less media-specific. > > (Also, I would probably use `WebContents` here as a singular term, because there > is a single object.) Renamed to FullscreenWebContentsActivity - I would've preferred FullscreenActivity, but it (or more rather FullScreenActivity) was already taken. https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/media/FullscreenMediaActivity.java:31: mTab.attachAndFinishReparenting(this, createTabDelegateFactory(), params); On 2017/04/10 17:08:49, Yusuf wrote: > I don't think we need to cache this here since it gets added to the model. Oh yes sorry - I think this was cruft from an earlier version. I'm annoyed I missed this because I got caught up on a bug because FullScreenActivity was doing the same thing (caching the Tab instead of just getting it from the TabModel). https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:291: * @param targetActivity The Activity to move the Tab to. On 2017/04/07 20:08:59, Bernhard Bauer wrote: > It's the Activity Class, no? You could also wrap the class names in {@link […]}. Done. https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2807663002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1461: if (mIsPendingFullscreen != null) { On 2017/04/10 17:08:49, Yusuf wrote: > see the finalizeCallback in detach.. That is a runnable that carries out any > activity to activity specific logic at the end of tab reparenting. If you look > up, the line: > > reparentingParams.finalizeTabReparenting(); > > runs that callback. I think this logic inside the if, is a good candidate for > that. You can just pass it in moveToOtherWindow as a runnable and then we can > keep detach-attach mostly UX flow agnostic. Done.
The CQ bit was checked by peconn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by peconn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
per offline discussion this lgtm for tabbed activity tab reparenting logic. It will break for custom tab activities and we should make a call about that.
I've added some complexity to fall back to the old fullscreen behaviour for CCTs and Webapps. Could I get a look over from: mlamouri for fullscreen stuff twellington for multiwindow stuff bauerb for general
https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java (right): https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:50: "org.chromium.chrome.browser.multiwindow.FromActivity"; Rather than introducing a new intent flag, I think you can reuse IntentHandler#EXTRA_PARENT_COMPONENT. That's what we use for history, bookmarks, and downloads to make sure we open items in the correct parent activity. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:307: MultiWindowUtils.onMultiInstanceModeStarted(); "Multi-instance" mode has historically referred to two ChromeTabbedActivity instances. ChromeTabbedActivity#onMultiInstanceModeStarted() relies on this definition for its tab model merging behavior (when exiting multi-window mode, we kill one of the ChromeTabbedActivity instances and merge its tabs into the other instance). It seems that the definition of "multi-instance" is changing and that the tab may be moving to a non-ChromeTabbedActivity? If so, calling this method here probably causes issues with tab model merging and possibly reintroduces crbug.com/657418. Maybe it's a simple fix, and we can just conditionally call onMultiInstanceModeStarted(): if (activity instanceof ChromeTabbedActivity) MultiWindowUtils.onMultiInstanceModeStarted() It'd also be great to update the JavaDoc for onMultiInstanceModeStarted to be specific about what multi-instance is now that we may be moving tabs to other types of activities. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:316: public static Class<? extends ChromeActivity> getSenderActivity(Activity activity) { Typically we use IntentUtils#safeGet*() to extract extras from intents. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:244: MultiWindowUtils.moveTabToActivity( Does the fullscreen activity sit on top of the parent ChromeTabbedActivity (part of the same task)? If so, please disregard the rest of this comment. If not, we probably want to introduce logic to determine if the task for the parent activity is still running before moving the tab. Previously when moving tabs between windows, if the activity for the other window didn't already exist it was always correct to create it (E.g. when the user selects "Open in Chrome" in a CCT or "Move to other window" from the app menu in Chrome). If the fullscreen activity is a separate task you'll need to handle this scenario: 1. Go into multi-window mode, create ChromeTabbedActivity2 2. Fullscreen a tab in ChromeTabbedActivity2 creating a separate task for FullscreenWebContentsActivity 3. Kill ChromeTabbedActivity2, exit multi-window mode 4. Exit fullscreen on the tab -- at this point, it's invalid to recreate ChromeTabbedActivtiy2, so we want the tab to get reparented into ChromeTabbedActivity.
https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:244: MultiWindowUtils.moveTabToActivity( On 2017/04/12 22:29:11, Theresa wrote: > Does the fullscreen activity sit on top of the parent ChromeTabbedActivity (part > of the same task)? If so, please disregard the rest of this comment. > > If not, we probably want to introduce logic to determine if the task for the > parent activity is still running before moving the tab. Previously when moving > tabs between windows, if the activity for the other window didn't already exist > it was always correct to create it (E.g. when the user selects "Open in Chrome" > in a CCT or "Move to other window" from the app menu in Chrome). > > If the fullscreen activity is a separate task you'll need to handle this > scenario: > > 1. Go into multi-window mode, create ChromeTabbedActivity2 > 2. Fullscreen a tab in ChromeTabbedActivity2 creating a separate task for > FullscreenWebContentsActivity > 3. Kill ChromeTabbedActivity2, exit multi-window mode > 4. Exit fullscreen on the tab -- at this point, it's invalid to recreate > ChromeTabbedActivtiy2, so we want the tab to get reparented into > ChromeTabbedActivity. I was planning on having the fullscreen Activity be part of the same task - it seems to make more sense. Will Android ever kill background Activities in the same task as the foreground one? Would there be any advantages to having the fullscreen Activity *not* be in the same task?
https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java (right): https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/TabWebContentsDelegateAndroid.java:244: MultiWindowUtils.moveTabToActivity( On 2017/04/13 14:45:57, PEConn wrote: > On 2017/04/12 22:29:11, Theresa wrote: > > Does the fullscreen activity sit on top of the parent ChromeTabbedActivity > (part > > of the same task)? If so, please disregard the rest of this comment. > > > > If not, we probably want to introduce logic to determine if the task for the > > parent activity is still running before moving the tab. Previously when moving > > tabs between windows, if the activity for the other window didn't already > exist > > it was always correct to create it (E.g. when the user selects "Open in > Chrome" > > in a CCT or "Move to other window" from the app menu in Chrome). > > > > If the fullscreen activity is a separate task you'll need to handle this > > scenario: > > > > 1. Go into multi-window mode, create ChromeTabbedActivity2 > > 2. Fullscreen a tab in ChromeTabbedActivity2 creating a separate task for > > FullscreenWebContentsActivity > > 3. Kill ChromeTabbedActivity2, exit multi-window mode > > 4. Exit fullscreen on the tab -- at this point, it's invalid to recreate > > ChromeTabbedActivtiy2, so we want the tab to get reparented into > > ChromeTabbedActivity. > > I was planning on having the fullscreen Activity be part of the same task - it > seems to make more sense. Will Android ever kill background Activities in the > same task as the foreground one? > > Would there be any advantages to having the fullscreen Activity *not* be in the > same task? I don't think Android will kill the background activity. It will be stopped and have a saved instance state. In this case it doesn't matter, because the scenario laid out requires the user to remove the task from Android Recents. Outside of PIP, I don't see any advantages of making the fullscreen activity a separate task. Once fullscreen activity is PIPed, though, does it become a separate task? If not, what does tapping on the Chrome launcher icon do?
The CQ bit was checked by peconn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by peconn@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
So I've made a few changes: - Undid all of my changes to MultiwindowUtils. - Moved the logic into a static method on FullscreenWebContentsActivity. - Added a Test. - Disabled this feature by default. twellington@ could you please have a look over FullscreenWebContentsActivity? I was hoping to commit as is, but will be making it more robust to come. bauerb@ - Could you have a look over the general thing. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java (right): https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:50: "org.chromium.chrome.browser.multiwindow.FromActivity"; On 2017/04/12 22:29:11, Theresa wrote: > Rather than introducing a new intent flag, I think you can reuse > IntentHandler#EXTRA_PARENT_COMPONENT. That's what we use for history, bookmarks, > and downloads to make sure we open items in the correct parent activity. Done. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:307: MultiWindowUtils.onMultiInstanceModeStarted(); On 2017/04/12 22:29:11, Theresa wrote: > "Multi-instance" mode has historically referred to two ChromeTabbedActivity > instances. ChromeTabbedActivity#onMultiInstanceModeStarted() relies on this > definition for its tab model merging behavior (when exiting multi-window mode, > we kill one of the ChromeTabbedActivity instances and merge its tabs into the > other instance). > > It seems that the definition of "multi-instance" is changing and that the tab > may be moving to a non-ChromeTabbedActivity? If so, calling this method here > probably causes issues with tab model merging and possibly reintroduces > crbug.com/657418. > > Maybe it's a simple fix, and we can just conditionally call > onMultiInstanceModeStarted(): > > if (activity instanceof ChromeTabbedActivity) > MultiWindowUtils.onMultiInstanceModeStarted() > > > It'd also be great to update the JavaDoc for onMultiInstanceModeStarted to be > specific about what multi-instance is now that we may be moving tabs to other > types of activities. Acknowledged. https://codereview.chromium.org/2807663002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtils.java:316: public static Class<? extends ChromeActivity> getSenderActivity(Activity activity) { On 2017/04/12 22:29:11, Theresa wrote: > Typically we use IntentUtils#safeGet*() to extract extras from intents. Done.
This seems much simpler. Looks good overall. https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:360: <activity android:name="org.chromium.chrome.browser.FullscreenWebContentsActivity" nit: reorder so that ChromeTabbedActivity and ChromeTabbedActivity2 aren't split up. Maybe put this below MultiInstanceChromeTabbedActivity? https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:366: <!-- TODO(peconn): Check Theme. --> TabbedModeTheme uses a 9-patch with a toolbar as the window background (set during app startup before any Views are drawn). This activity should probably use MainTheme or FullscreenWhite for now. https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java (right): https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:86: intent.addFlags(Intent.FLAG_ACTIVITY_MULTIPLE_TASK); Why do we need multiple task? Can there be multiple fullscreen activities at once? https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:95: intent.setClass(activity, ChromeTabbedActivity.class); We should probably send this back through ChromeLauncherActivity if we can't get the parent component.
https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:360: <activity android:name="org.chromium.chrome.browser.FullscreenWebContentsActivity" On 2017/04/26 18:32:11, Theresa wrote: > nit: reorder so that ChromeTabbedActivity and ChromeTabbedActivity2 aren't split > up. Maybe put this below MultiInstanceChromeTabbedActivity? Done. https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/An... chrome/android/java/AndroidManifest.xml:366: <!-- TODO(peconn): Check Theme. --> On 2017/04/26 18:32:11, Theresa wrote: > TabbedModeTheme uses a 9-patch with a toolbar as the window background (set > during app startup before any Views are drawn). This activity should probably > use MainTheme or FullscreenWhite for now. Done. https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java (right): https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:86: intent.addFlags(Intent.FLAG_ACTIVITY_MULTIPLE_TASK); On 2017/04/26 18:32:11, Theresa wrote: > Why do we need multiple task? Can there be multiple fullscreen activities at > once? I was thinking of the multiwindow case - we want to allow both ChromeTabbedActivity's to show fullscreen content. https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:95: intent.setClass(activity, ChromeTabbedActivity.class); On 2017/04/26 18:32:11, Theresa wrote: > We should probably send this back through ChromeLauncherActivity if we can't get > the parent component. Done.
The CQ bit was checked by peconn@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by peconn@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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by peconn@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...
Nice! LGTM if Theresa is happy.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java (right): https://codereview.chromium.org/2807663002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:86: intent.addFlags(Intent.FLAG_ACTIVITY_MULTIPLE_TASK); On 2017/04/27 08:53:11, PEConn wrote: > On 2017/04/26 18:32:11, Theresa wrote: > > Why do we need multiple task? Can there be multiple fullscreen activities at > > once? > > I was thinking of the multiwindow case - we want to allow both > ChromeTabbedActivity's to show fullscreen content. I think that this flag will allow an unlimited number of tasks to be created, when we want at most two. I think it's fine to leave for now, but we should do some testing around multi-window and fullscreen activities.
The CQ bit was checked by peconn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org Link to the patchset: https://codereview.chromium.org/2807663002/#ps200001 (title: "Fix test.")
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": 200001, "attempt_start_ts": 1493308093568080, "parent_rev": "e851ad470036997a9a43b0ee2710442f1b5c2096", "commit_rev": "b71e98cdf14f18cb967a73857826f6e8c568cea0"}
Message was sent while issue was closed.
Description was changed from ========== Move fullscreen web content to a new Activity. Introduce the FullscreenMediaActivity and move Tabs to it when entering fullscreen and away from it when leaving fullscreen. BUG=709042 ========== to ========== Move fullscreen web content to a new Activity. Introduce the FullscreenMediaActivity and move Tabs to it when entering fullscreen and away from it when leaving fullscreen. BUG=709042 Review-Url: https://codereview.chromium.org/2807663002 Cr-Commit-Position: refs/heads/master@{#467705} Committed: https://chromium.googlesource.com/chromium/src/+/b71e98cdf14f18cb967a73857826... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/b71e98cdf14f18cb967a73857826...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2849533004/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit(https://goo.gl/kROfz5) identified CL at revision 467705 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb.... |