Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656983005:
Fix the TabScrubber always starting from the first tab in full screen mode. (Closed)
DescriptionFix the TabScrubber always starting from the first tab in full screen mode.
The unrevealed fullscreened-browser's top view used to have the wrong bounds.
That was fixed in the CL: https://codereview.chromium.org/2640433004/. This CL
adds a test to prevent regressions.
BUG=666270
TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser
Review-Url: https://codereview.chromium.org/2656983005
Cr-Commit-Position: refs/heads/master@{#448421}
Committed: https://chromium.googlesource.com/chromium/src/+/bd5f0695c7ad98bbec8accb74a26d30f345dd3bc
Patch Set 1 #
Total comments: 4
Patch Set 2 : Leave only the test #
Total comments: 5
Patch Set 3 : sky's comments #
Messages
Total messages: 35 (21 generated)
The CQ bit was checked by afakhry@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...
afakhry@chromium.org changed reviewers: + jamescook@chromium.org
James, can you please take a look? Thank you!
LGTM https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/tab_scrubber.cc (right): https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. nice comment https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } Nice test. Easy to read.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
afakhry@chromium.org changed reviewers: + sky@chromium.org
+sky@ Please take a look at immersive_mode_controller*. Thank you! https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/tab_scrubber.cc (right): https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. On 2017/01/27 23:21:27, James Cook wrote: > nice comment Thanks! https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } On 2017/01/27 23:21:27, James Cook wrote: > Nice test. Easy to read. Thanks!
Description was changed from ========== Fix the TabScrubber always starting from the first tab in full screen mode. We used to get the starting point before we reveal the browser's top view, which resulted in getting a wrong tab bounds that always corresponded to the first (left-most (right-most in RTL)) tab. This CL fixes this and adds a test. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser ========== to ========== Fix the TabScrubber always starting from the first tab in full screen mode. We used to get the starting point before we reveal the browser's top view, which resulted in getting a wrong tab bounds that always corresponded to the first (left-most (right-most in RTL)) tab. This CL fixes this and adds a test. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser ==========
sky@chromium.org changed reviewers: + warx@chromium.org
On 2017/01/28 00:48:21, afakhry wrote: > +sky@ Please take a look at immersive_mode_controller*. Thank you! > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > File chrome/browser/ui/views/ash/tab_scrubber.cc (right): > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. > On 2017/01/27 23:21:27, James Cook wrote: > > nice comment > > Thanks! > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } > On 2017/01/27 23:21:27, James Cook wrote: > > Nice test. Easy to read. > > Thanks! Is this happening because the tabstrip is not visible in md anymore? https://codereview.chromium.org/2640433004/ is caused by the same issue. In that patch I suggested we should keep the tabstrip visible (not drawn) to avoid problems like this. If https://codereview.chromium.org/2640433004/ makes the tabstrip visible will this bug go away?
On 2017/01/30 16:46:40, sky wrote: > On 2017/01/28 00:48:21, afakhry wrote: > > +sky@ Please take a look at immersive_mode_controller*. Thank you! > > > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > > File chrome/browser/ui/views/ash/tab_scrubber.cc (right): > > > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > > chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. > > On 2017/01/27 23:21:27, James Cook wrote: > > > nice comment > > > > Thanks! > > > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > > File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): > > > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > > chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } > > On 2017/01/27 23:21:27, James Cook wrote: > > > Nice test. Easy to read. > > > > Thanks! > > Is this happening because the tabstrip is not visible in md anymore? > https://codereview.chromium.org/2640433004/ is caused by the same issue. In that > patch I suggested we should keep the tabstrip visible (not drawn) to avoid > problems like this. If https://codereview.chromium.org/2640433004/ makes the > tabstrip visible will this bug go away? It should go away. However, for the tab scrubber, we reveal the tab strip once scrubbing starts anyways, the problem is that we used to reveal it too late after we got the start point. This patch fixes the order and adds a test to catch regressions. I think this is a valid fix regardless of what the other CL will do. WDYT?
The change in chrome/browser/ui/views/ash/tab_scrubber.cc won't be necessary, right? I'm ok with the test, but it seems like the change shouldn't necessary assuming we keep the tabstrip visible. -Scott On Mon, Jan 30, 2017 at 9:36 AM, <afakhry@chromium.org> wrote: > On 2017/01/30 16:46:40, sky wrote: >> On 2017/01/28 00:48:21, afakhry wrote: >> > +sky@ Please take a look at immersive_mode_controller*. Thank you! >> > >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> > File chrome/browser/ui/views/ash/tab_scrubber.cc (right): >> > >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> > chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. >> > On 2017/01/27 23:21:27, James Cook wrote: >> > > nice comment >> > >> > Thanks! >> > >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> > File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): >> > >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> > chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } >> > On 2017/01/27 23:21:27, James Cook wrote: >> > > Nice test. Easy to read. >> > >> > Thanks! >> >> Is this happening because the tabstrip is not visible in md anymore? >> https://codereview.chromium.org/2640433004/ is caused by the same issue. >> In > that >> patch I suggested we should keep the tabstrip visible (not drawn) to avoid >> problems like this. If https://codereview.chromium.org/2640433004/ makes >> the >> tabstrip visible will this bug go away? > > It should go away. However, for the tab scrubber, we reveal the tab strip > once > scrubbing starts anyways, the problem is that we used to reveal it too late > after we got the start point. This patch fixes the order and adds a test to > catch regressions. I think this is a valid fix regardless of what the other > CL > will do. WDYT? > > https://codereview.chromium.org/2656983005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/01/30 18:30:01, sky wrote: > The change in chrome/browser/ui/views/ash/tab_scrubber.cc won't be > necessary, right? I'm ok with the test, but it seems like the change > shouldn't necessary assuming we keep the tabstrip visible. > > -Scott > > On Mon, Jan 30, 2017 at 9:36 AM, <mailto:afakhry@chromium.org> wrote: > > On 2017/01/30 16:46:40, sky wrote: > >> On 2017/01/28 00:48:21, afakhry wrote: > >> > +sky@ Please take a look at immersive_mode_controller*. Thank you! > >> > > >> > > >> > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > >> > File chrome/browser/ui/views/ash/tab_scrubber.cc (right): > >> > > >> > > >> > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > >> > chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. > >> > On 2017/01/27 23:21:27, James Cook wrote: > >> > > nice comment > >> > > >> > Thanks! > >> > > >> > > >> > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > >> > File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): > >> > > >> > > >> > > > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... > >> > chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } > >> > On 2017/01/27 23:21:27, James Cook wrote: > >> > > Nice test. Easy to read. > >> > > >> > Thanks! > >> > >> Is this happening because the tabstrip is not visible in md anymore? > >> https://codereview.chromium.org/2640433004/ is caused by the same issue. > >> In > > that > >> patch I suggested we should keep the tabstrip visible (not drawn) to avoid > >> problems like this. If https://codereview.chromium.org/2640433004/ makes > >> the > >> tabstrip visible will this bug go away? > > > > It should go away. However, for the tab scrubber, we reveal the tab strip > > once > > scrubbing starts anyways, the problem is that we used to reveal it too late > > after we got the start point. This patch fixes the order and adds a test to > > catch regressions. I think this is a valid fix regardless of what the other > > CL > > will do. WDYT? > > > > https://codereview.chromium.org/2656983005/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > assuming that the tabstrip is kept visible, the change in tab_scrubber.cc won't be necessary, correct. Should I wait for the other change to land?
Yes. On Mon, Jan 30, 2017 at 10:48 AM, <afakhry@chromium.org> wrote: > On 2017/01/30 18:30:01, sky wrote: >> The change in chrome/browser/ui/views/ash/tab_scrubber.cc won't be >> necessary, right? I'm ok with the test, but it seems like the change >> shouldn't necessary assuming we keep the tabstrip visible. >> >> -Scott >> >> On Mon, Jan 30, 2017 at 9:36 AM, <mailto:afakhry@chromium.org> wrote: >> > On 2017/01/30 16:46:40, sky wrote: >> >> On 2017/01/28 00:48:21, afakhry wrote: >> >> > +sky@ Please take a look at immersive_mode_controller*. Thank you! >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> >> > File chrome/browser/ui/views/ash/tab_scrubber.cc (right): >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> >> > chrome/browser/ui/views/ash/tab_scrubber.cc:239: // wrong. >> >> > On 2017/01/27 23:21:27, James Cook wrote: >> >> > > nice comment >> >> > >> >> > Thanks! >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> >> > File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): >> >> > >> >> > >> >> >> > >> > https://codereview.chromium.org/2656983005/diff/1/chrome/browser/ui/views/ash... >> >> > chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:356: } >> >> > On 2017/01/27 23:21:27, James Cook wrote: >> >> > > Nice test. Easy to read. >> >> > >> >> > Thanks! >> >> >> >> Is this happening because the tabstrip is not visible in md anymore? >> >> https://codereview.chromium.org/2640433004/ is caused by the same >> >> issue. >> >> In >> > that >> >> patch I suggested we should keep the tabstrip visible (not drawn) to >> >> avoid >> >> problems like this. If https://codereview.chromium.org/2640433004/ >> >> makes >> >> the >> >> tabstrip visible will this bug go away? >> > >> > It should go away. However, for the tab scrubber, we reveal the tab >> > strip >> > once >> > scrubbing starts anyways, the problem is that we used to reveal it too >> > late >> > after we got the start point. This patch fixes the order and adds a test >> > to >> > catch regressions. I think this is a valid fix regardless of what the >> > other >> > CL >> > will do. WDYT? >> > >> > https://codereview.chromium.org/2656983005/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "Chromium-reviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an > email >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > assuming that the tabstrip is kept visible, the change in tab_scrubber.cc > won't > be necessary, correct. Should I wait for the other change to land? > > https://codereview.chromium.org/2656983005/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Fix the TabScrubber always starting from the first tab in full screen mode. We used to get the starting point before we reveal the browser's top view, which resulted in getting a wrong tab bounds that always corresponded to the first (left-most (right-most in RTL)) tab. This CL fixes this and adds a test. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser ========== to ========== Fix the TabScrubber always starting from the first tab in full screen mode. The unrevealed fullscreened-browser's top view used to have the wrong bounds. That was fixed in the CL: https://codereview.chromium.org/2640433004/. This CL adds a test to prevent regressions. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser ==========
The CQ bit was checked by afakhry@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...
sky@, now that the other CL has landed, I kept only the test here and updated the commit's message and CL's description. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:73: immersive_controller_ = nullptr; RemoveObserver? https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:333: base::RunLoop().RunUntilIdle(); Why do you need the RunUntilIdle here?
The CQ bit was checked by afakhry@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...
https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:73: immersive_controller_ = nullptr; On 2017/02/02 22:29:45, sky wrote: > RemoveObserver? Done, but do we really need it? At this point we are already in the destructor of the ImmersiveModeController looping through the observers. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/immersive_... https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:333: base::RunLoop().RunUntilIdle(); On 2017/02/02 22:29:45, sky wrote: > Why do you need the RunUntilIdle here? Removed. I don't need it. This was a leftover from when I was trying to get this test to work properly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc (right): https://codereview.chromium.org/2656983005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/ash/tab_scrubber_browsertest.cc:73: immersive_controller_ = nullptr; On 2017/02/06 18:34:33, afakhry wrote: > On 2017/02/02 22:29:45, sky wrote: > > RemoveObserver? > > Done, but do we really need it? At this point we are already in the destructor > of the ImmersiveModeController looping through the observers. > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/immersive_... > It's nice to have ObserverLists check if there are no lingering observers. But I realize it doesn't matter here because the observerlist hasn't been configured to DCHECK in this case.
The CQ bit was checked by afakhry@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2656983005/#ps40001 (title: "sky's comments")
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": 40001, "attempt_start_ts": 1486420470952860,
"parent_rev": "afe5de5a95145154229b54047f1517929b741b23", "commit_rev":
"bd5f0695c7ad98bbec8accb74a26d30f345dd3bc"}
Message was sent while issue was closed.
Description was changed from ========== Fix the TabScrubber always starting from the first tab in full screen mode. The unrevealed fullscreened-browser's top view used to have the wrong bounds. That was fixed in the CL: https://codereview.chromium.org/2640433004/. This CL adds a test to prevent regressions. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser ========== to ========== Fix the TabScrubber always starting from the first tab in full screen mode. The unrevealed fullscreened-browser's top view used to have the wrong bounds. That was fixed in the CL: https://codereview.chromium.org/2640433004/. This CL adds a test to prevent regressions. BUG=666270 TEST=interactive_ui_tests --gtest_filter=TabScrubberTest.FullScreenBrowser Review-Url: https://codereview.chromium.org/2656983005 Cr-Commit-Position: refs/heads/master@{#448421} Committed: https://chromium.googlesource.com/chromium/src/+/bd5f0695c7ad98bbec8accb74a26... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bd5f0695c7ad98bbec8accb74a26... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
