Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972923002:
Update voice interaction burst animation (Closed)
DescriptionUpdate voice interaction burst animation
BUG=b:35624617
TEST=build and flash locally, saw the new animation
Review-Url: https://codereview.chromium.org/2972923002
Cr-Commit-Position: refs/heads/master@{#487128}
Committed: https://chromium.googlesource.com/chromium/src/+/4d5e7b4bd61189728e95142c83ed285b74178c26
Patch Set 1 #Patch Set 2 : update formating #
Total comments: 21
Patch Set 3 : fix comments #
Total comments: 30
Patch Set 4 : fix comments #Patch Set 5 : fix comments #
Total comments: 1
Messages
Total messages: 17 (5 generated)
xiaohuic@chromium.org changed reviewers: + oshima@chromium.org
This cl changes the burst animation from original fade out to a circle to rounded corner square transformation. The transformation is done by using 4 circles and 2 rectangles that transform at the same time, which resulted in the look of a circle changed to a rectangle. The transformation calculation logic is mostly borrowing from ui::views::SquareInkDropRipple https://cs.chromium.org/chromium/src/ui/views/animation/square_ink_drop_ripple.h SquareInkDropRipple isn't reusable. I will see if it is possible to refactor out the transformation logic out it.
just half way through. I'll send the rest this afternoon. maybe having a utility function to scale the size to half may help? https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/app_list_butt... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/app_list_butt... ash/shelf/app_list_button.cc:93: voice_interaction_animation_hide_delay_timer_->Stop(); just curious. Timer's dtor automatically abandon the scheduled tasks and stop the time. Did you need these? https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:108: private: // ui::LayerDelegate: https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:126: enum Shape { CIRCLE, ROUNDED_RECT }; nit: enum class https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:131: gfx::Size(kBackgroundLargeWidthDip, kBackgroundLargeHeightDip)), looks like it's now ok to use brace initialization to initialize members or variables, so you may use {}. Sorry for wrong advice. I still prefer to specify the type when passing as an argument tho. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:165: void AnimateToLarge(const gfx::PointF new_center, PointF& https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:192: center_point_ = center_point_.SetPoint() https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:198: enum PaintedShape { enum class https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); so just setting transform didn't work? https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.h (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.h:34: bool IsBursting() { return is_bursting_; } is_bustring() const
xiaohuic@google.com changed reviewers: + xiaohuic@google.com
What do you mean by "utility function to scale the size to half"? https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/app_list_butt... File ash/shelf/app_list_button.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/app_list_butt... ash/shelf/app_list_button.cc:93: voice_interaction_animation_hide_delay_timer_->Stop(); On 2017/07/06 19:23:54, oshima wrote: > just curious. Timer's dtor automatically abandon the scheduled tasks and stop > the time. Did you need these? Maybe I don't, removed. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:108: private: On 2017/07/06 19:23:54, oshima wrote: > // ui::LayerDelegate: Done. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:126: enum Shape { CIRCLE, ROUNDED_RECT }; On 2017/07/06 19:23:55, oshima wrote: > nit: enum class Done. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:131: gfx::Size(kBackgroundLargeWidthDip, kBackgroundLargeHeightDip)), On 2017/07/06 19:23:55, oshima wrote: > looks like it's now ok to use brace initialization to initialize members or > variables, so you may use {}. Sorry for wrong advice. > > I still prefer to specify the type when passing as an argument tho. Acknowledged. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:165: void AnimateToLarge(const gfx::PointF new_center, On 2017/07/06 19:23:54, oshima wrote: > PointF& Done. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:192: center_point_ = On 2017/07/06 19:23:54, oshima wrote: > center_point_.SetPoint() Done. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:198: enum PaintedShape { On 2017/07/06 19:23:55, oshima wrote: > enum class Done. https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); On 2017/07/06 19:23:55, oshima wrote: > so just setting transform didn't work? It should work. It's more about supporting LayerAnimaitonObserver, e.g. CallbackLayerAnimationObserver. Let me know if you still want to change it, then we will create an ImplicitLayerAnimationObserver inner class and it should work too.
https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); On 2017/07/06 20:55:41, xiaohuic wrote: > On 2017/07/06 19:23:55, oshima wrote: > > so just setting transform didn't work? > > It should work. It's more about supporting LayerAnimaitonObserver, e.g. > CallbackLayerAnimationObserver. Let me know if you still want to change it, then > we will create an ImplicitLayerAnimationObserver inner class and it should work > too. will animator->AddObserver(animation_observer) work? https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:132: center_point_( I was thinking something like HalfSize(small_size_) and use this in other places that needs half size. Please disregard if it won't helpmuch. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:138: new views::RectangleLayerDelegate(SK_ColorWHITE, MakeUnique https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:146: ++i) alternatively, you can define const PaintedShape kAllShapes[] = {... }; then you can do for (auto shape : kAllShapes) { } https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:280: ripple_bounds = gfx::RectF(gfx::ToEnclosingRect(ripple_bounds)); gfx::ScaleToEnclosingRect? https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:423: painted_layers_[static_cast<int>(PaintedShape::PAINTED_SHAPE_COUNT)]; alternatively, you can do std::map<PaintedShape, std::unique_ptr<ui::layer>> painted_layers; and avoid casts. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:427: const gfx::Size small_size_; looks like SizeF is better choice? https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:450: is_bursting_(false), initialize show_icons_ and should_hide_animation_ (or in header) https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:727: should_hide_animation_ = true; should this be inside following if, and reset after if? https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.h (right): https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.h:34: bool IsBursting() const { return is_bursting_; } is_bursting() const https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.h:52: bool should_hide_animation_; can you document this?
by the way, this probably won't work in RTL. No need to handle it now, but please add TODO to address it.
On 2017/07/06 22:06:24, oshima wrote: > by the way, this probably won't work in RTL. No need to handle it now, but > please add TODO to address it. Added
https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); It causes a crash. I think it requires the caller to also call RemoveObserver() since the layer is long-lived. Unlike using LayerAnimationSequence, which destroys the whole sequence so there is no need to explicitly remove observers. Received signal 11 <unknown> 000000000000 #0 0x7f26cd5493fb base::debug::StackTrace::StackTrace() #1 0x7f26cd54813c base::debug::StackTrace::StackTrace() #2 0x7f26cd548f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f26cd9b0330 <unknown> #4 0x7f26c28f2e74 std::less<>::operator()() #5 0x7f26c28f2e04 std::_Rb_tree<>::_M_lower_bound() #6 0x7f26c28f2d41 std::_Rb_tree<>::find() #7 0x7f26c28f2cdd std::__cxx1998::set<>::find() #8 0x7f26c28f1e15 std::__debug::set<>::find() #9 0x7f26c28f10d6 ui::LayerAnimationObserver::AttachedToSequence() #10 0x7f26c28f61f4 ui::LayerAnimationSequence::AddObserver() #11 0x7f26c28ff005 ui::LayerAnimator::OnScheduled() #12 0x7f26c28fda07 ui::LayerAnimator::StartAnimation() #13 0x7f26c28fd855 ui::LayerAnimator::SetTransform() #14 0x7f26c28d34fc ui::Layer::SetTransform() #15 0x7f26be302d8f ash::VoiceInteractionIconBackground::AnimateToTransforms() #16 0x7f26be3011d1 ash::VoiceInteractionIconBackground::AnimateToLarge() #17 0x7f26be2ffd0e ash::VoiceInteractionOverlay::BurstAnimation() https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:132: center_point_( On 2017/07/06 22:04:42, oshima wrote: > I was thinking something like > > HalfSize(small_size_) > > and use this in other places that needs half size. Please disregard if it won't > helpmuch. I see, some of the places are getting points, some of the places are just taking int/float, I don't think it would help much and the math is simple anyway. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:138: new views::RectangleLayerDelegate(SK_ColorWHITE, On 2017/07/06 22:04:42, oshima wrote: > MakeUnique Done. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:146: ++i) On 2017/07/06 22:04:42, oshima wrote: > alternatively, you can define > > const PaintedShape kAllShapes[] = {... }; > > then you can do > > for (auto shape : kAllShapes) { > } Done. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:280: ripple_bounds = gfx::RectF(gfx::ToEnclosingRect(ripple_bounds)); On 2017/07/06 22:04:42, oshima wrote: > gfx::ScaleToEnclosingRect? This one does not support RectF https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:423: painted_layers_[static_cast<int>(PaintedShape::PAINTED_SHAPE_COUNT)]; On 2017/07/06 22:04:42, oshima wrote: > alternatively, you can do > > std::map<PaintedShape, std::unique_ptr<ui::layer>> painted_layers; > > and avoid casts. isn't map much slower than just indexing an array? also we need the index to also index into the transformations generated at run time. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:427: const gfx::Size small_size_; On 2017/07/06 22:04:42, oshima wrote: > looks like SizeF is better choice? I have to use this to set the layer bounds, which only takes Rect. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:450: is_bursting_(false), On 2017/07/06 22:04:42, oshima wrote: > initialize show_icons_ and should_hide_animation_ (or in header) Done. Generally is it preferred to init here or in header? https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:727: should_hide_animation_ = true; On 2017/07/06 22:04:42, oshima wrote: > should this be inside following if, and reset after if? Done.
https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); On 2017/07/07 16:38:14, xc wrote: > It causes a crash. I think it requires the caller to also call RemoveObserver() > since the layer is long-lived. Unlike using LayerAnimationSequence, which > destroys the whole sequence so there is no need to explicitly remove observers. > > > Received signal 11 <unknown> 000000000000 > #0 0x7f26cd5493fb base::debug::StackTrace::StackTrace() > #1 0x7f26cd54813c base::debug::StackTrace::StackTrace() > #2 0x7f26cd548f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() > #3 0x7f26cd9b0330 <unknown> > #4 0x7f26c28f2e74 std::less<>::operator()() > #5 0x7f26c28f2e04 std::_Rb_tree<>::_M_lower_bound() > #6 0x7f26c28f2d41 std::_Rb_tree<>::find() > #7 0x7f26c28f2cdd std::__cxx1998::set<>::find() > #8 0x7f26c28f1e15 std::__debug::set<>::find() > #9 0x7f26c28f10d6 ui::LayerAnimationObserver::AttachedToSequence() > #10 0x7f26c28f61f4 ui::LayerAnimationSequence::AddObserver() > #11 0x7f26c28ff005 ui::LayerAnimator::OnScheduled() > #12 0x7f26c28fda07 ui::LayerAnimator::StartAnimation() > #13 0x7f26c28fd855 ui::LayerAnimator::SetTransform() > #14 0x7f26c28d34fc ui::Layer::SetTransform() > #15 0x7f26be302d8f ash::VoiceInteractionIconBackground::AnimateToTransforms() > #16 0x7f26be3011d1 ash::VoiceInteractionIconBackground::AnimateToLarge() > #17 0x7f26be2ffd0e ash::VoiceInteractionOverlay::BurstAnimation() I see, looks like CallbackLayerAnimatorObserver is designed this way. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:207: }; How about moving this to anonymous class, and define kAllShapes a constxepr? Then you can use arraysize(kAllShapes) and remove PAINTED_SHAPE_COUNT. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:246: ++i) kAllShapse https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:252: ++i) ditto https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:280: ripple_bounds = gfx::RectF(gfx::ToEnclosingRect(ripple_bounds)); On 2017/07/07 16:38:15, xc wrote: > On 2017/07/06 22:04:42, oshima wrote: > > gfx::ScaleToEnclosingRect? > > This one does not support RectF Doh, too bad. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:423: painted_layers_[static_cast<int>(PaintedShape::PAINTED_SHAPE_COUNT)]; On 2017/07/07 16:38:15, xc wrote: > On 2017/07/06 22:04:42, oshima wrote: > > alternatively, you can do > > > > std::map<PaintedShape, std::unique_ptr<ui::layer>> painted_layers; > > > > and avoid casts. > > isn't map much slower than just indexing an array? > also we need the index to also index into the transformations generated at run > time. std::map is implemented as a tree and accessing it in this size should be pretty fast, although I think we probably should just go back to enum, as you want to access using index, and using class requires casts. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:450: is_bursting_(false), On 2017/07/07 16:38:15, xc wrote: > On 2017/07/06 22:04:42, oshima wrote: > > initialize show_icons_ and should_hide_animation_ (or in header) > > Done. Generally is it preferred to init here or in header? I usually initialize bool/(null)pointer in header, but others in constructor, but i'm fine with either way.
https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/20001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:355: duration); On 2017/07/07 19:15:36, oshima wrote: > On 2017/07/07 16:38:14, xc wrote: > > It causes a crash. I think it requires the caller to also call > RemoveObserver() > > since the layer is long-lived. Unlike using LayerAnimationSequence, which > > destroys the whole sequence so there is no need to explicitly remove > observers. > > > > > > Received signal 11 <unknown> 000000000000 > > #0 0x7f26cd5493fb base::debug::StackTrace::StackTrace() > > #1 0x7f26cd54813c base::debug::StackTrace::StackTrace() > > #2 0x7f26cd548f0f base::debug::(anonymous namespace)::StackDumpSignalHandler() > > #3 0x7f26cd9b0330 <unknown> > > #4 0x7f26c28f2e74 std::less<>::operator()() > > #5 0x7f26c28f2e04 std::_Rb_tree<>::_M_lower_bound() > > #6 0x7f26c28f2d41 std::_Rb_tree<>::find() > > #7 0x7f26c28f2cdd std::__cxx1998::set<>::find() > > #8 0x7f26c28f1e15 std::__debug::set<>::find() > > #9 0x7f26c28f10d6 ui::LayerAnimationObserver::AttachedToSequence() > > #10 0x7f26c28f61f4 ui::LayerAnimationSequence::AddObserver() > > #11 0x7f26c28ff005 ui::LayerAnimator::OnScheduled() > > #12 0x7f26c28fda07 ui::LayerAnimator::StartAnimation() > > #13 0x7f26c28fd855 ui::LayerAnimator::SetTransform() > > #14 0x7f26c28d34fc ui::Layer::SetTransform() > > #15 0x7f26be302d8f ash::VoiceInteractionIconBackground::AnimateToTransforms() > > #16 0x7f26be3011d1 ash::VoiceInteractionIconBackground::AnimateToLarge() > > #17 0x7f26be2ffd0e ash::VoiceInteractionOverlay::BurstAnimation() > > I see, looks like CallbackLayerAnimatorObserver is designed this way. Acknowledged. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:207: }; On 2017/07/07 19:15:38, oshima wrote: > How about moving this to anonymous class, and define kAllShapes a constxepr? > Then you can use arraysize(kAllShapes) and remove PAINTED_SHAPE_COUNT. changed back to enum. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:246: ++i) On 2017/07/07 19:15:37, oshima wrote: > kAllShapse changed back to enum https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:252: ++i) On 2017/07/07 19:15:37, oshima wrote: > ditto changed back to enum https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:280: ripple_bounds = gfx::RectF(gfx::ToEnclosingRect(ripple_bounds)); On 2017/07/07 19:15:38, oshima wrote: > On 2017/07/07 16:38:15, xc wrote: > > On 2017/07/06 22:04:42, oshima wrote: > > > gfx::ScaleToEnclosingRect? > > > > This one does not support RectF > > Doh, too bad. Acknowledged. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:423: painted_layers_[static_cast<int>(PaintedShape::PAINTED_SHAPE_COUNT)]; On 2017/07/07 19:15:39, oshima wrote: > On 2017/07/07 16:38:15, xc wrote: > > On 2017/07/06 22:04:42, oshima wrote: > > > alternatively, you can do > > > > > > std::map<PaintedShape, std::unique_ptr<ui::layer>> painted_layers; > > > > > > and avoid casts. > > > > isn't map much slower than just indexing an array? > > also we need the index to also index into the transformations generated at run > > time. > > std::map is implemented as a tree and accessing it in this size should be pretty > fast, although I think we probably should just go back to enum, as you want to > access using index, and using class requires casts. changed back to enum as discussed offline. https://codereview.chromium.org/2972923002/diff/40001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:450: is_bursting_(false), On 2017/07/07 19:15:38, oshima wrote: > On 2017/07/07 16:38:15, xc wrote: > > On 2017/07/06 22:04:42, oshima wrote: > > > initialize show_icons_ and should_hide_animation_ (or in header) > > > > Done. Generally is it preferred to init here or in header? > > I usually initialize bool/(null)pointer in header, but others in constructor, > but i'm fine with either way. Acknowledged.
lgtm https://codereview.chromium.org/2972923002/diff/80001/ash/shelf/voice_interac... File ash/shelf/voice_interaction_overlay.cc (right): https://codereview.chromium.org/2972923002/diff/80001/ash/shelf/voice_interac... ash/shelf/voice_interaction_overlay.cc:205: PAINTED_SHAPE_COUNT Okay, sorry I wasn't clear. I'd say this is optional, but still recommended. you can avoid this extra enum by moving this to anonymous namespace and define constexpr kAllShapes = {...}; You can use arraysize(kAllShapes) to define arrays, and eliminate PAINTED_SHAPE_COUNT. You can also replace int loop with "for (auto shape : kAllShapes) {", and no extra case.
The CQ bit was checked by xiaohuic@chromium.org
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": 80001, "attempt_start_ts": 1500309169973790,
"parent_rev": "0649c1bc482d0d0ee07dce3e12c4f62cce042dd8", "commit_rev":
"4d5e7b4bd61189728e95142c83ed285b74178c26"}
Message was sent while issue was closed.
Description was changed from ========== Update voice interaction burst animation BUG=b:35624617 TEST=build and flash locally, saw the new animation ========== to ========== Update voice interaction burst animation BUG=b:35624617 TEST=build and flash locally, saw the new animation Review-Url: https://codereview.chromium.org/2972923002 Cr-Commit-Position: refs/heads/master@{#487128} Committed: https://chromium.googlesource.com/chromium/src/+/4d5e7b4bd61189728e95142c83ed... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4d5e7b4bd61189728e95142c83ed... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
