Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1547563003:
Fix and simplify bookmark bar detach/attach animation. (Closed)
|
Created:
4 years, 12 months ago by Evan Stade Modified:
4 years, 11 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix and simplify bookmark bar detach/attach animation. (Shown on NTP)
Problem 1: the cross-fading was not executed properly. It should start by
drawing the attached bar at full opacity and transition to detached by
painting detached on top of that at partial opacity (defined by
animation state).
Problem 2: it painted as fully detached when current_state was 0. At this
point it should be painting as fully attached. This led to a single-frame
flash of lightness at the start of the attached->detached animation.
BUG=none
Committed: https://crrev.com/e41343316020f0e843f68be647bbd4359cc6a87d
Cr-Commit-Position: refs/heads/master@{#367094}
Patch Set 1 #Patch Set 2 : . #
Total comments: 2
Patch Set 3 : no uint8 #Patch Set 4 : rebase #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== Simplify/fix bookmark bar detach/attach animation. Problem 1: the cross-fading was not executed properly. We should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: we painted as fully detached when current_state was 0. At this point we should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ========== to ========== Fix and simplify bookmark bar detach/attach animation. Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ==========
estade@chromium.org changed reviewers: + pkasting@chromium.org, sky@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547563003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547563003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Huh. When is this actually triggered? Toggling the bookmark bar on and off seems to slide it in and out rather than fade it for me (on this page on my Win Dev build). LGTM https://codereview.chromium.org/1547563003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1547563003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:418: double detach_percent = 1 - bookmark_bar_view_->GetAnimationValue(); Nit: Since this is the only callsite for GetAnimationValue(), perhaps that function should be changed to: SkAlpha BookmarkBarView::GetAnimationValue() const { return static_cast<SkAlpha>(size_animation_->CurrentValueBetween(255, 0)); } Then the code here simply becomes: SkAlpha detached_alpha = bookmark_bar_view_->GetAnimationValue(); if (detached_alpha < 255) { ... } ... Nitpicky note: I use SkAlpha instead of uint8 in part because we're trying to kill uint8 (see Avi's ongoing changes) and in part because SkAlpha, while typedefed to the same thing, is a more logical and descriptive type for alpha values, and I'm hoping to move toward using it directly in the various canvas APIs.
LGTM
Description was changed from ========== Fix and simplify bookmark bar detach/attach animation. Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ========== to ========== Fix and simplify bookmark bar detach/attach animation. (Shown on NTP) Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ==========
> When is this actually triggered? the detach/attach animation is displayed when toggling the bar on the NTP. Updated CL description. https://codereview.chromium.org/1547563003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1547563003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:418: double detach_percent = 1 - bookmark_bar_view_->GetAnimationValue(); On 2015/12/22 05:52:42, Peter Kasting wrote: > Nit: Since this is the only callsite for GetAnimationValue(), perhaps that > function should be changed to: > > SkAlpha BookmarkBarView::GetAnimationValue() const { > return static_cast<SkAlpha>(size_animation_->CurrentValueBetween(255, 0)); > } > > Then the code here simply becomes: > > SkAlpha detached_alpha = bookmark_bar_view_->GetAnimationValue(); > if (detached_alpha < 255) { > ... > } > ... > > Nitpicky note: I use SkAlpha instead of uint8 in part because we're trying to > kill uint8 (see Avi's ongoing changes) and in part because SkAlpha, while > typedefed to the same thing, is a more logical and descriptive type for alpha > values, and I'm hoping to move toward using it directly in the various canvas > APIs. I changed uint8 to SkAlpha and changed BookmarkBarView to expose the animation directly (since there were a couple of single-use pass throughs that can use this instead).
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547563003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547563003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547563003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547563003/60001
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 estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1547563003/#ps60001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1547563003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1547563003/60001
Message was sent while issue was closed.
Description was changed from ========== Fix and simplify bookmark bar detach/attach animation. (Shown on NTP) Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ========== to ========== Fix and simplify bookmark bar detach/attach animation. (Shown on NTP) Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix and simplify bookmark bar detach/attach animation. (Shown on NTP) Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none ========== to ========== Fix and simplify bookmark bar detach/attach animation. (Shown on NTP) Problem 1: the cross-fading was not executed properly. It should start by drawing the attached bar at full opacity and transition to detached by painting detached on top of that at partial opacity (defined by animation state). Problem 2: it painted as fully detached when current_state was 0. At this point it should be painting as fully attached. This led to a single-frame flash of lightness at the start of the attached->detached animation. BUG=none Committed: https://crrev.com/e41343316020f0e843f68be647bbd4359cc6a87d Cr-Commit-Position: refs/heads/master@{#367094} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e41343316020f0e843f68be647bbd4359cc6a87d Cr-Commit-Position: refs/heads/master@{#367094} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
