Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220063002:
Track if the tab switcher is transitioning in the toolbar (Closed)
DescriptionTrack if the tab switcher is transitioning in the toolbar
setTabSwitcherMode will set the state of the tab switcher for the
toolbar before the animations have started causing the progress
bar to be momentarily visible. This change tracks when the tab
switcher mode is set and when the animations finish to show the
progress bar correctly.
BUG=633972
Committed: https://crrev.com/31b3a9a9805e3ca9d7407c7d7a9da6d729460f97
Cr-Commit-Position: refs/heads/master@{#412606}
Patch Set 1 #Patch Set 2 : 4 state approach #
Total comments: 6
Patch Set 3 : address comments #Patch Set 4 : nits #Patch Set 5 : fix shadow bug #Messages
Total messages: 16 (5 generated)
mdjones@chromium.org changed reviewers: + tedchoc@chromium.org
Long story short: mIsInTabSwitcherMode is unreliable because it is set before the transition animations finish; moving the location where mIsInTabSwitcherMode is set to after the animation starts breaks a lot of things. I can find a different solution if this seems like too much of a hack.
mdjones@chromium.org changed reviewers: + bauerb@chromium.org
+bauerb while ted is ooo; see initial comment.
On 2016/08/09 23:40:17, mdjones wrote: > +bauerb while ted is ooo; see initial comment. Didn't have a ton of time to look at this, but I wonder if we could replace two booleans with a four state int? Non-tabswitcher, entering-tabswitcher, tab-switcher, exiting-tabswitcher If that worked, that would be my preference.
On 2016/08/10 05:07:39, Ted C (OOO till 8.15) wrote: > On 2016/08/09 23:40:17, mdjones wrote: > > +bauerb while ted is ooo; see initial comment. > > Didn't have a ton of time to look at this, but I wonder if we could replace two > booleans with a four state int? > > Non-tabswitcher, entering-tabswitcher, tab-switcher, exiting-tabswitcher > > If that worked, that would be my preference. I don't have a strong opinion in principle on four-state int vs. boolean pair, but in the current state I find it very hard to figure out what the interplay between the two flags is. Specifically, what does |mIsInTabSwitcherMode| mean if |mIsTransitioningTabSwitcher| is true -- is it the state we're transitioning from or the state we're transitioning to? Having a four-state iny with the values Ted lists would be more explicit about that, but I could also imagine one flag for the old state and one for the new state. Hm, one advantage of the multi-state int is that we could verify the allowed transitions, which might help prevent bugs (and/or just make it clearer to readers).
Decided to take the 4 state approach and cleaned up the resulting unused code.
lgtm
lgtm w/ a few small nits https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (left): https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:120: @ViewDebug.ExportedProperty(category = "chrome") I'd actually leave the @ViewDebug export. We could use it to diagnose issues where the state isn't getting changed as we expect. And I "think" you could do something fancy to make it even easier to read in HierarchyViewer/Monitor. @ViewDebug.ExportedProperty(category = "chrome", mapping = { @ViewDebug.IntToString(from = STATIC_TAB, to = "STATIC_TAB"), @ViewDebug.IntToString(from = TAB_SWITCHER, to = "TAB_SWITCHER"), ... }) https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:930: if (mTabSwitcherState > TAB_SWITCHER) return; Hmm...I think it'd be slightly easier to read if this check was mTabSwitcherState == ENTERING_TAB_SWITCHER || ... EXITING Maybe we could make a helper function? isTabSwitcherTransitionActive or something that we could push this into. I see that we used to have one isTabSwitcherAnimationRunning, so maybe we could use that and make it accurate with your changes? https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2110: boolean inOrEnteringTabSwitcher = mTabSwitcherState == TAB_SWITCHER couldn't this just be !inOrEnteringStaticTab?
https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (left): https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:120: @ViewDebug.ExportedProperty(category = "chrome") On 2016/08/15 23:00:00, Ted C wrote: > I'd actually leave the @ViewDebug export. We could use it to diagnose issues > where the state isn't getting changed as we expect. > > And I "think" you could do something fancy to make it even easier to read in > HierarchyViewer/Monitor. > > @ViewDebug.ExportedProperty(category = "chrome", mapping = { > @ViewDebug.IntToString(from = STATIC_TAB, to = "STATIC_TAB"), > @ViewDebug.IntToString(from = TAB_SWITCHER, to = "TAB_SWITCHER"), > ... > }) Done. https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java (right): https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:930: if (mTabSwitcherState > TAB_SWITCHER) return; On 2016/08/15 23:00:00, Ted C wrote: > Hmm...I think it'd be slightly easier to read if this check was > > mTabSwitcherState == ENTERING_TAB_SWITCHER || ... EXITING > > Maybe we could make a helper function? isTabSwitcherTransitionActive or > something that we could push this into. I see that we used to have one > isTabSwitcherAnimationRunning, so maybe we could use that and make it accurate > with your changes? Done. https://codereview.chromium.org/2220063002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarPhone.java:2110: boolean inOrEnteringTabSwitcher = mTabSwitcherState == TAB_SWITCHER On 2016/08/15 23:00:00, Ted C wrote: > couldn't this just be !inOrEnteringStaticTab? Done.
The CQ bit was checked by mdjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2220063002/#ps80001 (title: "fix shadow bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Track if the tab switcher is transitioning in the toolbar setTabSwitcherMode will set the state of the tab switcher for the toolbar before the animations have started causing the progress bar to be momentarily visible. This change tracks when the tab switcher mode is set and when the animations finish to show the progress bar correctly. BUG=633972 ========== to ========== Track if the tab switcher is transitioning in the toolbar setTabSwitcherMode will set the state of the tab switcher for the toolbar before the animations have started causing the progress bar to be momentarily visible. This change tracks when the tab switcher mode is set and when the animations finish to show the progress bar correctly. BUG=633972 Committed: https://crrev.com/31b3a9a9805e3ca9d7407c7d7a9da6d729460f97 Cr-Commit-Position: refs/heads/master@{#412606} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/31b3a9a9805e3ca9d7407c7d7a9da6d729460f97 Cr-Commit-Position: refs/heads/master@{#412606} |
