Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1589023003:
[Contextual Search] Enable for fullscreen (Closed)
Description[Contextual Search] Enable for fullscreen
Make changes necessary to handle the screen size change when a tab enters
and exits fullscreen, and enable Contextual Search for non-video fullscreen.
BUG=515151
Committed: https://crrev.com/f609f72d34bf97450d898e363662be69f2c89eb9
Cr-Commit-Position: refs/heads/master@{#371616}
Patch Set 1 #Patch Set 2 : Move a couple of methods #
Total comments: 9
Patch Set 3 : Rebase #Patch Set 4 : Add tests, rename isFullscreenPanel to isFullWidthSizePanel #
Total comments: 1
Patch Set 5 : Fix typo #Patch Set 6 : Rebase #
Total comments: 13
Patch Set 7 : Address reviewer comments #Patch Set 8 : Fix typo #Patch Set 9 : Rebase #Patch Set 10 : Rename getFullscreenHeight to getTabHeight, getMaximumHeight to getMaximumPanelHeight #Patch Set 11 : Rebase #Patch Set 12 : Add RESTRICTION_TYPE_PHONE to test that taps base page #Patch Set 13 : Rebase #Messages
Total messages: 41 (9 generated)
twellington@chromium.org changed reviewers: + changwan@chromium.org, donnd@chromium.org
changwan@ - ptal at chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java donnd@ - ptal at everything https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:81: @Override Note: this annotation was missing; this method overrides onSizeChanged in ContextualSearchSupportedLayout. https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:87: mBaseTab.setMaxContentHeight(panel.getFullscreenHeight()); Without this change I was seeing a bug in narrow panel mode when the tab was in fullscreen where the base tab contents didn't draw to the bottom of the screen. In narrow panel mode, when this call is made ContextualSearchPanelBase's mMaximumHeight = getFullScreenHeight() - mToolbarHeight. For the regular panel, when this call is made mMaximumHeight = getFullScreenHeight(). See ContextualSearchPanelBase#getPanelHeightFromState(PanelState.MAXIMIZED). The narrow panel's max height is shorter than the regular (fullscreen) panel's max height, which results in the tab contents height being shorter. I think changing these two lines of code makes sense, but I don't know if there's any special history about why getMaximumHeight() was chosen initially. This method was introduced in https://codereview.chromium.org/1536623003/ If any of the reviewers who are in town remember the original change/history, please let me know. Otherwise I will discuss with Pedro when he returns (there's no rush to get this CL landed).
LGTM, though I don't know this code very well. https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:308: public void setIsFullscreenSizePanelForTesting(boolean isFullscreenSizePanel) { Not from this CL, but "FullscreenSize" in this context means non-narrow panel, right? That's very confusing, since we have code that needs to know about fullscreen displays. Do you think it would be worthwhile renaming all these FullscreenSize to FullwidthSize (or FullWidthSize)? https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:81: @Override On 2016/01/14 20:26:02, Theresa Wellington wrote: > Note: this annotation was missing; this method overrides onSizeChanged in > ContextualSearchSupportedLayout. Acknowledged.
lgtm https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:308: public void setIsFullscreenSizePanelForTesting(boolean isFullscreenSizePanel) { On 2016/01/14 20:44:34, Donn Denman wrote: > Not from this CL, but "FullscreenSize" in this context means non-narrow panel, > right? That's very confusing, since we have code that needs to know about > fullscreen displays. Do you think it would be worthwhile renaming all these > FullscreenSize to FullwidthSize (or FullWidthSize)? ...or "NonNarrowSize", or "WideSize".
Any chance that we can add a test around the fullscreen scenario? ContextualSearchManagerTest and FullscreenManagerTest have some examples for contextual search and non-video fullscreen toggling, so I think it's possible. WDYT?
On 2016/01/15 00:14:26, Changwan Ryu wrote: > Any chance that we can add a test around the fullscreen scenario? > ContextualSearchManagerTest and FullscreenManagerTest have some examples for > contextual search and non-video fullscreen toggling, so I think it's possible. > WDYT? Sure. Maybe one to make sure the panel ends up in the right spot when expanded in fullscreen mode? That's the only real difference for fullscreen. Are there other scenarios you think it would be good to cover?
On 2016/01/15 00:17:23, Theresa Wellington wrote: > On 2016/01/15 00:14:26, Changwan Ryu wrote: > > Any chance that we can add a test around the fullscreen scenario? > > ContextualSearchManagerTest and FullscreenManagerTest have some examples for > > contextual search and non-video fullscreen toggling, so I think it's possible. > > WDYT? > > Sure. Maybe one to make sure the panel ends up in the right spot when expanded > in fullscreen mode? That's the only real difference for fullscreen. Are there > other scenarios you think it would be good to cover? I think it's good to cover the basic scenario: enter fullscreen -> trigger contextual search -> (change selection if possible) -> exit fullscreen
https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1105: // Workaround to disable Contextual Search in HTML fullscreen mode. crbug.com/511977 remove this comment. By the way, why do you still check overlay video mode? what scenario are you concerned about?
On 2016/01/15 00:42:05, Changwan Ryu wrote: > On 2016/01/15 00:17:23, Theresa Wellington wrote: > > On 2016/01/15 00:14:26, Changwan Ryu wrote: > > > Any chance that we can add a test around the fullscreen scenario? > > > ContextualSearchManagerTest and FullscreenManagerTest have some examples for > > > contextual search and non-video fullscreen toggling, so I think it's > possible. > > > WDYT? > > > > Sure. Maybe one to make sure the panel ends up in the right spot when expanded > > in fullscreen mode? That's the only real difference for fullscreen. Are there > > other scenarios you think it would be good to cover? > > I think it's good to cover the basic scenario: > > enter fullscreen -> trigger contextual search -> (change selection if possible) > -> exit fullscreen Okay. I'll add a test for that.
https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1105: // Workaround to disable Contextual Search in HTML fullscreen mode. crbug.com/511977 On 2016/01/15 01:17:10, Changwan Ryu wrote: > remove this comment. By the way, why do you still check overlay video mode? what > scenario are you concerned about? We don't want to Contextual Search to work in fullscreen videos because there isn't text on videos that's useful to do a search on. On YouTube, for example, you can popup a dialog with the video URL and long press to select & copy the URL text. We don't want the CS panel to show up in that scenario and it would without this check.
On 2016/01/15 01:37:24, Theresa Wellington wrote: > https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java > (right): > > https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1105: > // Workaround to disable Contextual Search in HTML fullscreen mode. > crbug.com/511977 > On 2016/01/15 01:17:10, Changwan Ryu wrote: > > remove this comment. By the way, why do you still check overlay video mode? > what > > scenario are you concerned about? > > We don't want to Contextual Search to work in fullscreen videos because there > isn't text on videos that's useful to do a search on. On YouTube, for example, > you can popup a dialog with the video URL and long press to select & copy the > URL text. We don't want the CS panel to show up in that scenario and it would > without this check. Got it. Thanks for the explanation.
twellington@chromium.org changed reviewers: + dtrainor@chromium.org
Added tests and made other change requested by reviewers. +dtrainor@ for changes to FullscreenManagerTest and introduction of FullscreenTestUtils.java https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/contextualsearch/ContextualSearchPanelBase.java:308: public void setIsFullscreenSizePanelForTesting(boolean isFullscreenSizePanel) { On 2016/01/14 20:47:05, Donn Denman wrote: > On 2016/01/14 20:44:34, Donn Denman wrote: > > Not from this CL, but "FullscreenSize" in this context means non-narrow panel, > > right? That's very confusing, since we have code that needs to know about > > fullscreen displays. Do you think it would be worthwhile renaming all these > > FullscreenSize to FullwidthSize (or FullWidthSize)? > > ...or "NonNarrowSize", or "WideSize". I agree that it should be changed, particularly with the introduction of support for CS in fullscreen mode. I changed it to FullWidthSizePanel to mirror our isNarrowSizePanelSupported method. https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java (right): https://codereview.chromium.org/1589023003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:1105: // Workaround to disable Contextual Search in HTML fullscreen mode. crbug.com/511977 On 2016/01/15 01:37:24, Theresa Wellington wrote: > On 2016/01/15 01:17:10, Changwan Ryu wrote: > > remove this comment. By the way, why do you still check overlay video mode? > what > > scenario are you concerned about? > > We don't want to Contextual Search to work in fullscreen videos because there > isn't text on videos that's useful to do a search on. On YouTube, for example, > you can popup a dialog with the video URL and long press to select & copy the > URL text. We don't want the CS panel to show up in that scenario and it would > without this check. Done. https://codereview.chromium.org/1589023003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1589023003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:804: float tapX = w / 2f; Note: I'm going to prepare another CL with this change and re-enable all of our tests that are restricted to phones only. Discussed w/ donnd@ and we shouldn't be calculating the tapX/Y here based on orientation.
twellington@chromium.org changed reviewers: + tedchoc@chromium.org - dtrainor@chromium.org
Moving a comment to the current patchset where it's more likely to be seen. -dtrainor@, +tedchoc@, who seems like a better owner of the fullscreen tests based on recent activity. tedchoc@ - ptal at changes to FullscreenManagerTest and introduction of FullscreenTestUtils.java https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManagerTest.java:822: float tapX = w / 2f; Note: I'm going to prepare another CL with this change and re-enable all of our tests that are restricted to phones only. Discussed w/ donnd@ and we shouldn't be calculating the tapX/Y here based on orientation.
lgtm compositor/ https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); could you leave some comment on why this should not be getMaximumHeight()?
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/18 02:25:30, Changwan Ryu wrote: > could you leave some comment on why this should not be getMaximumHeight()? Yes, I'll add a comment to the code. Most of the time getMaximumHeight() and getFullscreenHeight() return the same thing, but when we are in narrow panel mode the maximum panel height is shorter than the fullscreen height by the height of the toolbar. I think the base tab height should always be the fullscreen height and setting it to getMaximumHeight() was an oversight that did not consider narrow panel mode, but I don't know the history of this code (and Pedro is ooo). I believe you were a reviewer on the CL that introduced this code; do you remember why getMaximumHeight() was chosen?
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/18 23:06:48, Theresa Wellington wrote: > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > could you leave some comment on why this should not be getMaximumHeight()? > > Yes, I'll add a comment to the code. Most of the time getMaximumHeight() and > getFullscreenHeight() return the same thing, but when we are in narrow panel > mode the maximum panel height is shorter than the fullscreen height by the > height of the toolbar. I think the base tab height should always be the > fullscreen height and setting it to getMaximumHeight() was an oversight that > did not consider narrow panel mode, but I don't know the history of this code > (and Pedro is ooo). I believe you were a reviewer on the CL that introduced this > code; do you remember why getMaximumHeight() was chosen? Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks whether fullscreensizepanel is being used or not. I think this is a bug.
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/19 00:09:13, Changwan Ryu wrote: > On 2016/01/18 23:06:48, Theresa Wellington wrote: > > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > > could you leave some comment on why this should not be getMaximumHeight()? > > > > Yes, I'll add a comment to the code. Most of the time getMaximumHeight() and > > getFullscreenHeight() return the same thing, but when we are in narrow panel > > mode the maximum panel height is shorter than the fullscreen height by the > > height of the toolbar. I think the base tab height should always be the > > fullscreen height and setting it to getMaximumHeight() was an oversight that > > did not consider narrow panel mode, but I don't know the history of this code > > (and Pedro is ooo). I believe you were a reviewer on the CL that introduced > this > > code; do you remember why getMaximumHeight() was chosen? > > Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks whether > fullscreensizepanel is being used or not. I think this is a bug. I think that code is correct. In narrow panel mode, the panel doesn't go to the top of the screen, so the panel height in the expanded/maximum states should be the fullscreen height minus the toolbar height.
lgtm modulo comment for fullscreen test changes https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:365: private void togglePersistentFullscreen(final TabWebContentsDelegateAndroid delegate, delete this method too?
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/19 18:39:06, Theresa Wellington wrote: > On 2016/01/19 00:09:13, Changwan Ryu wrote: > > On 2016/01/18 23:06:48, Theresa Wellington wrote: > > > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > > > could you leave some comment on why this should not be getMaximumHeight()? > > > > > > Yes, I'll add a comment to the code. Most of the time getMaximumHeight() and > > > getFullscreenHeight() return the same thing, but when we are in narrow panel > > > mode the maximum panel height is shorter than the fullscreen height by the > > > height of the toolbar. I think the base tab height should always be the > > > fullscreen height and setting it to getMaximumHeight() was an oversight > that > > > did not consider narrow panel mode, but I don't know the history of this > code > > > (and Pedro is ooo). I believe you were a reviewer on the CL that introduced > > this > > > code; do you remember why getMaximumHeight() was chosen? > > > > Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks whether > > fullscreensizepanel is being used or not. I think this is a bug. > > I think that code is correct. In narrow panel mode, the panel doesn't go to the > top of the screen, so the panel height in the expanded/maximum states should be > the fullscreen height minus the toolbar height. Hmm... Ideally speaking, after entering fullscreen mode, shouldn't onSizeChanged() be called and panel.getMaximumHeight() be equal to getFullscreenHeight()? I was wondering if this function is called only once for some reason or if 'fullscreen mode' is technically entered after the size change. Anyways, I'm ok with this as long as CS works on non-fullscreen mode.
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/20 07:29:14, Changwan Ryu wrote: > On 2016/01/19 18:39:06, Theresa Wellington wrote: > > On 2016/01/19 00:09:13, Changwan Ryu wrote: > > > On 2016/01/18 23:06:48, Theresa Wellington wrote: > > > > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > > > > could you leave some comment on why this should not be > getMaximumHeight()? > > > > > > > > Yes, I'll add a comment to the code. Most of the time getMaximumHeight() > and > > > > getFullscreenHeight() return the same thing, but when we are in narrow > panel > > > > mode the maximum panel height is shorter than the fullscreen height by the > > > > height of the toolbar. I think the base tab height should always be the > > > > fullscreen height and setting it to getMaximumHeight() was an oversight > > that > > > > did not consider narrow panel mode, but I don't know the history of this > > code > > > > (and Pedro is ooo). I believe you were a reviewer on the CL that > introduced > > > this > > > > code; do you remember why getMaximumHeight() was chosen? > > > > > > Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks whether > > > fullscreensizepanel is being used or not. I think this is a bug. > > > > I think that code is correct. In narrow panel mode, the panel doesn't go to > the > > top of the screen, so the panel height in the expanded/maximum states should > be > > the fullscreen height minus the toolbar height. > > Hmm... Ideally speaking, after entering fullscreen mode, shouldn't > onSizeChanged() be called and panel.getMaximumHeight() be equal to > getFullscreenHeight()? I was wondering if this function is called only once for > some reason or if 'fullscreen mode' is technically entered after the size > change. > > Anyways, I'm ok with this as long as CS works on non-fullscreen mode. The maximum panel height is completely orthogonal to whether or not the tab is in fullscreen mode, so no, getMaximumHeight() shouldn't necessarily be equal to getFullscreenHeight() after the the tab enters fullscreen mode. I think getFullscreenHeight() is causing confusion. We have two panel modes - narrow width and full width. When the panel is full width, it's maximum height is the full height of the screen (regardless of whether the tab is in fullscreen mode or not). When the panel is narrow width, its maximum height is less than the full height of the screen. This was a design decision. The isFullWidthSizePanel() call in getPanelHeightFromState() checks whether the panel is full width or narrow width (not if the tab is in fullscreen mode). I already renamed some of the "fullscreen" methods in ContextualSearchPanelBase to "fullWidth" (e.g. isFullWidthSizePanel() used to be isFullscreenSizePanel()). I left getFullscreenHeight() alone, because it is returning the full height of the screen (as opposed to the height of the panel).
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/20 16:45:47, Theresa Wellington wrote: > On 2016/01/20 07:29:14, Changwan Ryu wrote: > > On 2016/01/19 18:39:06, Theresa Wellington wrote: > > > On 2016/01/19 00:09:13, Changwan Ryu wrote: > > > > On 2016/01/18 23:06:48, Theresa Wellington wrote: > > > > > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > > > > > could you leave some comment on why this should not be > > getMaximumHeight()? > > > > > > > > > > Yes, I'll add a comment to the code. Most of the time getMaximumHeight() > > and > > > > > getFullscreenHeight() return the same thing, but when we are in narrow > > panel > > > > > mode the maximum panel height is shorter than the fullscreen height by > the > > > > > height of the toolbar. I think the base tab height should always be the > > > > > fullscreen height and setting it to getMaximumHeight() was an oversight > > > that > > > > > did not consider narrow panel mode, but I don't know the history of this > > > code > > > > > (and Pedro is ooo). I believe you were a reviewer on the CL that > > introduced > > > > this > > > > > code; do you remember why getMaximumHeight() was chosen? > > > > > > > > Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks > whether > > > > fullscreensizepanel is being used or not. I think this is a bug. > > > > > > I think that code is correct. In narrow panel mode, the panel doesn't go to > > the > > > top of the screen, so the panel height in the expanded/maximum states > should > > be > > > the fullscreen height minus the toolbar height. > > > > Hmm... Ideally speaking, after entering fullscreen mode, shouldn't > > onSizeChanged() be called and panel.getMaximumHeight() be equal to > > getFullscreenHeight()? I was wondering if this function is called only once > for > > some reason or if 'fullscreen mode' is technically entered after the size > > change. > > > > Anyways, I'm ok with this as long as CS works on non-fullscreen mode. > > The maximum panel height is completely orthogonal to whether or not the tab is > in fullscreen mode, so no, getMaximumHeight() shouldn't necessarily be equal to > getFullscreenHeight() after the the tab enters fullscreen mode. > > I think getFullscreenHeight() is causing confusion. We have two panel modes - > narrow width and full width. When the panel is full width, it's maximum height > is the full height of the screen (regardless of whether the tab is in fullscreen > mode or not). When the panel is narrow width, its maximum height is less than > the full height of the screen. This was a design decision. The > isFullWidthSizePanel() call in getPanelHeightFromState() checks whether the > panel is full width or narrow width (not if the tab is in fullscreen mode). > > I already renamed some of the "fullscreen" methods in ContextualSearchPanelBase > to "fullWidth" (e.g. isFullWidthSizePanel() used to be isFullscreenSizePanel()). > I left getFullscreenHeight() alone, because it is returning the full height of > the screen (as opposed to the height of the panel). Re as long as CS works in non-fullscreen: it does; I didn't actually change any of the maximum height calculations, I'm just changing the base tab's maximum height because when the tab is in fullscreen mode when in landscape on a tablet, calling mBaseTab.setMaximumContentHeight(panel.getMaximumHeight()) was causing the bottom of the tab's content not to get drawn (since the maximum height is shorter than the fullscreen height)
Sorry for the many responses. Would it be less confusing I rename getFullscreenHeight() to getVisibleContentHeight() or getViewportHeight() or something like that? That method is returning the content height + visible toolbar height - the total drawing space we have for the activity.
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/20 16:49:24, Theresa Wellington wrote: > On 2016/01/20 16:45:47, Theresa Wellington wrote: > > On 2016/01/20 07:29:14, Changwan Ryu wrote: > > > On 2016/01/19 18:39:06, Theresa Wellington wrote: > > > > On 2016/01/19 00:09:13, Changwan Ryu wrote: > > > > > On 2016/01/18 23:06:48, Theresa Wellington wrote: > > > > > > On 2016/01/18 02:25:30, Changwan Ryu wrote: > > > > > > > could you leave some comment on why this should not be > > > getMaximumHeight()? > > > > > > > > > > > > Yes, I'll add a comment to the code. Most of the time > getMaximumHeight() > > > and > > > > > > getFullscreenHeight() return the same thing, but when we are in narrow > > > panel > > > > > > mode the maximum panel height is shorter than the fullscreen height by > > the > > > > > > height of the toolbar. I think the base tab height should always be > the > > > > > > fullscreen height and setting it to getMaximumHeight() was an > oversight > > > > that > > > > > > did not consider narrow panel mode, but I don't know the history of > this > > > > code > > > > > > (and Pedro is ooo). I believe you were a reviewer on the CL that > > > introduced > > > > > this > > > > > > code; do you remember why getMaximumHeight() was chosen? > > > > > > > > > > Actually, ContextualSearchPanelBase#getPanelHeightFromState() checks > > whether > > > > > fullscreensizepanel is being used or not. I think this is a bug. > > > > > > > > I think that code is correct. In narrow panel mode, the panel doesn't go > to > > > the > > > > top of the screen, so the panel height in the expanded/maximum states > > should > > > be > > > > the fullscreen height minus the toolbar height. > > > > > > Hmm... Ideally speaking, after entering fullscreen mode, shouldn't > > > onSizeChanged() be called and panel.getMaximumHeight() be equal to > > > getFullscreenHeight()? I was wondering if this function is called only once > > for > > > some reason or if 'fullscreen mode' is technically entered after the size > > > change. > > > > > > Anyways, I'm ok with this as long as CS works on non-fullscreen mode. > > > > The maximum panel height is completely orthogonal to whether or not the tab is > > in fullscreen mode, so no, getMaximumHeight() shouldn't necessarily be equal > to > > getFullscreenHeight() after the the tab enters fullscreen mode. > > > > I think getFullscreenHeight() is causing confusion. We have two panel modes - > > narrow width and full width. When the panel is full width, it's maximum height > > is the full height of the screen (regardless of whether the tab is in > fullscreen > > mode or not). When the panel is narrow width, its maximum height is less than > > the full height of the screen. This was a design decision. The > > isFullWidthSizePanel() call in getPanelHeightFromState() checks whether the > > panel is full width or narrow width (not if the tab is in fullscreen mode). > > > > I already renamed some of the "fullscreen" methods in > ContextualSearchPanelBase > > to "fullWidth" (e.g. isFullWidthSizePanel() used to be > isFullscreenSizePanel()). > > I left getFullscreenHeight() alone, because it is returning the full height of > > the screen (as opposed to the height of the panel). > > Re as long as CS works in non-fullscreen: it does; I didn't actually change any > of the maximum height calculations, I'm just changing the base tab's maximum > height because when the tab is in fullscreen mode when in landscape on a tablet, > calling mBaseTab.setMaximumContentHeight(panel.getMaximumHeight()) was causing > the bottom of the tab's content not to get drawn (since the maximum height is > shorter than the fullscreen height) Thanks for the explanation. I'm still a bit uncomfortable about setting content size slightly larger than what it is in non-fullscreen mode, but I guess it has nothing to do with this change.
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); On 2016/01/22 05:11:24, Changwan Ryu wrote: > Thanks for the explanation. I'm still a bit uncomfortable about setting content > size slightly larger than what it is in non-fullscreen mode, but I guess it has > nothing to do with this change. That's the wrong way to think about it. Replace "non-fullscreen mode" with "non-full-width mode". The maximum panel height is dependent on whether the panel takes up the full width of the screen, not whether the tab is in fullscreen mode. The tab's content height was getting set slightly smaller when the panel was in non-full-width mode. Now it's getting set to the same value regardless of whether the panel is in full-width or non-full-width mode. I sent a detailed email with pictures to illustrate :) I also added a comment to this method. I think if we need to set the base tab's content height (and I'm not convinced we should be doing that from a contextual search class), we should probably use the height value passed into this method, which I believe is equivalent to panel.getFullscreenHeight(). The base tab's height shouldn't be dependent on contextual search. I will discuss with Pedro next week. https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/fullscreen/FullscreenManagerTest.java:365: private void togglePersistentFullscreen(final TabWebContentsDelegateAndroid delegate, On 2016/01/19 19:10:42, Ted C wrote: > delete this method too? Done.
https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java (right): https://codereview.chromium.org/1589023003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/phone/ContextualSearchLayout.java:88: mBaseTab.setContentSize(width, panel.getFullscreenHeight()); As an aside, the reason the bug caused by using panel.getMaximumHeight() didn't present before this in-flight change is because this code was added to handle orientation changes on phones, which don't have a narrow-width (non-full-width) mode for the panel. On orientation change on tablets, the panel is still dismissed, so panel == null and this code isn't currently executed on ToT on tablets ever. In this CL, we get a call to onSizeChanged() when the panel is expanded and the tab is in fullscreen mode, because the Android bottom controls appear, changing the viewport size. The panel isn't null (it's peeking), so this code gets executed and the base tab's content size gets set incorrectly if we use getMaximumHeight() and the panel is narrow-width.
Still LGTM, just chiming in on some naming questions: > getFullscreenHeight() = the tab's height -- maybe this method should be renamed to getViewportHeight() or getTabHeight() or something that implies it's not the panel height? I like getTabHeight. > getMaximumHeight() = the panel's maximum height. The panel's maximum height is dependent on the tab's height (getFullscreenHeight()) AND whether the panel is in full-width or narrow-width mode. Maybe this method should be renamed to getMaximumPanelHeight()? SGTM Up to you to do renaming with this CL or leave as TODO.
On 2016/01/22 19:31:56, Donn Denman wrote: > Still LGTM, just chiming in on some naming questions: > > getFullscreenHeight() = the tab's height -- maybe this method should be > renamed to getViewportHeight() or getTabHeight() or something that implies it's > not the panel height? > I like getTabHeight. > > > getMaximumHeight() = the panel's maximum height. The panel's maximum height is > dependent on the tab's height (getFullscreenHeight()) AND whether the panel is > in full-width or narrow-width mode. Maybe this method should be renamed to > getMaximumPanelHeight()? > SGTM > > Up to you to do renaming with this CL or leave as TODO. I renamed the methods to getTabHeight() and getMaximumPanelHeight() & rebased to pick up mdjone's change to move ContextualSearchPanelBase.java to OverlayPanelBase.java.
I discussed the change to ContextualSearchLayout#onSizeChanged() to set mBaseTab's content size based on getTabHeight() (formerly getFullsreenHeight()) w/ Pedro offline and he agreed that the change made sense. I'm going to land this now.
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, changwan@chromium.org, donnd@chromium.org Link to the patchset: https://codereview.chromium.org/1589023003/#ps220001 (title: "Add RESTRICTION_TYPE_PHONE to test that taps base page")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589023003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589023003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by twellington@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, donnd@chromium.org, changwan@chromium.org Link to the patchset: https://codereview.chromium.org/1589023003/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1589023003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1589023003/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== [Contextual Search] Enable for fullscreen Make changes necessary to handle the screen size change when a tab enters and exits fullscreen, and enable Contextual Search for non-video fullscreen. BUG=515151 ========== to ========== [Contextual Search] Enable for fullscreen Make changes necessary to handle the screen size change when a tab enters and exits fullscreen, and enable Contextual Search for non-video fullscreen. BUG=515151 Committed: https://crrev.com/f609f72d34bf97450d898e363662be69f2c89eb9 Cr-Commit-Position: refs/heads/master@{#371616} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/f609f72d34bf97450d898e363662be69f2c89eb9 Cr-Commit-Position: refs/heads/master@{#371616} |
