Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(344)

Issue 2688413012: Don't animate the download shelf when entering/exiting fullscreen. (Closed)

Created:
3 years, 10 months ago by Sidney San Martín
Modified:
3 years, 10 months ago
CC:
chromium-reviews, asanka, tfarina, dbeam+watch-downloads_chromium.org, mac-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't animate the download shelf when entering/exiting fullscreen. On Views platforms, I could just barely perceive the animation, and it added jank, not smoothness, to the transition. On Mac, the download shelf animated in/out *after* the fullscreen transition, which was odd and drew attention to it as it disappeared. Now, the shelf appears/disappears as part of the transition — so on Views it doesn't add jank, and on Mac it fades in/out along with the rest of the window. As an added bonus, this fixes a bug where windows on Mac grew by the height of the downloads bar each time you enter/exit fullscreen. It should also fix a flaky test on 10.9 by adding an option to hide/show the shelf without animation. BUG=29629, 687447 Review-Url: https://codereview.chromium.org/2688413012 Cr-Commit-Position: refs/heads/master@{#451327} Committed: https://chromium.googlesource.com/chromium/src/+/7578925d7cf03c4e5a721eb757fd7c35490b57a2

Patch Set 1 #

Patch Set 2 : Test build fixes. #

Patch Set 3 : Rework. #

Patch Set 4 : Rename Show to Open more consistently. #

Total comments: 2

Patch Set 5 : Try fixing a 10.9 test; ignore this patchset for the moment #

Total comments: 8

Patch Set 6 : de-auto #

Patch Set 7 : Turn on animation for all tests except the one that was flaky due to animation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+111 lines, -68 lines) Patch
M chrome/browser/download/download_shelf.h View 1 2 3 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/download/download_shelf.cc View 1 2 3 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/download/download_shelf_unittest.cc View 1 2 3 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/download/test_download_shelf.h View 1 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/download/test_download_shelf.cc View 1 2 chunks +9 lines, -1 line 0 comments Download
M chrome/browser/ui/browser.h View 1 2 3 4 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_controller_private.mm View 1 2 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_controller.h View 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_controller.mm View 2 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm View 1 2 3 4 5 6 11 chunks +15 lines, -33 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_mac.h View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_mac.mm View 1 chunk +14 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/download/download_shelf_mac_unittest.mm View 1 2 3 3 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/ui/cocoa/view_id_util_browsertest.mm View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/exclusive_access/fullscreen_controller.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/exclusive_access/fullscreen_controller.cc View 1 2 3 4 5 2 chunks +10 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/download/download_shelf_view.h View 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/ui/views/download/download_shelf_view.cc View 2 chunks +13 lines, -1 line 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 45 (35 generated)
Sidney San Martín
PTAL https://codereview.chromium.org/2688413012/diff/80001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2688413012/diff/80001/chrome/browser/ui/views/frame/browser_view.cc#newcode2322 chrome/browser/ui/views/frame/browser_view.cc:2322: browser_->WindowFullscreenStateWillChange(); This is a touch awkward. Let me ...
3 years, 10 months ago (2017-02-16 21:32:39 UTC) #20
Avi (use Gerrit)
lgtm https://codereview.chromium.org/2688413012/diff/80001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2688413012/diff/80001/chrome/browser/ui/views/frame/browser_view.cc#newcode2322 chrome/browser/ui/views/frame/browser_view.cc:2322: browser_->WindowFullscreenStateWillChange(); On 2017/02/16 21:32:39, sdy wrote: > This ...
3 years, 10 months ago (2017-02-16 22:49:23 UTC) #23
Nico
lgtm spqchan has been doing fullscreen stuff, maybe she wants to take a quick glance. ...
3 years, 10 months ago (2017-02-17 15:53:46 UTC) #26
Sidney San Martín
https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller.mm File chrome/browser/ui/cocoa/download/download_shelf_controller.mm (right): https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller.mm#newcode253 chrome/browser/ui/cocoa/download/download_shelf_controller.mm:253: // smoother. On 2017/02/17 15:53:46, Nico wrote: > maybe ...
3 years, 10 months ago (2017-02-17 16:07:08 UTC) #28
Nico
https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm File chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm (right): https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm#newcode390 chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm:390: } On 2017/02/17 16:07:08, sdy wrote: > On 2017/02/17 ...
3 years, 10 months ago (2017-02-17 16:13:09 UTC) #31
Sidney San Martín
On 2017/02/17 16:13:09, Nico wrote: > https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm > File chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm > (right): > > https://codereview.chromium.org/2688413012/diff/120001/chrome/browser/ui/cocoa/download/download_shelf_controller_unittest.mm#newcode390 ...
3 years, 10 months ago (2017-02-17 16:22:10 UTC) #33
spqchan
lgtm
3 years, 10 months ago (2017-02-17 16:39:58 UTC) #35
Nico
lgtm++
3 years, 10 months ago (2017-02-17 16:41:32 UTC) #36
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2688413012/160001
3 years, 10 months ago (2017-02-17 17:16:15 UTC) #42
commit-bot: I haz the power
3 years, 10 months ago (2017-02-17 17:26:47 UTC) #45
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/7578925d7cf03c4e5a721eb757fd...

Powered by Google App Engine
This is Rietveld 408576698