Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2946223002:
Replaces FullscreenActivity with SingleTabActivity. (Closed)
DescriptionReplaces FullscreenActivity with SingleTabActivity.
Motivation for this patch is to unlock more flexibility for UI of WebappActivity,
where in the follow-up patches I will be implementing Minimal-UI.
FullscreenActivity no longer forces lack of browser UI, so it got renamed to
SingleTabActivity. Code initializing the UI and enforcing fullscreen mode
has been pushed to subclasses (WebappActivity and
FullscreenWebContentsActivity). Created duplication will mostly disappear
in follow-up CLs.
SingleTabActivity is moved up from browser/webapps to browser/, as there
is nothing webapp specific about it and one subclass is already in browser/,
so browser/ is a better place for it.
In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding
"Open in Chrome") had been renamed to WEB_APP_MODE, as it better
described what it is for. This mode is only used by WebappActivity now,
FullscreenWebContentsActivity gets normal context menu, as it should.
FullScreenDelegateFactory disappears, as it's no longer needed.
Functionally this is a no-op, apart from some subtle changes to
FullscreenWebContentsActivity, namely removing "Open in Chrome"
from context menu (improvement?) and subtly changing the rules on
when browser controls can allowed to be auto-hidden when they're
technically shown, but as they're always hidden for this subclass it's a no-op.
BUG=604390
Review-Url: https://codereview.chromium.org/2946223002
Cr-Commit-Position: refs/heads/master@{#484453}
Committed: https://chromium.googlesource.com/chromium/src/+/b7ad7d0762780aca4bd872f006d67fb33be97554
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved some more fullscreeny-logic down to subclasses #
Total comments: 4
Patch Set 3 : Merge #Patch Set 4 : Removes the WebContentsObserver on reparenting in FullscreenWebContentsActivity #Messages
Total messages: 57 (44 generated)
The CQ bit was checked by piotrs@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by piotrs@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. BUG= ========== to ========== Replaces FullscreenActivity with SingleTabActivity. BUG=604390 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. BUG=604390 ========== to ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear In follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, names removing "Open in Chrome" from context menu (improvement) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're shown, but as they're always hidden it's a no-op. BUG=604390 ==========
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear In follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, names removing "Open in Chrome" from context menu (improvement) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're shown, but as they're always hidden it's a no-op. BUG=604390 ========== to ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear In follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, names removing "Open in Chrome" from context menu (improvement) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're shown, but as they're always hidden it's a no-op. BUG=604390 ==========
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
The CQ bit was checked by piotrs@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by piotrs@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...
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear In follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, names removing "Open in Chrome" from context menu (improvement) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're shown, but as they're always hidden it's a no-op. BUG=604390 ========== to ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear in follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, namely removing "Open in Chrome" from context menu (improvement?) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're technically shown, but as they're always hidden for this subclass it's a no-op. BUG=604390 ==========
piotrs@chromium.org changed reviewers: + dominickn@chromium.org, peconn@chromium.org
Hi Dominick, Peter, Can you take a look first as guardians of WebappActivity and FullscreenWebContentsActivity respectively? Cheers, Piotr
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java (right): https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:86: * Creates the {@link Tab} used by the FullScreenActivity. Nit: SingleTabActivity https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:151: // not apply to fullscreen content views. Should this logic be pushed into the fullscreen subclasses?
On 2017/06/23 21:31:06, piotrs wrote: > Hi Dominick, Peter, > > Can you take a look first as guardians of WebappActivity and > FullscreenWebContentsActivity respectively? > > Cheers, > Piotr FullscreenWebContentsActivity LGTM, although I'm not an OWNER of that file.
The CQ bit was checked by piotrs@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: This issue passed the CQ dry run.
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will disappear in follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, namely removing "Open in Chrome" from context menu (improvement?) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're technically shown, but as they're always hidden for this subclass it's a no-op. BUG=604390 ========== to ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI and enforcing fullscreen mode has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will mostly disappear in follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, namely removing "Open in Chrome" from context menu (improvement?) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're technically shown, but as they're always hidden for this subclass it's a no-op. BUG=604390 ==========
All done, thanks! https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java (right): https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:86: * Creates the {@link Tab} used by the FullScreenActivity. On 2017/06/26 03:59:05, dominickn wrote: > Nit: SingleTabActivity Done. https://codereview.chromium.org/2946223002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:151: // not apply to fullscreen content views. I pushed this logic to WebappActivity, and annotated it with a TODO to myself to simplify/clean-up once pre-rendering has been disabled (crbug.com/678332). FullscreenWebContentsActivity hasn't been using the onContentChanged() listener, as it overridden the createTab(), so it only gets WebContentsObserver that doesn't react to WebContents swapping - maintaining existing logic.
webapps lgtm. Minor note for the future: upload rebases separately if possible so that it's easier to see what's changed between patchsets. :)
piotrs@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, this is a small refactoring we briefly discussed. Can you take a look? Thanks!
lgtm w/ a few comments https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java (right): https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:50: new WebContentsObserver(tab.getWebContents()) { I think you'll want to keep a reference to this observer and remove it in the toggleFullscreenMode code below where we detach the tab and send it back (otherwise, you are leaving this attached forever and since this is a non-static class, it might already be holding onto this activity and preventing it from being gc'd). https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java (left): https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:50: @SuppressWarnings("unused") // Reference needed to prevent GC. From your move to the other file, I see that you no longer keep a strong ref. Is that because we now have WebContentsObserverProxy that holds onto strong refs and thus this is no longer required.
The CQ bit was checked by piotrs@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...
Thanks everyone! https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java (right): https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/FullscreenWebContentsActivity.java:50: new WebContentsObserver(tab.getWebContents()) { Good catch, done. Thanks for educating me here! https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java (left): https://codereview.chromium.org/2946223002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/SingleTabActivity.java:50: @SuppressWarnings("unused") // Reference needed to prevent GC. Firstly, this comment is a leftover from earlier times, as the reference was used here to destroy the observer. In FullscreenWebContentsActivity I didn't keep the reference as code structure as such that it would never be used and I didn't see how it could get GC'ed, it's only clear to me now that WebContentsObserverProxy came in recently, which explains this comment. FullscreenWebContentsActivity holds the reference again in response to your other comment in order to detach observer on reparenting, which I missed. Thanks for thoughtful comments!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by piotrs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peconn@chromium.org, dominickn@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2946223002/#ps160001 (title: "Removes the WebContentsObserver on reparenting in FullscreenWebContentsActivity")
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": 160001, "attempt_start_ts": 1499308192739180,
"parent_rev": "1fca937e8656e8b5c5842bf7e99eecfe89b18cad", "commit_rev":
"b7ad7d0762780aca4bd872f006d67fb33be97554"}
Message was sent while issue was closed.
Description was changed from ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI and enforcing fullscreen mode has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will mostly disappear in follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, namely removing "Open in Chrome" from context menu (improvement?) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're technically shown, but as they're always hidden for this subclass it's a no-op. BUG=604390 ========== to ========== Replaces FullscreenActivity with SingleTabActivity. Motivation for this patch is to unlock more flexibility for UI of WebappActivity, where in the follow-up patches I will be implementing Minimal-UI. FullscreenActivity no longer forces lack of browser UI, so it got renamed to SingleTabActivity. Code initializing the UI and enforcing fullscreen mode has been pushed to subclasses (WebappActivity and FullscreenWebContentsActivity). Created duplication will mostly disappear in follow-up CLs. SingleTabActivity is moved up from browser/webapps to browser/, as there is nothing webapp specific about it and one subclass is already in browser/, so browser/ is a better place for it. In ChromeContextMenuPopulator FULLSCREEN_TAB_MODE (i.e. adding "Open in Chrome") had been renamed to WEB_APP_MODE, as it better described what it is for. This mode is only used by WebappActivity now, FullscreenWebContentsActivity gets normal context menu, as it should. FullScreenDelegateFactory disappears, as it's no longer needed. Functionally this is a no-op, apart from some subtle changes to FullscreenWebContentsActivity, namely removing "Open in Chrome" from context menu (improvement?) and subtly changing the rules on when browser controls can allowed to be auto-hidden when they're technically shown, but as they're always hidden for this subclass it's a no-op. BUG=604390 Review-Url: https://codereview.chromium.org/2946223002 Cr-Commit-Position: refs/heads/master@{#484453} Committed: https://chromium.googlesource.com/chromium/src/+/b7ad7d0762780aca4bd872f006d6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) as https://chromium.googlesource.com/chromium/src/+/b7ad7d0762780aca4bd872f006d6...
Message was sent while issue was closed.
On 2017/07/06 03:25:23, commit-bot: I haz the power wrote: > Committed patchset #4 (id:160001) as > https://chromium.googlesource.com/chromium/src/+/b7ad7d0762780aca4bd872f006d6... Would anyone have any objections to renaming FullscreenWebContentsActivity to FullscreenActivity, now that FullScreenActivity is no longer used?
Message was sent while issue was closed.
SGTM. After a period of initial confusion it would be much cleaner.
Message was sent while issue was closed.
Also SGTM, that makes things clearer. :) |
