Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892563004:
Use animated vector icon for app menu notification animation. (Closed)
|
Created:
3 years, 7 months ago by Evan Stade Modified:
3 years, 6 months ago CC:
chromium-reviews, sadrul, awdf+watch_chromium.org, Peter Beverloo, tfarina, mlamouri+watch-notifications_chromium.org, kalyank, spqchan Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse animated vector icon for app menu notification animation.
1. Expose IconDescription in vector icon code so when we add or
modify parameters we don't have to update the API.
TODO: remove the parts of the API that use multiple parameters
and replace them all with IconDescription.
2. Add AnimatedIconView, which caches the start and end states and
controls animating in between as well.
3. Add an animated version of browser_tools.icon. This was crafted
by hand based on the non-animated version and a visual description,
but in the future designers should deliver Android vector drawables
which will make the process of creating these icons much easier
(and perhaps automate-able).
TODO: actually craft a 2x version instead of reusing 1x.
4. Apply this all to the app menu. This solves the issue of pixel
perfection in the steady state.
BUG=718549, 704786
Review-Url: https://codereview.chromium.org/2892563004
Cr-Commit-Position: refs/heads/master@{#476425}
Committed: https://chromium.googlesource.com/chromium/src/+/9d068dc8a60aea79f24f02f6a659972936387087
Patch Set 1 #Patch Set 2 : checkpoint with color transition #Patch Set 3 : git add #
Total comments: 33
Patch Set 4 : sadrul review #Patch Set 5 : sadrul review #Patch Set 6 : gfx_export #
Total comments: 13
Patch Set 7 : pkasting review, sprinkle 'f's to make msvc happy #Patch Set 8 : rebase #Patch Set 9 : fix compile, move comment #Patch Set 10 : EXPORT #Patch Set 11 : one more f #Messages
Total messages: 58 (33 generated)
Description was changed from
==========
WIP - motion in app menu icon
BUG=
==========
to
==========
Use animated vector icon for app menu notification animation.
1. Expose IconDescription in vector icon code so when we add or
modify parameters we don't have to update the API.
TODO: remove the parts of the API that use multiple parameters
and replace them all with IconDescription.
2. Add AnimatedIconView, which caches the start and end states and
controls animating in between as well.
3. Add an animated version of browser_tools.icon. This was crafted
by hand based on the non-animated version and a visual description,
but in the future designers should deliver Android vector drawables
which will make the process of creating these icons much easier
(and perhaps automate-able).
TODO: actually craft a 2x version instead of reusing 1x.
4. Apply this all to the app menu. This solves the issue of pixel
perfection in the steady state.
BUG=718549,704786
==========
estade@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, ptal. AnimatedIconView is probably a good place to start. Sarah, I will ping you after Sadrul reviews and approves.
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/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...)
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; I don't think < operator makes sense for IconDescription. https://codereview.chromium.org/2892563004/diff/40001/ui/message_center/DEPS File ui/message_center/DEPS (right): https://codereview.chromium.org/2892563004/diff/40001/ui/message_center/DEPS#... ui/message_center/DEPS:17: "+ui/vector_icons", Is this related to this CL? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:19: set_can_process_events_within_subtree(false); Why? Shouldn't this be something the creator of AnimatedIconView decide? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); How does this work when the animation ends? (i.e. when does UpdateStaticImage() get called for the end of the animation?) https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:49: target_bounds.ClampToCenteredSize(GetImage().size()); Can this use GetImageBounds()? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:50: canvas->Translate(gfx::Vector2d(target_bounds.OffsetFromOrigin())); OffsetFromOrigin() already returns a Vector2d, right? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:65: return GetWidget()->GetCompositor()->HasAnimationObserver(this); Can we maintain a flag instead? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.h:23: END, Does it not need a TRANSITION state? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.h:50: SkColor color_; It's curious that we don't have this for ImageView itself. How is the color set for those?
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; On 2017/05/23 18:01:14, sadrul wrote: > I don't think < operator makes sense for IconDescription. Why not? This is just used as an ordering operation so we can use it as a key in a map. https://codereview.chromium.org/2892563004/diff/40001/ui/message_center/DEPS File ui/message_center/DEPS (right): https://codereview.chromium.org/2892563004/diff/40001/ui/message_center/DEPS#... ui/message_center/DEPS:17: "+ui/vector_icons", On 2017/05/23 18:01:14, sadrul wrote: > Is this related to this CL? no, removed https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:19: set_can_process_events_within_subtree(false); On 2017/05/23 18:01:14, sadrul wrote: > Why? Shouldn't this be something the creator of AnimatedIconView decide? I don't think there will be any instances of this class that want it to handle events, but if so they can change it away from the default. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/23 18:01:14, sadrul wrote: > How does this work when the animation ends? (i.e. when does UpdateStaticImage() > get called for the end of the animation?) Animate() calls SetState() calls UpdateStaticImage(). When the animation is over that image will be visible. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:49: target_bounds.ClampToCenteredSize(GetImage().size()); On 2017/05/23 18:01:14, sadrul wrote: > Can this use GetImageBounds()? seems so, thanks! https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:50: canvas->Translate(gfx::Vector2d(target_bounds.OffsetFromOrigin())); On 2017/05/23 18:01:14, sadrul wrote: > OffsetFromOrigin() already returns a Vector2d, right? Done. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:65: return GetWidget()->GetCompositor()->HasAnimationObserver(this); On 2017/05/23 18:01:14, sadrul wrote: > Can we maintain a flag instead? how's this? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.h:23: END, On 2017/05/23 18:01:14, sadrul wrote: > Does it not need a TRANSITION state? no, this enum just describes the static state. When transitioning (IsAnimating() is true), |state_| defines the target. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.h:50: SkColor color_; On 2017/05/23 18:01:14, sadrul wrote: > It's curious that we don't have this for ImageView itself. How is the color set > for those? ImageView doesn't know about VectorIcon, it only deals in ImageSkia.
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:530: std::tie(other_icon, other.dip_size, other.color, other.elapsed_time, This is .. comparing pointers? https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; On 2017/05/23 23:38:42, Evan Stade wrote: > On 2017/05/23 18:01:14, sadrul wrote: > > I don't think < operator makes sense for IconDescription. > > Why not? This is just used as an ordering operation so we can use it as a key in > a map. I don't know what the result of comparing two non-animated IconDescription of the same size + dip + color would mean. Can you use a std::unordered_map() with a hash-function instead? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:19: set_can_process_events_within_subtree(false); On 2017/05/23 23:38:42, Evan Stade wrote: > On 2017/05/23 18:01:14, sadrul wrote: > > Why? Shouldn't this be something the creator of AnimatedIconView decide? > > I don't think there will be any instances of this class that want it to handle > events, but if so they can change it away from the default. When you create a View, the default expectation should be that it can receive events. If the creator does not want the View to receive events, which is typically the case when one control View embeds other Views (e.g. |LabelButton::image_|), then the creator should turn the flag off. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/23 23:38:42, Evan Stade wrote: > On 2017/05/23 18:01:14, sadrul wrote: > > How does this work when the animation ends? (i.e. when does > UpdateStaticImage() > > get called for the end of the animation?) > > Animate() calls SetState() calls UpdateStaticImage(). When the animation is over > that image will be visible. Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() when elapsed > duration_?
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:530: std::tie(other_icon, other.dip_size, other.color, other.elapsed_time, On 2017/05/24 17:35:13, sadrul wrote: > This is .. comparing pointers? yes. This is not new. Our code is already doing this. You can compare addresses because each icon only has one definition. https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; On 2017/05/24 17:35:14, sadrul wrote: > On 2017/05/23 23:38:42, Evan Stade wrote: > > On 2017/05/23 18:01:14, sadrul wrote: > > > I don't think < operator makes sense for IconDescription. > > > > Why not? This is just used as an ordering operation so we can use it as a key > in > > a map. > > I don't know what the result of comparing two non-animated IconDescription of > the same size + dip + color would mean. Does it matter what the meaning of the comparison is? It just imposes a deterministic ordering. > > Can you use a std::unordered_map() with a hash-function instead? Aren't we supposed to avoid hash maps? https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:19: set_can_process_events_within_subtree(false); On 2017/05/24 17:35:14, sadrul wrote: > On 2017/05/23 23:38:42, Evan Stade wrote: > > On 2017/05/23 18:01:14, sadrul wrote: > > > Why? Shouldn't this be something the creator of AnimatedIconView decide? > > > > I don't think there will be any instances of this class that want it to handle > > events, but if so they can change it away from the default. > > When you create a View, the default expectation should be that it can receive > events. If the creator does not want the View to receive events, which is > typically the case when one control View embeds other Views (e.g. > |LabelButton::image_|), then the creator should turn the flag off. ok, I will move this to AppMenuButton. https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/24 17:35:14, sadrul wrote: > On 2017/05/23 23:38:42, Evan Stade wrote: > > On 2017/05/23 18:01:14, sadrul wrote: > > > How does this work when the animation ends? (i.e. when does > > UpdateStaticImage() > > > get called for the end of the animation?) > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation is > over > > that image will be visible. > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() when > elapsed > duration_? The client is calling it. In this case, AppMenuButton::AnimateIconIfPossible(). Right now, Animate(START) is never called, but when we have buttons that want to toggle, e.g. a bluetooth icon with or without a strikethrough to indicate disabled/enabled, this will change.
On 2017/05/24 17:47:54, Evan Stade wrote: > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... > File ui/gfx/paint_vector_icon.cc (right): > > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... > ui/gfx/paint_vector_icon.cc:530: std::tie(other_icon, other.dip_size, > other.color, other.elapsed_time, > On 2017/05/24 17:35:13, sadrul wrote: > > This is .. comparing pointers? > > yes. This is not new. Our code is already doing this. You can compare addresses > because each icon only has one definition. > > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h > File ui/gfx/paint_vector_icon.h (right): > > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... > ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) > const; > On 2017/05/24 17:35:14, sadrul wrote: > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > I don't think < operator makes sense for IconDescription. > > > > > > Why not? This is just used as an ordering operation so we can use it as a > key > > in > > > a map. > > > > I don't know what the result of comparing two non-animated IconDescription of > > the same size + dip + color would mean. > > Does it matter what the meaning of the comparison is? It just imposes a > deterministic ordering. > > > > > Can you use a std::unordered_map() with a hash-function instead? > > Aren't we supposed to avoid hash maps? > > https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... > File ui/views/controls/animated_icon_view.cc (right): > > https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... > ui/views/controls/animated_icon_view.cc:19: > set_can_process_events_within_subtree(false); > On 2017/05/24 17:35:14, sadrul wrote: > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > Why? Shouldn't this be something the creator of AnimatedIconView decide? > > > > > > I don't think there will be any instances of this class that want it to > handle > > > events, but if so they can change it away from the default. > > > > When you create a View, the default expectation should be that it can receive > > events. If the creator does not want the View to receive events, which is > > typically the case when one control View embeds other Views (e.g. > > |LabelButton::image_|), then the creator should turn the flag off. > > ok, I will move this to AppMenuButton. > > https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... > ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); > On 2017/05/24 17:35:14, sadrul wrote: > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > How does this work when the animation ends? (i.e. when does > > > UpdateStaticImage() > > > > get called for the end of the animation?) > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation is > > over > > > that image will be visible. > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() when > > elapsed > duration_? > > The client is calling it. In this case, AppMenuButton::AnimateIconIfPossible(). > > Right now, Animate(START) is never called, but when we have buttons that want to > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > disabled/enabled, this will change. friendly ping. The app menu icon launch is blocked on this CL.
https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/24 17:47:53, Evan Stade wrote: > On 2017/05/24 17:35:14, sadrul wrote: > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > How does this work when the animation ends? (i.e. when does > > > UpdateStaticImage() > > > > get called for the end of the animation?) > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation is > > over > > > that image will be visible. > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() when > > elapsed > duration_? > > The client is calling it. In this case, AppMenuButton::AnimateIconIfPossible(). > > Right now, Animate(START) is never called, but when we have buttons that want to > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > disabled/enabled, this will change. The client having to track when the animation ends, and explicitly having to call Animate(END) does not look like a good API. Why can AnimatedIconView() not do that in OnAnimationStep()?
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; On 2017/05/24 17:47:53, Evan Stade wrote: > On 2017/05/24 17:35:14, sadrul wrote: > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > I don't think < operator makes sense for IconDescription. > > > > > > Why not? This is just used as an ordering operation so we can use it as a > key > > in > > > a map. > > > > I don't know what the result of comparing two non-animated IconDescription of > > the same size + dip + color would mean. > > Does it matter what the meaning of the comparison is? It just imposes a > deterministic ordering. Let's create a separate function for that, then, and not override < operator. > > > > > Can you use a std::unordered_map() with a hash-function instead? > > Aren't we supposed to avoid hash maps? Can you link that explains why?
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h File ui/gfx/paint_vector_icon.h (right): https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) const; On 2017/05/26 19:50:27, sadrul wrote: > On 2017/05/24 17:47:53, Evan Stade wrote: > > On 2017/05/24 17:35:14, sadrul wrote: > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > I don't think < operator makes sense for IconDescription. > > > > > > > > Why not? This is just used as an ordering operation so we can use it as a > > key > > > in > > > > a map. > > > > > > I don't know what the result of comparing two non-animated IconDescription > of > > > the same size + dip + color would mean. > > > > Does it matter what the meaning of the comparison is? It just imposes a > > deterministic ordering. > > Let's create a separate function for that, then, and not override < operator. I moved this to a separate struct, although I'd still like to understand why that's important. We define operator< for the sake of std:: data structures a lot, e.g. in gfx::Point, gfx::Rect, gfx::ColorSpace. When should we avoid it and when is it ok? > > > > > > > > > Can you use a std::unordered_map() with a hash-function instead? > > > > Aren't we supposed to avoid hash maps? > > Can you link that explains why? https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/rdxOHKzQmRY https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/26 19:44:47, sadrul wrote: > On 2017/05/24 17:47:53, Evan Stade wrote: > > On 2017/05/24 17:35:14, sadrul wrote: > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > How does this work when the animation ends? (i.e. when does > > > > UpdateStaticImage() > > > > > get called for the end of the animation?) > > > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation > is > > > over > > > > that image will be visible. > > > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() when > > > elapsed > duration_? > > > > The client is calling it. In this case, > AppMenuButton::AnimateIconIfPossible(). > > > > Right now, Animate(START) is never called, but when we have buttons that want > to > > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > > disabled/enabled, this will change. > > The client having to track when the animation ends, and explicitly having to > call Animate(END) does not look like a good API. Why can AnimatedIconView() not > do that in OnAnimationStep()? The client doesn't have to do anything when the animation ends. The client is calling Animation(END) to initiate the animation to the end state.
On 2017/05/26 20:48:52, Evan Stade wrote: > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_icon.h > File ui/gfx/paint_vector_icon.h (right): > > https://codereview.chromium.org/2892563004/diff/40001/ui/gfx/paint_vector_ico... > ui/gfx/paint_vector_icon.h:31: bool operator<(const IconDescription& other) > const; > On 2017/05/26 19:50:27, sadrul wrote: > > On 2017/05/24 17:47:53, Evan Stade wrote: > > > On 2017/05/24 17:35:14, sadrul wrote: > > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > > I don't think < operator makes sense for IconDescription. > > > > > > > > > > Why not? This is just used as an ordering operation so we can use it as > a > > > key > > > > in > > > > > a map. > > > > > > > > I don't know what the result of comparing two non-animated IconDescription > > of > > > > the same size + dip + color would mean. > > > > > > Does it matter what the meaning of the comparison is? It just imposes a > > > deterministic ordering. > > > > Let's create a separate function for that, then, and not override < operator. > > I moved this to a separate struct, although I'd still like to understand why > that's important. We define operator< for the sake of std:: data structures a > lot, e.g. in gfx::Point, gfx::Rect, gfx::ColorSpace. When should we avoid it and > when is it ok? > > > > > > > > > > > > > > Can you use a std::unordered_map() with a hash-function instead? > > > > > > Aren't we supposed to avoid hash maps? > > > > Can you link that explains why? > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/rdxOHKzQmRY > > https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... > File ui/views/controls/animated_icon_view.cc (right): > > https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... > ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); > On 2017/05/26 19:44:47, sadrul wrote: > > On 2017/05/24 17:47:53, Evan Stade wrote: > > > On 2017/05/24 17:35:14, sadrul wrote: > > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > > How does this work when the animation ends? (i.e. when does > > > > > UpdateStaticImage() > > > > > > get called for the end of the animation?) > > > > > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation > > is > > > > over > > > > > that image will be visible. > > > > > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() > when > > > > elapsed > duration_? > > > > > > The client is calling it. In this case, > > AppMenuButton::AnimateIconIfPossible(). > > > > > > Right now, Animate(START) is never called, but when we have buttons that > want > > to > > > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > > > disabled/enabled, this will change. > > > > The client having to track when the animation ends, and explicitly having to > > call Animate(END) does not look like a good API. Why can AnimatedIconView() > not > > do that in OnAnimationStep()? > > The client doesn't have to do anything when the animation ends. The client is > calling Animation(END) to initiate the animation to the end state. Animate(END)*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
ping
https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/26 20:48:52, Evan Stade wrote: > On 2017/05/26 19:44:47, sadrul wrote: > > On 2017/05/24 17:47:53, Evan Stade wrote: > > > On 2017/05/24 17:35:14, sadrul wrote: > > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > > How does this work when the animation ends? (i.e. when does > > > > > UpdateStaticImage() > > > > > > get called for the end of the animation?) > > > > > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the animation > > is > > > > over > > > > > that image will be visible. > > > > > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() > when > > > > elapsed > duration_? > > > > > > The client is calling it. In this case, > > AppMenuButton::AnimateIconIfPossible(). > > > > > > Right now, Animate(START) is never called, but when we have buttons that > want > > to > > > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > > > disabled/enabled, this will change. > > > > The client having to track when the animation ends, and explicitly having to > > call Animate(END) does not look like a good API. Why can AnimatedIconView() > not > > do that in OnAnimationStep()? > > The client doesn't have to do anything when the animation ends. The client is > calling Animation(END) to initiate the animation to the end state. I see. So I guess I don't quite understand how OnPaint() would do the right thing at the end of an animation. Consider the following sequence of events: . AnimatedIconView is created. It sets the 'static image' to the start state. . The client starts animation with Animate(START); The 'static image' is again set to the start of the animation state. . The animation happens, and completes. So IsAnimating() would now return false. . Some damage happens on the view, and OnPaint() is called again. . Because it's not animating, it will paint the 'static image', which is the start-state of the animation, instead of the end-state. This is a bug, right? I am probably missing something? https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:235: new_icon_->Animate(views::AnimatedIconView::END); Who calls Animate(START)? https://codereview.chromium.org/2892563004/diff/100001/ui/views/controls/anim... File ui/views/controls/animated_icon_view.h (right): https://codereview.chromium.org/2892563004/diff/100001/ui/views/controls/anim... ui/views/controls/animated_icon_view.h:45: bool IsAnimating(); const method
https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/30 17:06:51, sadrul wrote: > On 2017/05/26 20:48:52, Evan Stade wrote: > > On 2017/05/26 19:44:47, sadrul wrote: > > > On 2017/05/24 17:47:53, Evan Stade wrote: > > > > On 2017/05/24 17:35:14, sadrul wrote: > > > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > > > How does this work when the animation ends? (i.e. when does > > > > > > UpdateStaticImage() > > > > > > > get called for the end of the animation?) > > > > > > > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the > animation > > > is > > > > > over > > > > > > that image will be visible. > > > > > > > > > > Who is calling Animate(END)? Shouldn't that happen in OnAnimationStep() > > when > > > > > elapsed > duration_? > > > > > > > > The client is calling it. In this case, > > > AppMenuButton::AnimateIconIfPossible(). > > > > > > > > Right now, Animate(START) is never called, but when we have buttons that > > want > > > to > > > > toggle, e.g. a bluetooth icon with or without a strikethrough to indicate > > > > disabled/enabled, this will change. > > > > > > The client having to track when the animation ends, and explicitly having to > > > call Animate(END) does not look like a good API. Why can AnimatedIconView() > > not > > > do that in OnAnimationStep()? > > > > The client doesn't have to do anything when the animation ends. The client is > > calling Animation(END) to initiate the animation to the end state. > > I see. So I guess I don't quite understand how OnPaint() would do the right > thing at the end of an animation. Consider the following sequence of events: > . AnimatedIconView is created. It sets the 'static image' to the start state. > . The client starts animation with Animate(START); The 'static image' is again > set to the start of the animation state. > . The animation happens, and completes. So IsAnimating() would now return > false. > . Some damage happens on the view, and OnPaint() is called again. > . Because it's not animating, it will paint the 'static image', which is the > start-state of the animation, instead of the end-state. This is a bug, right? > > I am probably missing something? You would have called Animate(END), not Animate(START). If you called Animate(START) while already in the initial state, it would jump to the end of the animation and progress backwards to the start state again.
lgtm https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... File ui/views/controls/animated_icon_view.cc (right): https://codereview.chromium.org/2892563004/diff/40001/ui/views/controls/anima... ui/views/controls/animated_icon_view.cc:39: views::ImageView::OnPaint(canvas); On 2017/05/30 18:53:45, Evan Stade wrote: > On 2017/05/30 17:06:51, sadrul wrote: > > On 2017/05/26 20:48:52, Evan Stade wrote: > > > On 2017/05/26 19:44:47, sadrul wrote: > > > > On 2017/05/24 17:47:53, Evan Stade wrote: > > > > > On 2017/05/24 17:35:14, sadrul wrote: > > > > > > On 2017/05/23 23:38:42, Evan Stade wrote: > > > > > > > On 2017/05/23 18:01:14, sadrul wrote: > > > > > > > > How does this work when the animation ends? (i.e. when does > > > > > > > UpdateStaticImage() > > > > > > > > get called for the end of the animation?) > > > > > > > > > > > > > > Animate() calls SetState() calls UpdateStaticImage(). When the > > animation > > > > is > > > > > > over > > > > > > > that image will be visible. > > > > > > > > > > > > Who is calling Animate(END)? Shouldn't that happen in > OnAnimationStep() > > > when > > > > > > elapsed > duration_? > > > > > > > > > > The client is calling it. In this case, > > > > AppMenuButton::AnimateIconIfPossible(). > > > > > > > > > > Right now, Animate(START) is never called, but when we have buttons that > > > want > > > > to > > > > > toggle, e.g. a bluetooth icon with or without a strikethrough to > indicate > > > > > disabled/enabled, this will change. > > > > > > > > The client having to track when the animation ends, and explicitly having > to > > > > call Animate(END) does not look like a good API. Why can > AnimatedIconView() > > > not > > > > do that in OnAnimationStep()? > > > > > > The client doesn't have to do anything when the animation ends. The client > is > > > calling Animation(END) to initiate the animation to the end state. > > > > I see. So I guess I don't quite understand how OnPaint() would do the right > > thing at the end of an animation. Consider the following sequence of events: > > . AnimatedIconView is created. It sets the 'static image' to the start state. > > . The client starts animation with Animate(START); The 'static image' is > again > > set to the start of the animation state. > > . The animation happens, and completes. So IsAnimating() would now return > > false. > > . Some damage happens on the view, and OnPaint() is called again. > > . Because it's not animating, it will paint the 'static image', which is the > > start-state of the animation, instead of the end-state. This is a bug, right? > > > > I am probably missing something? > > You would have called Animate(END), not Animate(START). If you called > Animate(START) while already in the initial state, it would jump to the end of > the animation and progress backwards to the start state again. Aah, yes. That makes sense now. Thanks for explaining.
estade@chromium.org changed reviewers: + pkasting@chromium.org
thanks Sadrul. +pkasting for c/b/u/ https://codereview.chromium.org/2892563004/diff/100001/ui/views/controls/anim... File ui/views/controls/animated_icon_view.h (right): https://codereview.chromium.org/2892563004/diff/100001/ui/views/controls/anim... ui/views/controls/animated_icon_view.h:45: bool IsAnimating(); On 2017/05/30 17:06:51, sadrul wrote: > const method note to self: do
c/b/ui LGTM https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:191: // background. What about just always running the severity_color and the toolbar background color through GetReadableColor()? Seems like that would work, and avoid the need for a conditional here. (I assume the severity color is never near luma 0.5.) https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:235: new_icon_->Animate(views::AnimatedIconView::END); On 2017/05/30 17:06:51, sadrul wrote: > Who calls Animate(START)? I assume no one, since this means "animate to the end state" and I don't think there's a need anywhere to reverse-animate to the start state. https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:115: // when there's a need to alert the user. TODO(estade): rename to Nit: Can this first sentence just be "The menu icon." to avoid the future maintenance hazard if we change the appearance without tweaking the verbiage here to match? https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:118: views::AnimatedIconView* new_icon_ = nullptr; Nit: I really like initing members in the declaration like this. But I'm not so big a fan of doing it for some members and not others (e.g. |severity_|, |type_|, |toolbar_view_|), because it's inconsistent and still sort of error-prone. I suggest (either in this CL or another) moving all the other initializations here as well. (Note that even |weak_factory_| can be inited here using {}; see https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_del... .)
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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:191: // background. On 2017/05/31 02:08:28, Peter Kasting wrote: > What about just always running the severity_color and the toolbar background > color through GetReadableColor()? Seems like that would work, and avoid the > need for a conditional here. > > (I assume the severity color is never near luma 0.5.) I can experiment with that as a follow up, but even if it does work consistently in terms of getting a contrasting color, I'm not sure it would create a color that makes sense in the same way that red means "severe". https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:115: // when there's a need to alert the user. TODO(estade): rename to On 2017/05/31 02:08:29, Peter Kasting wrote: > Nit: Can this first sentence just be "The menu icon." to avoid the future > maintenance hazard if we change the appearance without tweaking the verbiage > here to match? I understand the concern but I think specificity is useful for people who are less familiar with the code. When I'm reading through UI code I'm unfamiliar with, I find concrete descriptions are very useful. Hopefully we won't let this get out of date. https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:118: views::AnimatedIconView* new_icon_ = nullptr; On 2017/05/31 02:08:29, Peter Kasting wrote: > Nit: I really like initing members in the declaration like this. But I'm not so > big a fan of doing it for some members and not others (e.g. |severity_|, > |type_|, |toolbar_view_|), because it's inconsistent and still sort of > error-prone. I suggest (either in this CL or another) moving all the other > initializations here as well. (Note that even |weak_factory_| can be inited > here using {}; see > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_del... > .) good call. Updated all except toolbar_view_, which gets inited to the parameter passed into the ctor. In that case, it doesn't seem like inline initing adds much except the false notion that the member starts out as null.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:191: // background. On 2017/05/31 17:08:16, Evan Stade wrote: > On 2017/05/31 02:08:28, Peter Kasting wrote: > > What about just always running the severity_color and the toolbar background > > color through GetReadableColor()? Seems like that would work, and avoid the > > need for a conditional here. > > > > (I assume the severity color is never near luma 0.5.) > > I can experiment with that as a follow up, but even if it does work consistently > in terms of getting a contrasting color, I'm not sure it would create a color > that makes sense in the same way that red means "severe". If you put in red, you always get red out. That's why GetReadableColor() only mucks with luma: so it doesn't change the "meaning" of the color. https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:115: // when there's a need to alert the user. TODO(estade): rename to On 2017/05/31 17:08:16, Evan Stade wrote: > On 2017/05/31 02:08:29, Peter Kasting wrote: > > Nit: Can this first sentence just be "The menu icon." to avoid the future > > maintenance hazard if we change the appearance without tweaking the verbiage > > here to match? > > I understand the concern but I think specificity is useful for people who are > less familiar with the code. When I'm reading through UI code I'm unfamiliar > with, I find concrete descriptions are very useful. Hopefully we won't let this > get out of date. I think there's value in describing what this is, but I think that makes sense to do in a comment above the top of the class. I wouldn't describe the visual look on the member down here, just because it's a lot less likely that's where someone is looking or would update if things change.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2892563004/#ps160001 (title: "fix compile, move comment")
https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2892563004/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:115: // when there's a need to alert the user. TODO(estade): rename to On 2017/05/31 17:24:31, Peter Kasting wrote: > On 2017/05/31 17:08:16, Evan Stade wrote: > > On 2017/05/31 02:08:29, Peter Kasting wrote: > > > Nit: Can this first sentence just be "The menu icon." to avoid the future > > > maintenance hazard if we change the appearance without tweaking the verbiage > > > here to match? > > > > I understand the concern but I think specificity is useful for people who are > > less familiar with the code. When I'm reading through UI code I'm unfamiliar > > with, I find concrete descriptions are very useful. Hopefully we won't let > this > > get out of date. > > I think there's value in describing what this is, but I think that makes sense > to do in a comment above the top of the class. I wouldn't describe the visual > look on the member down here, just because it's a lot less likely that's where > someone is looking or would update if things change. Seems like it could go either way. If you're changing the appearance dramatically you might remove this view completely and naturally delete or change this comment, whereas you might never look at or update the class-level comment. In any case I've promoted the comment to be class-level.
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2892563004/#ps180001 (title: "EXPORT")
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
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...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2892563004/#ps200001 (title: "one more f")
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": 200001, "attempt_start_ts": 1496339358343760,
"parent_rev": "81ba45d614d7e3abd6f4d9062baab9f43c5f283b", "commit_rev":
"9d068dc8a60aea79f24f02f6a659972936387087"}
Message was sent while issue was closed.
Description was changed from
==========
Use animated vector icon for app menu notification animation.
1. Expose IconDescription in vector icon code so when we add or
modify parameters we don't have to update the API.
TODO: remove the parts of the API that use multiple parameters
and replace them all with IconDescription.
2. Add AnimatedIconView, which caches the start and end states and
controls animating in between as well.
3. Add an animated version of browser_tools.icon. This was crafted
by hand based on the non-animated version and a visual description,
but in the future designers should deliver Android vector drawables
which will make the process of creating these icons much easier
(and perhaps automate-able).
TODO: actually craft a 2x version instead of reusing 1x.
4. Apply this all to the app menu. This solves the issue of pixel
perfection in the steady state.
BUG=718549,704786
==========
to
==========
Use animated vector icon for app menu notification animation.
1. Expose IconDescription in vector icon code so when we add or
modify parameters we don't have to update the API.
TODO: remove the parts of the API that use multiple parameters
and replace them all with IconDescription.
2. Add AnimatedIconView, which caches the start and end states and
controls animating in between as well.
3. Add an animated version of browser_tools.icon. This was crafted
by hand based on the non-animated version and a visual description,
but in the future designers should deliver Android vector drawables
which will make the process of creating these icons much easier
(and perhaps automate-able).
TODO: actually craft a 2x version instead of reusing 1x.
4. Apply this all to the app menu. This solves the issue of pixel
perfection in the steady state.
BUG=718549,704786
Review-Url: https://codereview.chromium.org/2892563004
Cr-Commit-Position: refs/heads/master@{#476425}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d068dc8a60aea79f24f02f6a659...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/9d068dc8a60aea79f24f02f6a659... |
