Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789203003:
[Views] App Menu Animated Icon (Closed)
Description[Views] App Menu Animated Icon
Implementation of the new app menu animated icon.
The new icon is set behind the "app-menu-icon" flag.
The flag gives us three options:
1) Old Behavior - Shows the old non-animated icon
2) Persistent Opened State - Shows the new icon which animated and stays in
an opened state. The animation is only triggered when a new window is created
3) Persistent Closed State - Shows the new icon which animates to an opened
state before it animates back to a closed state. The animation is triggered when
a new window/tab is created and when the menu is opened.
The flag's default option is the "Old Behavior"
BUG=704786
Review-Url: https://codereview.chromium.org/2789203003
Cr-Commit-Position: refs/heads/master@{#465267}
Committed: https://chromium.googlesource.com/chromium/src/+/064a811986ce8b3bdcb9bd134d4927b9b36fee2a
Patch Set 1 #Patch Set 2 : Cleaned up #
Total comments: 32
Patch Set 3 : Fixes for msw #
Total comments: 22
Patch Set 4 : Fixes for sky (and rebased) #
Total comments: 5
Patch Set 5 : Fix for danakj #Patch Set 6 : Added layer #
Total comments: 24
Patch Set 7 : Fix for msw #
Total comments: 6
Patch Set 8 : Fixes for msw #Patch Set 9 : nit #Messages
Total messages: 132 (107 generated)
Description was changed from ========== Clean up Just need to figure out the animation icon logic Cleaned up I think I fixed it yay Hooked the color change Flipped the closing dot order. Okay it works but it could be better. Actually I should change it :/ Close duration is there now Changed the format of the timing values Cleaned up a bit Used tween functions. Also the color transforms now Tween added in stroke is not animated slightly differently than width Animate the duration of each dot differently Created a dot class Starting to separate out the dots Added in the correct dimensions Animated the width yay Switched to rounded rect Animation stuff is starting to be implemented Changed the app menu drawing class Created a file just for painting Drew all three dots. They are a bit off though unfortunately Drawing the other dots but it feels off? Drawing the middle circle BUG= ========== to ========== ==========
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: 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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: 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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:80001) 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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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 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...
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== ========== to ========== [Views] Global Menu Icon BUG=704786 ==========
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 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...
Patchset #2 (id:120001) has been deleted
Description was changed from ========== [Views] Global Menu Icon BUG=704786 ========== to ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new global BUG=704786 ==========
Description was changed from ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new global BUG=704786 ========== to ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state const char kGlobalErrorMenuIconName[] = "Global Error Menu Icon"; 2730 2731 const char kGlobalErrorMenuIconDescription[] = 2732 "Changes the global error icon in the wrench menu."; 2733 2734 const char kGlobalErrorMenuIconOldBehavior[] = "Old Behavior"; 2735 2736 const char kGlobalErrorMenuIconPersistentOpenedState[] = 2737 "Persistent Opened State"; 2738 2739 const char kGlobalErrorMenuIconPersistentClosedState[] = 2740 "Persistent Closed State" BUG=704786 ==========
Description was changed from ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state const char kGlobalErrorMenuIconName[] = "Global Error Menu Icon"; 2730 2731 const char kGlobalErrorMenuIconDescription[] = 2732 "Changes the global error icon in the wrench menu."; 2733 2734 const char kGlobalErrorMenuIconOldBehavior[] = "Old Behavior"; 2735 2736 const char kGlobalErrorMenuIconPersistentOpenedState[] = 2737 "Persistent Opened State"; 2738 2739 const char kGlobalErrorMenuIconPersistentClosedState[] = 2740 "Persistent Closed State" BUG=704786 ========== to ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state BUG=704786 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:140001) 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...
Description was changed from ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state BUG=704786 ========== to ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu animated icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state. The animation is only triggered when a new window is created 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state. The animation is triggered when a new window/tab is created and when the menu is opened. The flag's default option is the "Old Behavior" BUG=704786 ==========
spqchan@chromium.org changed reviewers: + msw@chromium.org
Patchset #2 (id:160001) 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: This issue passed the CQ dry run.
Patchset #2 (id:180001) has been deleted
PTAL
On 2017/04/08 01:04:28, spqchan wrote: > PTAL ping
Sorry for the delay, that's a lot of confusing animation code, especially with the different units of measurement (literals vs. percentages for both times and sizes). Honestly, I feel like someone with more animation familiarity should take a look at AppMenuAnimation, they'd probably have better feedback than myself. Perhaps Scott (CC'ed) can suggest an extra reviewer? https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:117: delegate_->UpdateSeverity(IconType::NONE, Severity::NONE, false); Why change |animate| true->false here? https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:7: #include "base/time/time.h" q: needed? https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:19: const float kOpenDuration = 733.0f; nit: constexpr for all these https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:55: float widthOpenInterval, Use the |unix_hacker| naming convention for anything that is not a kConstantValue (arguments, members, etc.). There are lots of identifiers that need to be fixed, please do a careful audit. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:61: void AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_pt, nit: |center_point| https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:75: float delay = is_opening ? delay_ : kDotDelayMs * 2 - delay_; nit: use parens around (kDotDelayMs * 2 - delay_) for clarity https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:77: float widthProgress = 0.0; Use the |unix_hacker| naming convention for these: |width_progress|, etc. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:80: float progress = animation->GetCurrentValue() - (delay / totalDuration); nit: move this up immediately after |delay|, for clarity https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:22: explicit AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); nit: no explicit for two args https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:53: // % of time it takes for each dot to animate to its width and stroke in nit: "The percent of the slide animation's time taken for each dot..." https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:61: // True if the animation close after it finished opening. nit: "should close" and "finishes" https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:74: // The severity color of the dots. This is expected color at the end of the nit: "This is the final color at the..." https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:62: animation_.reset(new AppMenuAnimation(this, true)); nit: = base::MakeUnique here and below https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:142: if (animation_.get()) { nit: remove |.get()|, you can just check |if (animation_)|, ditto below https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:68: void UpdateIcon(bool should_animate); nit: describe |should_animate| in the comment above.
One more question https://codereview.chromium.org/2789203003/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2789203003/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101308: + <int value="950287809" label="global-app-menu"/> Is this used at all?
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
sky@chromium.org changed reviewers: + danakj@chromium.org, sky@chromium.org
I can't think of anything better in AppMenuAnimation. +dankj in case she has suggestions. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:53: AppMenuAnimation::AppMenuDot::AppMenuDot(float delay, Use TimeDelta https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:60: void AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_point, const & https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:65: gfx::SlideAnimation* animation) { const gfx::SlideAnimation* if possible. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:131: start_color_(gfx::kPlaceholderColor), Can the real colors be passed in rather than set later on? https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:10: #include "ui/gfx/animation/slide_animation.h" Generally we use unique_ptr and forward declare members (see chromium style guide for specifics). https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:22: AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); I suggest changing AppMenuButton to View* as the only thing you need is called SchedulePaint, which is in View. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:28: void UpdateIconColor(SkColor start_color, SkColor severity_color); SetIconColors? https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:53: float delay_; const where possible. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:59: }; DISALLOW... https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:64: bool should_animation_close_; should_animate_closed_? https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:79: }; DISALLOW...
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
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...
Patchset #4 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
This seems good, but one thing I suggest is to do what "will-change:content" would do for a web element that indicates it will be animating its contents, that is to make the owning view get its own layer when the animation is active. https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:68: views::View* owner_; // weak. Comments are complete sentences, starting with a capital. Here, I think weak is implied since it's a raw pointer FWIW. https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:68: views::View* owner_; // weak. View* const owner_ https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:71: bool should_animate_closed_; const
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...
Thanks all! I addressed all of your comments > This seems good, but one thing I suggest is to do what "will-change:content" would do for a web element that indicates it will be animating its contents, that is to make the owning view get its own layer when the animation is active. Can you elaborate more on that? What do you mean by the owning view gets its own layer? Should I do something like SetPaintToLayer()? https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/tool... File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/tool... chrome/browser/ui/toolbar/app_menu_icon_controller.cc:117: delegate_->UpdateSeverity(IconType::NONE, Severity::NONE, false); On 2017/04/11 17:34:13, msw wrote: > Why change |animate| true->false here? The |animate| param was actually dead code before I made this change. For the new icon, it should not animate if the security level is NONE https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:7: #include "base/time/time.h" On 2017/04/11 17:34:13, msw wrote: > q: needed? Removed https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:19: const float kOpenDuration = 733.0f; On 2017/04/11 17:34:13, msw wrote: > nit: constexpr for all these Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:55: float widthOpenInterval, On 2017/04/11 17:34:13, msw wrote: > Use the |unix_hacker| naming convention for anything that is not a > kConstantValue (arguments, members, etc.). There are lots of identifiers that > need to be fixed, please do a careful audit. Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:61: void AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_pt, On 2017/04/11 17:34:13, msw wrote: > nit: |center_point| Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:75: float delay = is_opening ? delay_ : kDotDelayMs * 2 - delay_; On 2017/04/11 17:34:13, msw wrote: > nit: use parens around (kDotDelayMs * 2 - delay_) for clarity Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:77: float widthProgress = 0.0; On 2017/04/11 17:34:13, msw wrote: > Use the |unix_hacker| naming convention for these: |width_progress|, etc. Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:80: float progress = animation->GetCurrentValue() - (delay / totalDuration); On 2017/04/11 17:34:13, msw wrote: > nit: move this up immediately after |delay|, for clarity Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:22: explicit AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); On 2017/04/11 17:34:13, msw wrote: > nit: no explicit for two args Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:53: // % of time it takes for each dot to animate to its width and stroke in On 2017/04/11 17:34:13, msw wrote: > nit: "The percent of the slide animation's time taken for each dot..." Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:61: // True if the animation close after it finished opening. On 2017/04/11 17:34:13, msw wrote: > nit: "should close" and "finishes" Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:74: // The severity color of the dots. This is expected color at the end of the On 2017/04/11 17:34:13, msw wrote: > nit: "This is the final color at the..." Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:62: animation_.reset(new AppMenuAnimation(this, true)); On 2017/04/11 17:34:14, msw wrote: > nit: = base::MakeUnique here and below Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:142: if (animation_.get()) { On 2017/04/11 17:34:14, msw wrote: > nit: remove |.get()|, you can just check |if (animation_)|, ditto below Done. https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.h (right): https://codereview.chromium.org/2789203003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.h:68: void UpdateIcon(bool should_animate); On 2017/04/11 17:34:14, msw wrote: > nit: describe |should_animate| in the comment above. Done. https://codereview.chromium.org/2789203003/diff/200001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2789203003/diff/200001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:101308: + <int value="950287809" label="global-app-menu"/> On 2017/04/11 17:36:08, msw wrote: > Is this used at all? This is required: https://cs.chromium.org/chromium/src/chrome/browser/about_flags.cc?rcl=b93e97... https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:53: AppMenuAnimation::AppMenuDot::AppMenuDot(float delay, On 2017/04/11 20:52:00, sky wrote: > Use TimeDelta Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:60: void AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_point, On 2017/04/11 20:52:00, sky wrote: > const & Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:65: gfx::SlideAnimation* animation) { On 2017/04/11 20:52:00, sky wrote: > const gfx::SlideAnimation* if possible. Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:131: start_color_(gfx::kPlaceholderColor), On 2017/04/11 20:52:00, sky wrote: > Can the real colors be passed in rather than set later on? No, the severity levels aren't set until after the app menu button is created. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:10: #include "ui/gfx/animation/slide_animation.h" On 2017/04/11 20:52:00, sky wrote: > Generally we use unique_ptr and forward declare members (see chromium style > guide for specifics). Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:22: AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); On 2017/04/11 20:52:00, sky wrote: > I suggest changing AppMenuButton to View* as the only thing you need is called > SchedulePaint, which is in View. Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:28: void UpdateIconColor(SkColor start_color, SkColor severity_color); On 2017/04/11 20:52:00, sky wrote: > SetIconColors? Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:53: float delay_; On 2017/04/11 20:52:00, sky wrote: > const where possible. Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:59: }; On 2017/04/11 20:52:00, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:64: bool should_animation_close_; On 2017/04/11 20:52:00, sky wrote: > should_animate_closed_? Done. https://codereview.chromium.org/2789203003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:79: }; On 2017/04/11 20:52:00, sky wrote: > DISALLOW... Done. https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:68: views::View* owner_; // weak. On 2017/04/12 14:38:39, danakj (out sick) wrote: > View* const owner_ Done. https://codereview.chromium.org/2789203003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:71: bool should_animate_closed_; On 2017/04/12 14:38:39, danakj (out sick) wrote: > const Done.
On Wed, Apr 12, 2017 at 12:42 PM, <spqchan@chromium.org> wrote: > Thanks all! I addressed all of your comments > > > This seems good, but one thing I suggest is to do what > "will-change:content" > would do for a web element that indicates it will be animating its > contents, > that is to make the owning view get its own layer when the animation is > active. > > Can you elaborate more on that? What do you mean by the owning view gets > its own > layer? Should I do something like SetPaintToLayer()? > Exactly. When the animation starts call SetPaintToLayer(), and when the animation completes call DestroyLayer(). > > > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/toolbar/app_menu_icon_controller.cc > File chrome/browser/ui/toolbar/app_menu_icon_controller.cc (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/toolbar/app_menu_icon_controller.cc#newcode117 > chrome/browser/ui/toolbar/app_menu_icon_controller.cc:117: > delegate_->UpdateSeverity(IconType::NONE, Severity::NONE, false); > On 2017/04/11 17:34:13, msw wrote: > > Why change |animate| true->false here? > > The |animate| param was actually dead code before I made this change. > For the new icon, it should not animate if the security level is NONE > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc > File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode7 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:7: #include > "base/time/time.h" > On 2017/04/11 17:34:13, msw wrote: > > q: needed? > > Removed > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode19 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:19: const float > kOpenDuration = 733.0f; > On 2017/04/11 17:34:13, msw wrote: > > nit: constexpr for all these > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode55 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:55: float > widthOpenInterval, > On 2017/04/11 17:34:13, msw wrote: > > Use the |unix_hacker| naming convention for anything that is not a > > kConstantValue (arguments, members, etc.). There are lots of > identifiers that > > need to be fixed, please do a careful audit. > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode61 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:61: void > AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_pt, > On 2017/04/11 17:34:13, msw wrote: > > nit: |center_point| > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode75 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:75: float delay = > is_opening ? delay_ : kDotDelayMs * 2 - delay_; > On 2017/04/11 17:34:13, msw wrote: > > nit: use parens around (kDotDelayMs * 2 - delay_) for clarity > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode77 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:77: float > widthProgress = 0.0; > On 2017/04/11 17:34:13, msw wrote: > > Use the |unix_hacker| naming convention for these: |width_progress|, > etc. > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode80 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:80: float progress > = animation->GetCurrentValue() - (delay / totalDuration); > On 2017/04/11 17:34:13, msw wrote: > > nit: move this up immediately after |delay|, for clarity > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h > File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode22 > chrome/browser/ui/views/toolbar/app_menu_animation.h:22: explicit > AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); > On 2017/04/11 17:34:13, msw wrote: > > nit: no explicit for two args > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode53 > chrome/browser/ui/views/toolbar/app_menu_animation.h:53: // % of time it > takes for each dot to animate to its width and stroke in > On 2017/04/11 17:34:13, msw wrote: > > nit: "The percent of the slide animation's time taken for each dot..." > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode61 > chrome/browser/ui/views/toolbar/app_menu_animation.h:61: // True if the > animation close after it finished opening. > On 2017/04/11 17:34:13, msw wrote: > > nit: "should close" and "finishes" > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode74 > chrome/browser/ui/views/toolbar/app_menu_animation.h:74: // The severity > color of the dots. This is expected color at the end of the > On 2017/04/11 17:34:13, msw wrote: > > nit: "This is the final color at the..." > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_button.cc > File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_button.cc#newcode62 > chrome/browser/ui/views/toolbar/app_menu_button.cc:62: > animation_.reset(new AppMenuAnimation(this, true)); > On 2017/04/11 17:34:14, msw wrote: > > nit: = base::MakeUnique here and below > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_button.cc#newcode142 > chrome/browser/ui/views/toolbar/app_menu_button.cc:142: if > (animation_.get()) { > On 2017/04/11 17:34:14, msw wrote: > > nit: remove |.get()|, you can just check |if (animation_)|, ditto > below > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_button.h > File chrome/browser/ui/views/toolbar/app_menu_button.h (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > chrome/browser/ui/views/toolbar/app_menu_button.h#newcode68 > chrome/browser/ui/views/toolbar/app_menu_button.h:68: void > UpdateIcon(bool should_animate); > On 2017/04/11 17:34:14, msw wrote: > > nit: describe |should_animate| in the comment above. > > Done. > > https://codereview.chromium.org/2789203003/diff/200001/ > tools/metrics/histograms/histograms.xml > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2789203003/diff/200001/ > tools/metrics/histograms/histograms.xml#newcode101308 > tools/metrics/histograms/histograms.xml:101308: + <int > value="950287809" label="global-app-menu"/> > On 2017/04/11 17:36:08, msw wrote: > > Is this used at all? > > This is required: > https://cs.chromium.org/chromium/src/chrome/browser/about_flags.cc?rcl= > b93e979abdf9e47f3063b82e18bdecb56fc1d0bd&l=850 > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc > File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode53 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:53: > AppMenuAnimation::AppMenuDot::AppMenuDot(float delay, > On 2017/04/11 20:52:00, sky wrote: > > Use TimeDelta > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode60 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:60: void > AppMenuAnimation::AppMenuDot::Paint(gfx::PointF center_point, > On 2017/04/11 20:52:00, sky wrote: > > const & > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode65 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:65: > gfx::SlideAnimation* animation) { > On 2017/04/11 20:52:00, sky wrote: > > const gfx::SlideAnimation* if possible. > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.cc#newcode131 > chrome/browser/ui/views/toolbar/app_menu_animation.cc:131: > start_color_(gfx::kPlaceholderColor), > On 2017/04/11 20:52:00, sky wrote: > > Can the real colors be passed in rather than set later on? > > No, the severity levels aren't set until after the app menu button is > created. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h > File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode10 > chrome/browser/ui/views/toolbar/app_menu_animation.h:10: #include > "ui/gfx/animation/slide_animation.h" > On 2017/04/11 20:52:00, sky wrote: > > Generally we use unique_ptr and forward declare members (see chromium > style > > guide for specifics). > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode22 > chrome/browser/ui/views/toolbar/app_menu_animation.h:22: > AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); > On 2017/04/11 20:52:00, sky wrote: > > I suggest changing AppMenuButton to View* as the only thing you need > is called > > SchedulePaint, which is in View. > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode28 > chrome/browser/ui/views/toolbar/app_menu_animation.h:28: void > UpdateIconColor(SkColor start_color, SkColor severity_color); > On 2017/04/11 20:52:00, sky wrote: > > SetIconColors? > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode53 > chrome/browser/ui/views/toolbar/app_menu_animation.h:53: float delay_; > On 2017/04/11 20:52:00, sky wrote: > > const where possible. > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode59 > chrome/browser/ui/views/toolbar/app_menu_animation.h:59: }; > On 2017/04/11 20:52:00, sky wrote: > > DISALLOW... > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode64 > chrome/browser/ui/views/toolbar/app_menu_animation.h:64: bool > should_animation_close_; > On 2017/04/11 20:52:00, sky wrote: > > should_animate_closed_? > > Done. > > https://codereview.chromium.org/2789203003/diff/220001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode79 > chrome/browser/ui/views/toolbar/app_menu_animation.h:79: }; > On 2017/04/11 20:52:00, sky wrote: > > DISALLOW... > > Done. > > https://codereview.chromium.org/2789203003/diff/260001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h > File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): > > https://codereview.chromium.org/2789203003/diff/260001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode68 > chrome/browser/ui/views/toolbar/app_menu_animation.h:68: views::View* > owner_; // weak. > On 2017/04/12 14:38:39, danakj (out sick) wrote: > > View* const owner_ > > Done. > > https://codereview.chromium.org/2789203003/diff/260001/ > chrome/browser/ui/views/toolbar/app_menu_animation.h#newcode71 > chrome/browser/ui/views/toolbar/app_menu_animation.h:71: bool > should_animate_closed_; > On 2017/04/12 14:38:39, danakj (out sick) wrote: > > const > > Done. > > https://codereview.chromium.org/2789203003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the clarification! I called owner_->SetPaintToLayer(), however I'm not painting the background, I ended up with this result: https://screenshot.googleplex.com/nATaeWEahss Do you have any suggestions on what I can do for that? Also, what is the reasoning behind using SetPaintToLayer()?
On Wed, Apr 12, 2017 at 4:32 PM, <spqchan@chromium.org> wrote: > Thanks for the clarification! I called owner_->SetPaintToLayer(), however > I'm > not painting the background, I ended up with this result: > https://screenshot.googleplex.com/nATaeWEahss > Maybe you need to tell the layer it is not filling bounds opaquely. Do you have any suggestions on what I can do for that? Also, what is the > reasoning behind using SetPaintToLayer()? > When the layers bounds are about equal to the area being updated, it creates a more optimal situation as the compositor will have to do less work. Reason is when you're updating a small part of a larger layer, we may have to re-raster up to a whole tile (256x256 pixels), and we will always upload 256x256 pixels. Also the raster work involves all the views behind the current one if its not opaque and filling the whole tile. Whereas if the view is its own layer, the raster work can draw transparent and just draw that view on top of it, which is less overdraw (updating the same pixels multiple times) during raster. > > https://codereview.chromium.org/2789203003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #6 (id:300001) 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...
On Wed, Apr 12, 2017 at 1:57 PM, <danakj@chromium.org> wrote: > On Wed, Apr 12, 2017 at 4:32 PM, <spqchan@chromium.org> wrote: > >> Thanks for the clarification! I called owner_->SetPaintToLayer(), however >> I'm >> not painting the background, I ended up with this result: >> https://screenshot.googleplex.com/nATaeWEahss >> > > Maybe you need to tell the layer it is not filling bounds opaquely. > I suspect that is it. owner_->layer()->SetFillsBoundsOpaquely(false); > > Do you have any suggestions on what I can do for that? Also, what is the >> reasoning behind using SetPaintToLayer()? >> > > When the layers bounds are about equal to the area being updated, it > creates a more optimal situation as the compositor will have to do less > work. Reason is when you're updating a small part of a larger layer, we may > have to re-raster up to a whole tile (256x256 pixels), and we will always > upload 256x256 pixels. Also the raster work involves all the views behind > the current one if its not opaque and filling the whole tile. Whereas if > the view is its own layer, the raster work can draw transparent and just > draw that view on top of it, which is less overdraw (updating the same > pixels multiple times) during raster. > > >> >> https://codereview.chromium.org/2789203003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/12 22:46:04, sky (OOO) wrote: > On Wed, Apr 12, 2017 at 1:57 PM, <mailto:danakj@chromium.org> wrote: > > > On Wed, Apr 12, 2017 at 4:32 PM, <mailto:spqchan@chromium.org> wrote: > > > >> Thanks for the clarification! I called owner_->SetPaintToLayer(), however > >> I'm > >> not painting the background, I ended up with this result: > >> https://screenshot.googleplex.com/nATaeWEahss > >> > > > > Maybe you need to tell the layer it is not filling bounds opaquely. > > > > I suspect that is it. owner_->layer()->SetFillsBoundsOpaquely(false); > > > > > > Do you have any suggestions on what I can do for that? Also, what is the > >> reasoning behind using SetPaintToLayer()? > >> > > > > When the layers bounds are about equal to the area being updated, it > > creates a more optimal situation as the compositor will have to do less > > work. Reason is when you're updating a small part of a larger layer, we may > > have to re-raster up to a whole tile (256x256 pixels), and we will always > > upload 256x256 pixels. Also the raster work involves all the views behind > > the current one if its not opaque and filling the whole tile. Whereas if > > the view is its own layer, the raster work can draw transparent and just > > draw that view on top of it, which is less overdraw (updating the same > > pixels multiple times) during raster. > > > > > >> > >> https://codereview.chromium.org/2789203003/ > >> > > > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Thanks for the clear explanation! I added the layer
https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:544: const FeatureEntry::Choice kGlobalErrorMenuIconChoices[] = { nit: It's a bit odd to use the GlobalError name throughout these flags, since it seems like the global error icon is a different thing (IconType::GLOBAL_ERROR vs. IconType::UPGRADE_NOTIFICATION). It doesn't matter too much since this is a temporary flag for experimentation, but maybe consider renaming these something like AppMenuAnimation/UpgradeAnimation/UpdateAnimation/AppIconAnimation? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:2779: "Changes the global error icon in the wrench menu."; nit: s/wrench/app/? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:19: constexpr float kOpenDuration = 733.0f; nit: be consistent about the 'Ms' post-fix for duration constants. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:74: // Reverse the delay if the animation is closing. nit: rephrase this for a bit of clarity, 'reverse the delay' doesn't quite explain the intent. That said, I don't really like my own suggestion: "When the animation is closing, each dot uses the remainder of the full 2*kDotDelayMs delay period (0->2x, 1x->1x, 2x->0)." https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:77: : base::TimeDelta::FromMicroseconds(kDotDelayMs * 2) - delay_; Should this be FromMilliseconds? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:90: float color_delay = kColorDelayMs / total_duration; nit: should these two values use the 'interval' postfix (since they are %s)? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:133: middle_dot_(base::TimeDelta::FromMicroseconds(kDotDelayMs), Should this be FromMilliseconds? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:136: top_dot_(base::TimeDelta::FromMicroseconds(kDotDelayMs * 2), Should this be FromMilliseconds? https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:23: AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); nit: should_animate_closed to match cc. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:44: AppMenuDot(base::TimeDelta delay, nit: document these arguments https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:58: // The percentage of time it takes for each dot to animate to its width nit: "The percentage of the overall animation duration it takes" https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:66: AppMenuButton* const owner_; // Weak. nit: I agree, this comment isn't necessary.
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Patchset #7 (id:340001) 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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #7 (id:360001) 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...
PTAL https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/about_f... chrome/browser/about_flags.cc:544: const FeatureEntry::Choice kGlobalErrorMenuIconChoices[] = { On 2017/04/13 17:23:12, msw wrote: > nit: It's a bit odd to use the GlobalError name throughout these flags, since it > seems like the global error icon is a different thing (IconType::GLOBAL_ERROR > vs. IconType::UPGRADE_NOTIFICATION). It doesn't matter too much since this is a > temporary flag for experimentation, but maybe consider renaming these something > like AppMenuAnimation/UpgradeAnimation/UpdateAnimation/AppIconAnimation? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:2779: "Changes the global error icon in the wrench menu."; On 2017/04/13 17:23:12, msw wrote: > nit: s/wrench/app/? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:19: constexpr float kOpenDuration = 733.0f; On 2017/04/13 17:23:12, msw wrote: > nit: be consistent about the 'Ms' post-fix for duration constants. Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:74: // Reverse the delay if the animation is closing. On 2017/04/13 17:23:12, msw wrote: > nit: rephrase this for a bit of clarity, 'reverse the delay' doesn't quite > explain the intent. That said, I don't really like my own suggestion: "When the > animation is closing, each dot uses the remainder of the full 2*kDotDelayMs > delay period (0->2x, 1x->1x, 2x->0)." I...got nothing. Hope you don't mind me using your suggestion :) https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:77: : base::TimeDelta::FromMicroseconds(kDotDelayMs * 2) - delay_; On 2017/04/13 17:23:12, msw wrote: > Should this be FromMilliseconds? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:90: float color_delay = kColorDelayMs / total_duration; On 2017/04/13 17:23:12, msw wrote: > nit: should these two values use the 'interval' postfix (since they are %s)? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:133: middle_dot_(base::TimeDelta::FromMicroseconds(kDotDelayMs), On 2017/04/13 17:23:12, msw wrote: > Should this be FromMilliseconds? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:136: top_dot_(base::TimeDelta::FromMicroseconds(kDotDelayMs * 2), On 2017/04/13 17:23:12, msw wrote: > Should this be FromMilliseconds? Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:23: AppMenuAnimation(AppMenuButton* owner, bool should_animation_close); On 2017/04/13 17:23:12, msw wrote: > nit: should_animate_closed to match cc. Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:44: AppMenuDot(base::TimeDelta delay, On 2017/04/13 17:23:12, msw wrote: > nit: document these arguments Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:58: // The percentage of time it takes for each dot to animate to its width On 2017/04/13 17:23:12, msw wrote: > nit: "The percentage of the overall animation duration it takes" Done. https://codereview.chromium.org/2789203003/diff/320001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:66: AppMenuButton* const owner_; // Weak. On 2017/04/13 17:23:12, msw wrote: > nit: I agree, this comment isn't necessary. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with minor comments. https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:74: // When the animation is closing, the the remainder of the full 2*kDotDelayMs nit: s/the the/each dot uses the/ or similar https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:44: // The class constructor. The params |delay|, |width_open_interval|, and This doesn't explain the semantics of the arguments. I suppose the members are sufficiently documented and I should not have asked for separate explanations here; just remove this. https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:64: } else { Should this explicitly check if |flag == switches::kAppMenuIconPersistentOpenedState|, or do we want any value, like "--app-menu-icon=foobar" to just use the persistent opened behavior? Optionally remove the check for kAppMenuIconOldBehavior?
Thanks! https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.cc (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.cc:74: // When the animation is closing, the the remainder of the full 2*kDotDelayMs On 2017/04/13 23:05:24, msw wrote: > nit: s/the the/each dot uses the/ or similar Done. https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_animation.h (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_animation.h:44: // The class constructor. The params |delay|, |width_open_interval|, and On 2017/04/13 23:05:24, msw wrote: > This doesn't explain the semantics of the arguments. I suppose the members are > sufficiently documented and I should not have asked for separate explanations > here; just remove this. Done. https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... File chrome/browser/ui/views/toolbar/app_menu_button.cc (right): https://codereview.chromium.org/2789203003/diff/380001/chrome/browser/ui/view... chrome/browser/ui/views/toolbar/app_menu_button.cc:64: } else { On 2017/04/13 23:05:24, msw wrote: > Should this explicitly check if |flag == > switches::kAppMenuIconPersistentOpenedState|, or do we want any value, like > "--app-menu-icon=foobar" to just use the persistent opened behavior? Optionally > remove the check for kAppMenuIconOldBehavior? Done.
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2789203003/#ps420001 (title: "nit")
The CQ bit was unchecked by spqchan@chromium.org
Please update the CL title and description (not really global error related)
spqchan@chromium.org changed reviewers: + jwd@chromium.org
+jwd for histograms.xml owner
Description was changed from ========== [Views] Global Error Menu Animated Icon Implementation of the new global error menu animated icon. The new icon is set behind the "global-error-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state. The animation is only triggered when a new window is created 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state. The animation is triggered when a new window/tab is created and when the menu is opened. The flag's default option is the "Old Behavior" BUG=704786 ========== to ========== [Views] App Menu Animated Icon Implementation of the new app menu animated icon. The new icon is set behind the "app-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state. The animation is only triggered when a new window is created 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state. The animation is triggered when a new window/tab is created and when the menu is opened. The flag's default option is the "Old Behavior" BUG=704786 ==========
On 2017/04/13 23:49:51, spqchan wrote: > +jwd for histograms.xml owner ping on the CL
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: This issue passed the CQ dry run.
spqchan@chromium.org changed reviewers: + isherman@chromium.org - jwd@chromium.org
I forgot today is a holiday in Canada. +isherman for histogram.xml changes
histograms.xml lgtm
On 2017/04/18 06:10:08, Ilya Sherman wrote: > histograms.xml lgtm thanks!
On 2017/04/18 06:10:08, Ilya Sherman wrote: > histograms.xml lgtm thanks!
The CQ bit was checked by spqchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2789203003/#ps420001 (title: "nit")
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": 420001, "attempt_start_ts": 1492529025919800,
"parent_rev": "87648f2cf83870acac8d73ea6bb61a5dcf11b921", "commit_rev":
"064a811986ce8b3bdcb9bd134d4927b9b36fee2a"}
Message was sent while issue was closed.
Description was changed from ========== [Views] App Menu Animated Icon Implementation of the new app menu animated icon. The new icon is set behind the "app-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state. The animation is only triggered when a new window is created 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state. The animation is triggered when a new window/tab is created and when the menu is opened. The flag's default option is the "Old Behavior" BUG=704786 ========== to ========== [Views] App Menu Animated Icon Implementation of the new app menu animated icon. The new icon is set behind the "app-menu-icon" flag. The flag gives us three options: 1) Old Behavior - Shows the old non-animated icon 2) Persistent Opened State - Shows the new icon which animated and stays in an opened state. The animation is only triggered when a new window is created 3) Persistent Closed State - Shows the new icon which animates to an opened state before it animates back to a closed state. The animation is triggered when a new window/tab is created and when the menu is opened. The flag's default option is the "Old Behavior" BUG=704786 Review-Url: https://codereview.chromium.org/2789203003 Cr-Commit-Position: refs/heads/master@{#465267} Committed: https://chromium.googlesource.com/chromium/src/+/064a811986ce8b3bdcb9bd134d49... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:420001) as https://chromium.googlesource.com/chromium/src/+/064a811986ce8b3bdcb9bd134d49... |
