Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827383004:
[Mac] Fullscreen Toolbar Janky Animation Fix (Closed)
|
Created:
3 years, 8 months ago by spqchan Modified:
3 years, 8 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, mac-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Mac] Fullscreen Toolbar Janky Animation Fix
The browser UI doesn't update when the menubar is being revealed in 10.12+
which makes the toolbar janky. To smooth things out, we should animate
the toolbar in and out.
BUG=672254
Review-Url: https://codereview.chromium.org/2827383004
Cr-Commit-Position: refs/heads/master@{#467368}
Committed: https://chromium.googlesource.com/chromium/src/+/34fe593135e5c15e0660fca8a86b5dbd960e3255
Patch Set 1 : . #Patch Set 2 : Nits #Patch Set 3 : Update comment #
Total comments: 2
Patch Set 4 : Added crbug to TODO #Messages
Total messages: 36 (29 generated)
The CQ bit was checked by spqchan@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by spqchan@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by spqchan@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...
Description was changed from ========== [Mac] Fullscreen Toolbar Jank Fix BUG= ========== to ========== [Mac] Fullscreen Toolbar Janky Animation Fix The Menu Bar Reveal Carbon events are flaky on 10.12+, which makes the toolbar janky. To smooth things out, we should animate the toolbar in and out. BUG=672254 ==========
spqchan@chromium.org changed reviewers: + rsesek@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by spqchan@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Why is this now janky? In my test program, I still seem to be getting the 'rvlf' events delivered and the values look pretty well tweened. Do you see that if you log in MenuBarRevealHandler?
Description was changed from ========== [Mac] Fullscreen Toolbar Janky Animation Fix The Menu Bar Reveal Carbon events are flaky on 10.12+, which makes the toolbar janky. To smooth things out, we should animate the toolbar in and out. BUG=672254 ========== to ========== [Mac] Fullscreen Toolbar Janky Animation Fix The browser UI doesn't update when the menubar is being revealed in 10.12+ which makes the toolbar janky. To smooth things out, we should animate the toolbar in and out. BUG=672254 ==========
The CQ bit was checked by spqchan@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...
On 2017/04/21 16:28:00, Robert Sesek wrote: > Why is this now janky? In my test program, I still seem to be getting the 'rvlf' > events delivered and the values look pretty well tweened. Do you see that if you > log in MenuBarRevealHandler? Sorry, flaky wasn't the right description.The events and values we get and compute are alright. The weird thing though that the UI doesn't getting updated until after the menu bar finished revealing itself, which causes a jank. I'm not sure why, it's something I have to investigate (hence the TODO) In the meantime, applying the animation will smoothes things out (but it's far from perfect) I updated this with a better description (and comments)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm I suppose https://codereview.chromium.org/2827383004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right): https://codereview.chromium.org/2827383004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:145: // TODO(spqchan): Figure out why it's not updating and make the toolbar drop Please file a bug to track this.
https://codereview.chromium.org/2827383004/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm (right): https://codereview.chromium.org/2827383004/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/fullscreen/fullscreen_menubar_tracker.mm:145: // TODO(spqchan): Figure out why it's not updating and make the toolbar drop On 2017/04/25 17:33:33, Robert Sesek wrote: > Please file a bug to track this. Done. I'm using the same bug btw (no intention to close it since this is just a bandaid)
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the patchset: https://codereview.chromium.org/2827383004/#ps100001 (title: "Added crbug to TODO")
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": 100001, "attempt_start_ts": 1493227304386780,
"parent_rev": "078e9bc6857d4b22cc0f1a3a0b38128ff948fc8a", "commit_rev":
"34fe593135e5c15e0660fca8a86b5dbd960e3255"}
Message was sent while issue was closed.
Description was changed from ========== [Mac] Fullscreen Toolbar Janky Animation Fix The browser UI doesn't update when the menubar is being revealed in 10.12+ which makes the toolbar janky. To smooth things out, we should animate the toolbar in and out. BUG=672254 ========== to ========== [Mac] Fullscreen Toolbar Janky Animation Fix The browser UI doesn't update when the menubar is being revealed in 10.12+ which makes the toolbar janky. To smooth things out, we should animate the toolbar in and out. BUG=672254 Review-Url: https://codereview.chromium.org/2827383004 Cr-Commit-Position: refs/heads/master@{#467368} Committed: https://chromium.googlesource.com/chromium/src/+/34fe593135e5c15e0660fca8a86b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/34fe593135e5c15e0660fca8a86b... |
