Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2636833003:
Support "display": "fullscreen" for sites added to the home screen. (Closed)
DescriptionImplement the immersive mode for the FullscreenActivity on Android K+.
A web app in immersive mode will hide the Android system navigation and
status bars. It will be used to support fullscreen mode for sites added
to the home screen.
Currently even the "display" mode in web app manifest was configured to
fullscreen, it's still launched to a standalone mode.
BUG=581522, 680385
Review-Url: https://codereview.chromium.org/2636833003
Cr-Commit-Position: refs/heads/master@{#451533}
Committed: https://chromium.googlesource.com/chromium/src/+/f0f67bb9cea082b6e8b1fdf0e392e2fdea524eda
Patch Set 1 #Patch Set 2 : Fix incorrect indentation. #
Total comments: 10
Patch Set 3 : refactoring code for performance #
Total comments: 12
Patch Set 4 : fix #
Total comments: 16
Patch Set 5 : Fix comments from Ted #Patch Set 6 : move all change to WebappActivity #
Total comments: 4
Patch Set 7 : fix indent #Patch Set 8 : exit Fullscreen first #Patch Set 9 : disable html5 fullscreen #
Total comments: 2
Messages
Total messages: 32 (12 generated)
dominickn@chromium.org changed reviewers: + dominickn@chromium.org
Looks pretty good, thanks! I have mostly minor comments and a couple of questions: 1. What happens if you're on a site in display: fullscreen, then you click on a link to outside that site? For display: standalone, it pops down a little URL bar (to show that you've navigated "outside" the PWA" - we need to check that this still works right. 2. Can you make the CL description more descriptive? You can describe how currently display: fullscreen is downgraded to display: standalone, which shows a status bar at the top and the buttons at the bottom. Immersive fullscreen allows none of this UI to be shown. Thanks! https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:216: // Register a SystemUiVisibilityChangeListener to restore immersive mode. Can these comments be a little more descriptive? Something like: // When we enter immersive mode for the first time, register a SystemUiVisibilityChangeListener that restores immersive mode. This is necessary because user actions like focusing a keyboard will break out of immersive. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:225: // Set Immersive mode on demand. Nit: add a newline above this comment, and explain why you have a short delay to enter https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:96: // WebDisplayMode.Fullscreen. See crbug.com/581522 Nit: remove this TODO. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: WebDisplayMode.Standalone); Nit: this should be: getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode( mWebappInfo.displayMode());
Description was changed from ========== Support "display": "fullscreen" for sites added to the home screen. Make the fullscreen really fullscreen. BUG=581522 ========== to ========== Implement the immersive mode for the FullscreenActivity on Android K+. Setting webapp in this mode will hide the Android system navigation and status bars. So it's used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522 ==========
Thanks for the review, Dominick. On 2017/01/16 22:49:01, dominickn wrote: > Looks pretty good, thanks! I have mostly minor comments and a couple of > questions: > > 1. What happens if you're on a site in display: fullscreen, then you click on a > link to outside that site? For display: standalone, it pops down a little URL > bar (to show that you've navigated "outside" the PWA" - we need to check that > this still works right. It will do the same as standalone mode. Taking a screenshot as proof. https://drive.google.com/a/chromium.org/file/d/0B7BQekACXW_VanNIUVJfN2tLQlU/v... > > 2. Can you make the CL description more descriptive? You can describe how > currently display: fullscreen is downgraded to display: standalone, which shows > a status bar at the top and the buttons at the bottom. Immersive fullscreen > allows none of this UI to be shown. Added more details in the description. > > Thanks! > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java > (right): > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:216: > // Register a SystemUiVisibilityChangeListener to restore immersive mode. > Can these comments be a little more descriptive? Something like: > > // When we enter immersive mode for the first time, register a > SystemUiVisibilityChangeListener that restores immersive mode. This is necessary > because user actions like focusing a keyboard will break out of immersive. > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:225: > // Set Immersive mode on demand. > Nit: add a newline above this comment, and explain why you have a short delay to > enter > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java > (right): > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:96: > // WebDisplayMode.Fullscreen. See crbug.com/581522 > Nit: remove this TODO. > > https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: > WebDisplayMode.Standalone); > Nit: this should be: > > getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode( > mWebappInfo.displayMode());
Made some code refactoring to avoid duplicate System UI flags checking and setting. And replied all the comments. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:225: // Set Immersive mode on demand. On 2017/01/16 22:49:01, dominickn wrote: > Nit: add a newline above this comment, and explain why you have a short delay to > enter Actually I just follow the sample codes created by Google and copy the 300 ms as a friendly delay. IMO, the main purpose is just to be asynchronous. Setting a short delay is not required but could tolerate some annoying system faults. https://android.googlesource.com/platform/development/+/e7a6ab4/samples/devby... https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: WebDisplayMode.Standalone); On 2017/01/16 22:49:01, dominickn wrote: > Nit: this should be: > > getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode( > mWebappInfo.displayMode()); Thanks for the tips. So what will happen if the display mode was MinimalUi? Is there any logic to fallback to standalone safely?
Couple of minor style related nits, then we can send it out. :) https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:225: // Set Immersive mode on demand. On 2017/01/18 05:09:40, Leo wrote: > On 2017/01/16 22:49:01, dominickn wrote: > > Nit: add a newline above this comment, and explain why you have a short delay > to > > enter > Actually I just follow the sample codes created by Google and copy the 300 ms as > a friendly delay. > IMO, the main purpose is just to be asynchronous. Setting a short delay is not > required but could tolerate some annoying system faults. > > https://android.googlesource.com/platform/development/+/e7a6ab4/samples/devby... Ah, I see, that makes sense. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: WebDisplayMode.Standalone); On 2017/01/18 05:09:40, Leo wrote: > On 2017/01/16 22:49:01, dominickn wrote: > > Nit: this should be: > > > > getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode( > > mWebappInfo.displayMode()); > > Thanks for the tips. So what will happen if the display mode was MinimalUi? > Is there any logic to fallback to standalone safely? Currently, minimal-ui does not open a WebappActivity (it just opens an ordinary browser tab), so we don't need to worry about a fallback. When we move to implementing minimal-ui, we'll add the appropriate logic in here. :) https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:211: if (!supportsImmersiveMode()) { Minor nit: the style guide lets you do this for short conditionals: if (!supportsImmersiveMode()) return; https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:241: // Set immersive mode asynchronously. Minor nit: you can probably remove this comment because it just repeats the method name. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:247: if (mSetImmersiveRunnable == null) { Minor style nit: you can inline the return here. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:283: ** @return true if this device supports immersive mode functionality. Minor style nit: single * after the first line of the comment. /** * @return etc. */ https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:95: // TODO(dominickn): send the web app into fullscreen if mDisplayMode is Please delete this comment - it's not needed any more. :) https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: // Add Fullscreen support and the other modes will fallback to Standalone. Nit: the comment here probably isn't necessary (the code is very descriptive). Also, in the future there will also be display:minimal-ui. That currently falls back to a browser tab.
One thing I forgot - please wrap the description to a width of 80 characters. You can use git cl description to do this :)
Description was changed from ========== Implement the immersive mode for the FullscreenActivity on Android K+. Setting webapp in this mode will hide the Android system navigation and status bars. So it's used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522 ========== to ========== Implement the immersive mode for the FullscreenActivity on Android K+. A web app in immersive mode will hide the Android system navigation and status bars. It will be used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522 ==========
The CQ bit was checked by googleo@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.
Thanks for the review. All comments were fixed and passed the passed the CQ dry run. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:96: // WebDisplayMode.Fullscreen. See crbug.com/581522 On 2017/01/16 22:49:01, dominickn wrote: > Nit: remove this TODO. Acknowledged. https://codereview.chromium.org/2636833003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: WebDisplayMode.Standalone); On 2017/01/18 05:56:17, dominickn wrote: > On 2017/01/18 05:09:40, Leo wrote: > > On 2017/01/16 22:49:01, dominickn wrote: > > > Nit: this should be: > > > > > > getActivityTab().getTabWebContentsDelegateAndroid().setDisplayMode( > > > mWebappInfo.displayMode()); > > > > Thanks for the tips. So what will happen if the display mode was MinimalUi? > > Is there any logic to fallback to standalone safely? > > Currently, minimal-ui does not open a WebappActivity (it just opens an ordinary > browser tab), so we don't need to worry about a fallback. When we move to > implementing minimal-ui, we'll add the appropriate logic in here. :) Thanks for the explanation. Good to know that. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:211: if (!supportsImmersiveMode()) { On 2017/01/18 05:56:17, dominickn wrote: > Minor nit: the style guide lets you do this for short conditionals: > > if (!supportsImmersiveMode()) return; Done. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:241: // Set immersive mode asynchronously. On 2017/01/18 05:56:17, dominickn wrote: > Minor nit: you can probably remove this comment because it just repeats the > method name. Done. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:247: if (mSetImmersiveRunnable == null) { On 2017/01/18 05:56:17, dominickn wrote: > Minor style nit: you can inline the return here. Done. And remove unused decor. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:283: ** @return true if this device supports immersive mode functionality. On 2017/01/18 05:56:17, dominickn wrote: > Minor style nit: single * after the first line of the comment. > > /** > * @return etc. > */ Thanks for the correction. I found this method is only called once, so merged code into where it was used. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:95: // TODO(dominickn): send the web app into fullscreen if mDisplayMode is On 2017/01/18 05:56:17, dominickn wrote: > Please delete this comment - it's not needed any more. :) Acknowledged. https://codereview.chromium.org/2636833003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:121: // Add Fullscreen support and the other modes will fallback to Standalone. On 2017/01/18 05:56:17, dominickn wrote: > Nit: the comment here probably isn't necessary (the code is very descriptive). > Also, in the future there will also be display:minimal-ui. That currently falls > back to a browser tab. Acknowledged.
googleo@chromium.org changed reviewers: + tedchoc@chromium.org
Hi Ted, Please take a look, this Cl implement the immersive mode for FullscreenActivity, which will be used for "fullscreen" display mode for PWAs. Also let me know if you have any advice for tests? Thanks
https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:51: private static final int ENTER_IMMERSIVE_MODE_DELAY_MILLIS = 300; any reason all of this logic isn't in WebappActivity? Doesn't "seem" to be used by this class at all. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:202: if (hasFocus && (mSetImmersiveRunnable != null)) { the () around the null check aren't needed https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:203: enterImmersiveMode(getWindow().getDecorView()); looks like both call sites just pass in getWindow().getDecorView(), any reason to not just do that internally in the method? https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:231: if (!mEnteredImmersive) { can we just put this block in the mSetImmersiveRunnable == null block above? Seems like we could use just one variable as a signal for initial pass (i.e. delete mEnteredImmersive). https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:236: asyncSetImmersive(RESTORE_IMMERSIVE_MODE_DELAY_MILLIS); 3 seconds seems like a very long time. Why does it need to be that high?
Thanks for the review, Ted. Please take a look again. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:51: private static final int ENTER_IMMERSIVE_MODE_DELAY_MILLIS = 300; On 2017/01/20 05:47:07, Ted C wrote: > any reason all of this logic isn't in WebappActivity? Doesn't "seem" to be used > by this class at all. Original changes was in WebappActivity. Somehow I found since its parent's name is FullScreenActivity which doesn't support really full-screen mode. So maybe it's better to implement the "full-screen" mode in the parent class, then all the subclass could easily support "full-screen" mode. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:202: if (hasFocus && (mSetImmersiveRunnable != null)) { On 2017/01/20 05:47:07, Ted C wrote: > the () around the null check aren't needed Thanks for the catch. Use asyncSetImmersive here instead of enterImmersiveMode. So it no-ops before mSetImmersiveRunnable was not initialized. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:203: enterImmersiveMode(getWindow().getDecorView()); On 2017/01/20 05:47:07, Ted C wrote: > looks like both call sites just pass in getWindow().getDecorView(), any reason > to not just do that internally in the method? Sure, getWindow().getDecorView() can be used internally. My intent is try to make sure the user understand that enterImmersiveMode should be called at a proper time (after Decor View was ready). I can remove the arg and add more comments instead. WDYT? https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:231: if (!mEnteredImmersive) { On 2017/01/20 05:47:07, Ted C wrote: > can we just put this block in the mSetImmersiveRunnable == null block above? > Seems like we could use just one variable as a signal for initial pass (i.e. > delete mEnteredImmersive). Thanks for the tips, Done. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:236: asyncSetImmersive(RESTORE_IMMERSIVE_MODE_DELAY_MILLIS); On 2017/01/20 05:47:07, Ted C wrote: > 3 seconds seems like a very long time. Why does it need to be that high? In most scenarios, immersive mode will be restored after users triggers some event to show system status bar or navigation bar. The delay here is used to give user some time to make the next choice. Here is a demo video for 3 seconds delay. https://drive.google.com/a/chromium.org/file/d/0B7BQekACXW_VRGVkanQzS09ZN1U/v... What's your suggestion for the delay setting?
https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:51: private static final int ENTER_IMMERSIVE_MODE_DELAY_MILLIS = 300; On 2017/01/23 01:14:27, Leo wrote: > On 2017/01/20 05:47:07, Ted C wrote: > > any reason all of this logic isn't in WebappActivity? Doesn't "seem" to be > used > > by this class at all. > > Original changes was in WebappActivity. Somehow I found since its parent's name > is FullScreenActivity which doesn't support really full-screen mode. So maybe > it's better to implement the "full-screen" mode in the parent class, then all > the subclass could easily support "full-screen" mode. Fullscreen in the name of this activity means that it doesn't show an omnibox. So, full-screen mode here is very Webapp specific. Also as a general rule, it is best to keep the code isolated to the class that needs it and generalize it later. It isn't hard to move in the future, but for now it makes it harder to read being split between two classes. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:203: enterImmersiveMode(getWindow().getDecorView()); On 2017/01/23 01:14:27, Leo wrote: > On 2017/01/20 05:47:07, Ted C wrote: > > looks like both call sites just pass in getWindow().getDecorView(), any reason > > to not just do that internally in the method? > > Sure, getWindow().getDecorView() can be used internally. My intent is try to > make sure the user understand that enterImmersiveMode should be called at a > proper time (after Decor View was ready). > > I can remove the arg and add more comments instead. WDYT? I would just remove the variable. getDecorView() will build the view lazily so I think it would just confuse api consumers if you add documentation about decor view lifetime. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:236: asyncSetImmersive(RESTORE_IMMERSIVE_MODE_DELAY_MILLIS); On 2017/01/23 01:14:27, Leo wrote: > On 2017/01/20 05:47:07, Ted C wrote: > > 3 seconds seems like a very long time. Why does it need to be that high? > > In most scenarios, immersive mode will be restored after users triggers some > event to show system status bar or navigation bar. The delay here is used to > give user some time to make the next choice. > > Here is a demo video for 3 seconds delay. > https://drive.google.com/a/chromium.org/file/d/0B7BQekACXW_VRGVkanQzS09ZN1U/v... > > What's your suggestion for the delay setting? That video looks good to me. Good to know the context.
Thanks for the review Ted! googleo@ is OOO for a couple of weeks now FYI, so he'll pick this up when he comes back.
Sorry for the delay. Thanks Ted. All your comments were fixed. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java (right): https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:51: private static final int ENTER_IMMERSIVE_MODE_DELAY_MILLIS = 300; On 2017/01/23 19:01:08, Ted C wrote: > On 2017/01/23 01:14:27, Leo wrote: > > On 2017/01/20 05:47:07, Ted C wrote: > > > any reason all of this logic isn't in WebappActivity? Doesn't "seem" to be > > used > > > by this class at all. > > > > Original changes was in WebappActivity. Somehow I found since its parent's > name > > is FullScreenActivity which doesn't support really full-screen mode. So maybe > > it's better to implement the "full-screen" mode in the parent class, then all > > the subclass could easily support "full-screen" mode. > > Fullscreen in the name of this activity means that it doesn't show an omnibox. > So, full-screen mode here is very Webapp specific. > > Also as a general rule, it is best to keep the code isolated to the class that > needs it and generalize it later. It isn't hard to move in the future, but for > now it makes it harder to read being split between two classes. Thanks for the advice. Moved all changes to WebappActivity. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:203: enterImmersiveMode(getWindow().getDecorView()); On 2017/01/23 19:01:08, Ted C wrote: > On 2017/01/23 01:14:27, Leo wrote: > > On 2017/01/20 05:47:07, Ted C wrote: > > > looks like both call sites just pass in getWindow().getDecorView(), any > reason > > > to not just do that internally in the method? > > > > Sure, getWindow().getDecorView() can be used internally. My intent is try to > > make sure the user understand that enterImmersiveMode should be called at a > > proper time (after Decor View was ready). > > > > I can remove the arg and add more comments instead. WDYT? > > I would just remove the variable. getDecorView() will build the view lazily so > I think it would just confuse api consumers if you add documentation about decor > view lifetime. Done. https://codereview.chromium.org/2636833003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/webapps/FullScreenActivity.java:236: asyncSetImmersive(RESTORE_IMMERSIVE_MODE_DELAY_MILLIS); On 2017/01/23 19:01:08, Ted C wrote: > On 2017/01/23 01:14:27, Leo wrote: > > On 2017/01/20 05:47:07, Ted C wrote: > > > 3 seconds seems like a very long time. Why does it need to be that high? > > > > In most scenarios, immersive mode will be restored after users triggers some > > event to show system status bar or navigation bar. The delay here is used to > > give user some time to make the next choice. > > > > Here is a demo video for 3 seconds delay. > > > https://drive.google.com/a/chromium.org/file/d/0B7BQekACXW_VRGVkanQzS09ZN1U/v... > > > > What's your suggestion for the delay setting? > > That video looks good to me. Good to know the context. Acknowledged.
https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:63: | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN I think this line and the following need to be indented +4 more https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:235: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return; any reason why this is the case? our other fullscreen stuff supports all versions of android (albeit using non system visibility to do it) Also, how does this feature behave with html5 fullscreen? If you have a webapp that points to something that uses those features, will FullscreenHtmlApiHandler start clobbering these bits? Do we need to do anything to prevent them from stomping on each others toes? How about fullscreen video? Is that a different path?
Sorry for the delay Ted. It took me some time to dig deeper and build proper demos for understanding these underlying features you mentioned. After more testings, I created a doc for the different results from these possiable use cases. https://docs.google.com/a/chromium.org/document/d/189N6GboVg09Fwm7Gmw3rOyGj4p... Please take a look. Hope it makes sense. https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:63: | View.SYSTEM_UI_FLAG_LAYOUT_FULLSCREEN On 2017/02/07 18:35:17, Ted C wrote: > I think this line and the following need to be indented +4 more Acknowledged. https://codereview.chromium.org/2636833003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:235: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.KITKAT) return; On 2017/02/07 18:35:17, Ted C wrote: > any reason why this is the case? our other fullscreen stuff supports all > versions of android (albeit using non system visibility to do it) I have another CL to add the support for pre-K versions. The reason I didn't add them to this CL is to keep this CL simple and safe. AFAIK, old devices may have very different Navi buttons (soft or physical) and UI flags settings, which requires much more cases or scenarios to be tested. So hopefully we introduce that complexity in another single CL. > > Also, how does this feature behave with html5 fullscreen? If you have a webapp > that points to something that uses those features, will FullscreenHtmlApiHandler > start clobbering these bits? Thanks for your insight, I didn't realize HTML5 Fullscreen implementation sets the same bits until you pointed it out. It took my some time to build the proper demos so I can understand these features' details. > > Do we need to do anything to prevent them from stomping on each others toes? > How about fullscreen video? Is that a different path? > In my testings, some use cases in PWA did cause some UX issues. I wrote them down in the docs below. https://docs.google.com/a/chromium.org/document/d/189N6GboVg09Fwm7Gmw3rOyGj4p... And seems some issue has existed from PWA standalone mode. Looks we need a code-refact to unify all fullscreen api.
Thanks for your great suggestions Ted, Some changes was made blew. 1, Add an exitfullscreen step in handleBackPressed for WebappActivity. 2, Disable HTML5 fullscreen once Webapp is in fullscreen set by site owner.
Description was changed from ========== Implement the immersive mode for the FullscreenActivity on Android K+. A web app in immersive mode will hide the Android system navigation and status bars. It will be used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522 ========== to ========== Implement the immersive mode for the FullscreenActivity on Android K+. A web app in immersive mode will hide the Android system navigation and status bars. It will be used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522,680385 ==========
https://codereview.chromium.org/2636833003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:451: return new ChromeFullscreenManager(this, false) { I'm also not against returning null here assuming we know mWebappInfo is initialized properly before we get here. Although if you navigate to a different domain, do we bring down the tiny little location bar from the top in this mode? If we return null, does that break (I believe it uses fullscreen manager to do the showing/hiding of that these days). If it does break, this seems ok to me (but i would hope it would since I suggested it :-P)
Thanks for the checking, Ted. It shows the location bar if navigating a different domain in your webapp. https://codereview.chromium.org/2636833003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java (right): https://codereview.chromium.org/2636833003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java:451: return new ChromeFullscreenManager(this, false) { On 2017/02/17 05:51:50, Ted C (OOO till 2.17) wrote: > I'm also not against returning null here assuming we know mWebappInfo is > initialized properly before we get here. > > Although if you navigate to a different domain, do we bring down the tiny little > location bar from the top in this mode? If we return null, does that break (I > believe it uses fullscreen manager to do the showing/hiding of that these days). > > If it does break, this seems ok to me (but i would hope it would since I > suggested it :-P) I prefer your suggestion. Even returning null is cleaner and worked fine in my testing (all existing code checked fullscreenManager before using), it still looks not very safe. For the different domain bar. In the testing, I switched my test webapp's url in fullscreen mode to google.com.au and the location bar was shown correctly https://drive.google.com/a/chromium.org/file/d/0B7BQekACXW_VanNIUVJfN2tLQlU/v... Thanks
lgtm! thanks for all the videos and comments that helped make this super clear (and thank you for putting up with my rambling comments)
The CQ bit was checked by googleo@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": 160001, "attempt_start_ts": 1487544480866890,
"parent_rev": "3554f65cd92e8391454b0698510b84f6864749c8", "commit_rev":
"f0f67bb9cea082b6e8b1fdf0e392e2fdea524eda"}
Message was sent while issue was closed.
Description was changed from ========== Implement the immersive mode for the FullscreenActivity on Android K+. A web app in immersive mode will hide the Android system navigation and status bars. It will be used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522,680385 ========== to ========== Implement the immersive mode for the FullscreenActivity on Android K+. A web app in immersive mode will hide the Android system navigation and status bars. It will be used to support fullscreen mode for sites added to the home screen. Currently even the "display" mode in web app manifest was configured to fullscreen, it's still launched to a standalone mode. BUG=581522,680385 Review-Url: https://codereview.chromium.org/2636833003 Cr-Commit-Position: refs/heads/master@{#451533} Committed: https://chromium.googlesource.com/chromium/src/+/f0f67bb9cea082b6e8b1fdf0e392... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f0f67bb9cea082b6e8b1fdf0e392... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
