Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
884483006:
Allows top controls to be hidden by default (Closed)
|
Created:
5 years, 10 months ago by Changwan Ryu Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllows top controls to be hidden by default
- Fix some issues regarding the assumption that top controls are shown by
default.
- Pass tab as an argument for full screen events, so that we can call change the top controls state through tab.
- Add isShowingTopControlsEnabled() and use it to determine default position.
BUG=430635
Committed: https://crrev.com/ee8ae5465df0fe689356e21674cc0f604f37c175
Cr-Commit-Position: refs/heads/master@{#315941}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : changed tab as well #Patch Set 7 : add overlappingTopControlsSupported #
Total comments: 17
Patch Set 8 : addressed david's comments #Patch Set 9 : #
Total comments: 4
Patch Set 10 : addressed david's and ted's comments #Patch Set 11 : fix for downstream #
Messages
Total messages: 16 (5 generated)
changwan@chromium.org changed reviewers: + dtrainor@chromium.org, jaekyun@chromium.org, tedchoc@chromium.org
Hi all, This is still WIP, but please take a look when you have time. The downstream change (https://chrome-internal-review.googlesource.com/#/c/193001/) requires this. Still, I couldn't figure out how to make the initial page not to show URL bar at all. Probably I'll need to allow initial top_controls_delta to be changed just like we change top_controls_height, but wasn't sure.
changwan@chromium.org changed reviewers: + aelias@chromium.org
https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2177: public boolean isHidingTopControlsEnabled() { Do these have to be public? Are we calling them externally? https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2209: isShowingTopControlsEnabled() && !mFullscreenManager.getPersistentFullscreenMode(); Can we put !mFullscreenManager.getPersistentFullscreenMode() inside isShowingTopControlsEnabled? https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:49: private static final String TAG = "ChromeFullscreenManager"; Unused? https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:279: if (tab.isHidingTopControlsEnabled() && !tab.isShowingTopControlsEnabled()) { Can you explain what this logic is doing? I'm a little confused to why we need it. Is something not driving ChromeFullscreenManager#updateVisuals? https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:300: tab.updateTopControlsState(TopControlsState.SHOWN, true); Can't we just use tab#updateFullscreenEnabledState()? Is something not updating FullscreenHtmlApiAdapter? https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:648: if (tab == null || tab.isShowingTopControlsEnabled()) { Shouldn't this call (in theory) always be called when showing the top controls are enabled? If not, something seems a little wrong. I'd almost rather you override WebappFullscreenManager in this case actually. This logic feels a little hacky but I understand why you're doing it.
https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:279: if (tab.isHidingTopControlsEnabled() && !tab.isShowingTopControlsEnabled()) { On 2015/02/06 22:30:14, David Trainor wrote: > Can you explain what this logic is doing? I'm a little confused to why we need > it. Is something not driving ChromeFullscreenManager#updateVisuals? Ah ok I think I understand this a bit more. I think this was actually a bug in the old version. If we're already hiding the toolbar and don't get a frame we'll never actually enter fullscreen! Can we just look at the current render offset to make this decision? If it's already fully hidden lets just call enterFullscreen(tab), otherwise continue with the other logic.
https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/Tab.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2177: public boolean isHidingTopControlsEnabled() { On 2015/02/06 22:30:13, David Trainor wrote: > Do these have to be public? Are we calling them externally? Not any more. Changed back to protected. https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/Tab.java:2209: isShowingTopControlsEnabled() && !mFullscreenManager.getPersistentFullscreenMode(); On 2015/02/06 22:30:13, David Trainor wrote: > Can we put !mFullscreenManager.getPersistentFullscreenMode() inside > isShowingTopControlsEnabled? Done. https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:49: private static final String TAG = "ChromeFullscreenManager"; On 2015/02/06 22:30:14, David Trainor wrote: > Unused? Removed https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:279: if (tab.isHidingTopControlsEnabled() && !tab.isShowingTopControlsEnabled()) { On 2015/02/07 00:48:32, David Trainor wrote: > On 2015/02/06 22:30:14, David Trainor wrote: > > Can you explain what this logic is doing? I'm a little confused to why we > need > > it. Is something not driving ChromeFullscreenManager#updateVisuals? > > Ah ok I think I understand this a bit more. I think this was actually a bug in > the old version. If we're already hiding the toolbar and don't get a frame > we'll never actually enter fullscreen! Can we just look at the current render > offset to make this decision? If it's already fully hidden lets just call > enterFullscreen(tab), otherwise continue with the other logic. Ok, now it uses render offset to make the decision. https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:300: tab.updateTopControlsState(TopControlsState.SHOWN, true); On 2015/02/06 22:30:13, David Trainor wrote: > Can't we just use tab#updateFullscreenEnabledState()? Is something not updating > FullscreenHtmlApiAdapter? We need to change the current state to SHOWN, and it should only happen when we are switching from fullscreen to non-fullscreen mode. I couldn't think of a better way, but updateFullscreenEnabledState() can't detect the mode switching. https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:648: if (tab == null || tab.isShowingTopControlsEnabled()) { On 2015/02/06 22:30:13, David Trainor wrote: > Shouldn't this call (in theory) always be called when showing the top controls > are enabled? If not, something seems a little wrong. I'd almost rather you > override WebappFullscreenManager in this case actually. This logic feels a > little hacky but I understand why you're doing it. Four different cases to think about: 1) normal webapp (and embed content view): top controls shouldn't be shown (false) 2) non-secure webapp: top controls should always be shown (true) 3) normal chrome: top controls can be shown (true) 4) chrome in full screen: top controls shouldn't be shown (false) But for #4 we shouldn't be here. For the other three cases, I believe this makes sense. Basically, if top controls can be shown at all, we want the default position to be 0, and otherwise we want the default position to be -mControlContainerHeight. Please let me know if you still find this problematic.
Another question, but this lgtm. https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:300: tab.updateTopControlsState(TopControlsState.SHOWN, true); On 2015/02/09 08:31:40, Changwan Ryu wrote: > On 2015/02/06 22:30:13, David Trainor wrote: > > Can't we just use tab#updateFullscreenEnabledState()? Is something not > updating > > FullscreenHtmlApiAdapter? > > We need to change the current state to SHOWN, and it should only happen when > we are switching from fullscreen to non-fullscreen mode. I couldn't think of a > better way, but updateFullscreenEnabledState() can't detect the mode switching. Doesn't updateFullscreenEnabledState() check FullscreenManager#isPersistentFullscreenMode()? If that isn't enough and we need this I'm okay with it :). https://chromiumcodereview.appspot.com/884483006/diff/120001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:648: if (tab == null || tab.isShowingTopControlsEnabled()) { On 2015/02/09 08:31:40, Changwan Ryu wrote: > On 2015/02/06 22:30:13, David Trainor wrote: > > Shouldn't this call (in theory) always be called when showing the top controls > > are enabled? If not, something seems a little wrong. I'd almost rather you > > override WebappFullscreenManager in this case actually. This logic feels a > > little hacky but I understand why you're doing it. > > Four different cases to think about: > 1) normal webapp (and embed content view): top controls shouldn't be shown > (false) > 2) non-secure webapp: top controls should always be shown (true) > 3) normal chrome: top controls can be shown (true) > 4) chrome in full screen: top controls shouldn't be shown (false) > > But for #4 we shouldn't be here. For the other three cases, I believe this makes > sense. > Basically, if top controls can be shown at all, we want the default position to > be 0, and otherwise we want the default position to be -mControlContainerHeight. > Please let me know if you still find this problematic. I'm okay with it I guess. I just don't like how the behavior can be the exact opposite of what you'd expect given the name of the function. But I see your point.
https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:196: int resControlContainerHeight, boolean overlappingTopControlsSupported) { need to add javadoc for this as well. And I would call this supportsBrowserOverride instead of overlappingTopControlsSupported. Really, here you are trying to disable the token system used by the browser when it wants to overwrite the render's position. https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java (right): https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:60: private Tab mTabInFullscreen; We shouldn't be storing the tab (or storing just the tab). The problem is that we are setting fullscreen ui state on the contentviewcore's container view, and a tab can switch that out without notifying this fullscreen handler (i.e. if you navigate to a native page). In this case, we want to make sure we are setting and clearing the fullscreen flags on the same view, and storing just the tab is not sufficient.
https://codereview.chromium.org/884483006/diff/120001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/884483006/diff/120001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:300: tab.updateTopControlsState(TopControlsState.SHOWN, true); On 2015/02/09 18:49:52, David Trainor wrote: > On 2015/02/09 08:31:40, Changwan Ryu wrote: > > On 2015/02/06 22:30:13, David Trainor wrote: > > > Can't we just use tab#updateFullscreenEnabledState()? Is something not > > updating > > > FullscreenHtmlApiAdapter? > > > > We need to change the current state to SHOWN, and it should only happen when > > we are switching from fullscreen to non-fullscreen mode. I couldn't think of a > > better way, but updateFullscreenEnabledState() can't detect the mode > switching. > > Doesn't updateFullscreenEnabledState() check > FullscreenManager#isPersistentFullscreenMode()? If that isn't enough and we > need this I'm okay with it :). It is possible to change updateFullscreenEnabledState() to check it can set current state to SHOWN in case it isn't in full screen mode. But I'm not sure if it is safe for all the cases. Let me leave it as is. https://codereview.chromium.org/884483006/diff/120001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:648: if (tab == null || tab.isShowingTopControlsEnabled()) { On 2015/02/09 18:49:52, David Trainor wrote: > On 2015/02/09 08:31:40, Changwan Ryu wrote: > > On 2015/02/06 22:30:13, David Trainor wrote: > > > Shouldn't this call (in theory) always be called when showing the top > controls > > > are enabled? If not, something seems a little wrong. I'd almost rather you > > > override WebappFullscreenManager in this case actually. This logic feels a > > > little hacky but I understand why you're doing it. > > > > Four different cases to think about: > > 1) normal webapp (and embed content view): top controls shouldn't be shown > > (false) > > 2) non-secure webapp: top controls should always be shown (true) > > 3) normal chrome: top controls can be shown (true) > > 4) chrome in full screen: top controls shouldn't be shown (false) > > > > But for #4 we shouldn't be here. For the other three cases, I believe this > makes > > sense. > > Basically, if top controls can be shown at all, we want the default position > to > > be 0, and otherwise we want the default position to be > -mControlContainerHeight. > > Please let me know if you still find this problematic. > > I'm okay with it I guess. I just don't like how the behavior can be the exact > opposite of what you'd expect given the name of the function. But I see your > point. Acknowledged. https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java (right): https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java:196: int resControlContainerHeight, boolean overlappingTopControlsSupported) { On 2015/02/09 23:18:26, Ted C wrote: > need to add javadoc for this as well. > > And I would call this supportsBrowserOverride instead of > overlappingTopControlsSupported. Really, here you are trying to disable the > token system used by the browser when it wants to overwrite the render's > position. Done. https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java (right): https://codereview.chromium.org/884483006/diff/160001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/fullscreen/FullscreenHtmlApiHandler.java:60: private Tab mTabInFullscreen; On 2015/02/09 23:18:27, Ted C wrote: > We shouldn't be storing the tab (or storing just the tab). The problem is that > we are setting fullscreen ui state on the contentviewcore's container view, and > a tab can switch that out without notifying this fullscreen handler (i.e. if you > navigate to a native page). > > In this case, we want to make sure we are setting and clearing the fullscreen > flags on the same view, and storing just the tab is not sufficient. Done. But I couldn't find an easy way to test this scenario of navigating from full-screen to a native webpage. It seems that links against chrome:// webpages are not effective: http://changwanryu.blogspot.com/2015/02/fullscreen-to-native-webpage-tests.html
New patchsets have been uploaded after l-g-t-m from dtrainor@chromium.org
lgtm
New patchsets have been uploaded after l-g-t-m from tedchoc@chromium.org
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/884483006/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/ee8ae5465df0fe689356e21674cc0f604f37c175 Cr-Commit-Position: refs/heads/master@{#315941} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
