Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8826002:
Panels: Adding 3-step animation on minimize on Linux (Closed)
|
Created:
9 years ago by Dmitry Titov Modified:
9 years ago CC:
chromium-reviews, jennb, jianli, dcheng, prasadt Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPanels: Adding 3-step animation on minimize on Linux
BUG=104645
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113526
Patch Set 1 #Patch Set 2 : fix win build break #
Total comments: 14
Patch Set 3 : another speculative win fix #Patch Set 4 : more fixes for win #
Total comments: 22
Patch Set 5 : review feedback #
Total comments: 1
Patch Set 6 : patch for landing #Messages
Total messages: 15 (0 generated)
I've extracted the PanelSlideAnimation previously used in Win class into a separate file and use it from Linus as well. Prasad: for Linux and general Jian: for Win
http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:19: : ui::SlideAnimation(target), nit: 4 spaces for indenting. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:41: PanelSlideAnimation::~PanelSlideAnimation() { } nit: better to put into 2 lines. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:43: double PanelSlideAnimation::GetCurrentValue() const { It might be better to change this to a helper function so that it can also be used by Mac. Maybe a static public method defined in PanelSlideAnimation, like: static double GetValueFromProgress(double progress, double animation_stop_to_show_titlebar, bool for_minimize); http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:59: value = progress * animation_stop_to_show_titlebar_ / nit: indenting http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_slide_animation.h (right): http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:9: #include "ui/base/animation/animation_delegate.h" Probably we do not need to include this. Instead, we can use forward declaration. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:18: gfx::Rect initial_bounds, nit: const & http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:19: gfx::Rect final_bounds); ditto
http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:9: namespace { Empty line needed as per coding-style. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:12: const int kSetBoundsAnimationMinimizeMs = 1500; Empty line needed as per coding-style. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:33: if (animation_stop_to_show_titlebar_ > 0.7) { // Relatively big movement. I find it easier to read the other way: distance_to_title_height_ratio = distance_y / hidden_title_height; if (distance_to_title_height_ratio > 2.5) { .... http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:34: for_minimize_ = true; for_minimize_ is not quite what it is. How about calling it is_three_step_ which is what it is? http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:35: duration = kSetBoundsAnimationMinimizeMs; Same comment - I suggest calling it kSetBoundsAnimationThreeStepMs. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:51: // Minimize animation: Minimize animation: -> Three step animation: http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:60: kAnimationStopAfterQuickDecrease; I think the intent comes through more clearly if you say: animation_stop_to_show_titlebar_ * (progress / kAnimationStopAfterQuickDecrease) http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:61: } else if (progress <= kAnimationStopAfterShowingTitlebar) { Shouldn't the if condition be: if (progress <= (kAnimationStopAfterQuickDecrease + kAnimationStopAfterShowingTitlebar)) The animation from title only to three pixels feels more sluggish than the numbers suggest. I think this is why. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:67: (1.0 - kAnimationStopAfterShowingTitlebar); The above equation doesn't seem to add up for somehow: I believe the following is the intent: animation_stop_to_show_titlebar_ + (progress - animation_stop_to_show_titlebar_) * (progress - (kAnimationStopAfterQuickDecrease + kAnimationStopAfterShowingTitlebar)) / (1.0 - (kAnimationStopAfterQuickDecrease + kAnimationStopAfterShowingTitlebar))
http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:51: // Minimize animation: On 2011/12/06 23:21:30, prasadt wrote: > Minimize animation: -> Three step animation: Doesn't the 3-step animation only apply to minimizing? If that's the case, I think the current naming makes sense.
http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:51: // Minimize animation: On 2011/12/06 23:49:01, dcheng wrote: > On 2011/12/06 23:21:30, prasadt wrote: > > Minimize animation: -> Three step animation: > > Doesn't the 3-step animation only apply to minimizing? If that's the case, I > think the current naming makes sense. 3-step implies minimizing but minimizing does not imply 3-step. 3-step is done for minimize only when the window is big enough to warrant it. So, sometimes you can have for_minimize_=false and yet have it be a Minimize animation.
http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:51: // Minimize animation: On 2011/12/06 23:58:11, prasadt wrote: > On 2011/12/06 23:49:01, dcheng wrote: > > On 2011/12/06 23:21:30, prasadt wrote: > > > Minimize animation: -> Three step animation: > > > > Doesn't the 3-step animation only apply to minimizing? If that's the case, I > > think the current naming makes sense. > > 3-step implies minimizing but minimizing does not imply 3-step. 3-step is done > for minimize only when the window is big enough to warrant it. > > So, sometimes you can have for_minimize_=false and yet have it be a Minimize > animation. I see. I don't think three step is particularly specific either (what are the three steps? how do they work) but I see your point. Maybe we should just call it should_use_fancy_pants_animation_. =) (In all seriousness maybe an enum would work well here...)
http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:19: : ui::SlideAnimation(target), On 2011/12/06 19:55:10, jianli wrote: > nit: 4 spaces for indenting. Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:41: PanelSlideAnimation::~PanelSlideAnimation() { } On 2011/12/06 19:55:10, jianli wrote: > nit: better to put into 2 lines. Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:43: double PanelSlideAnimation::GetCurrentValue() const { On 2011/12/06 19:55:10, jianli wrote: > It might be better to change this to a helper function so that it can also be > used by Mac. Maybe a static public method defined in PanelSlideAnimation, like: > static double GetValueFromProgress(double progress, double > animation_stop_to_show_titlebar, bool for_minimize); Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.cc:59: value = progress * animation_stop_to_show_titlebar_ / On 2011/12/06 19:55:10, jianli wrote: > nit: indenting Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... File chrome/browser/ui/panels/panel_slide_animation.h (right): http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:9: #include "ui/base/animation/animation_delegate.h" On 2011/12/06 19:55:10, jianli wrote: > Probably we do not need to include this. Instead, we can use forward > declaration. Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:18: gfx::Rect initial_bounds, On 2011/12/06 19:55:10, jianli wrote: > nit: const & Done. http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_... chrome/browser/ui/panels/panel_slide_animation.h:19: gfx::Rect final_bounds); On 2011/12/06 19:55:10, jianli wrote: > ditto Done. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:9: namespace { On 2011/12/06 23:21:30, prasadt wrote: > Empty line needed as per coding-style. Done. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:12: const int kSetBoundsAnimationMinimizeMs = 1500; On 2011/12/06 23:21:30, prasadt wrote: > Empty line needed as per coding-style. Done. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:33: if (animation_stop_to_show_titlebar_ > 0.7) { // Relatively big movement. On 2011/12/06 23:21:30, prasadt wrote: > I find it easier to read the other way: > > distance_to_title_height_ratio = distance_y / hidden_title_height; > if (distance_to_title_height_ratio > 2.5) { > .... Hmm. it would add another long-name variable. Why would it be easier to read? http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:34: for_minimize_ = true; On 2011/12/06 23:21:30, prasadt wrote: > for_minimize_ is not quite what it is. How about calling it is_three_step_ which > is what it is? I wanted to capture "we are doing minimize animation" semantic here... The way we do it can change (and did change few times when I was implementing it, perhaps it will become 4 steps etc) but it still will be 'minimize' animation. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:35: duration = kSetBoundsAnimationMinimizeMs; On 2011/12/06 23:21:30, prasadt wrote: > Same comment - I suggest calling it kSetBoundsAnimationThreeStepMs. Ditto. 3 steps is just current impl of it. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:51: // Minimize animation: On 2011/12/06 23:21:30, prasadt wrote: > Minimize animation: -> Three step animation: I changed comment to mention that minimize animation is implemented as 3-step one. Really, the 3-step-ness of it is just in the following 3 lines, otherwise this class is about catching and playing minimize movement in a different way, not about the way it's done. I understand what you mean by saying not every minimize is 3 step, changed the variable name to better reflect that. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:60: kAnimationStopAfterQuickDecrease; On 2011/12/06 23:21:30, prasadt wrote: > I think the intent comes through more clearly if you say: > > animation_stop_to_show_titlebar_ * (progress / kAnimationStopAfterQuickDecrease) Done. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:61: } else if (progress <= kAnimationStopAfterShowingTitlebar) { On 2011/12/06 23:21:30, prasadt wrote: > Shouldn't the if condition be: > if (progress <= (kAnimationStopAfterQuickDecrease + > kAnimationStopAfterShowingTitlebar)) > > The animation from title only to three pixels feels more sluggish than the > numbers suggest. I think this is why. Discussed offline, changed constants names to be more descriptive. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:67: (1.0 - kAnimationStopAfterShowingTitlebar); On 2011/12/06 23:21:30, prasadt wrote: > The above equation doesn't seem to add up for somehow: > > I believe the following is the intent: > > animation_stop_to_show_titlebar_ + > (progress - animation_stop_to_show_titlebar_) * > (progress - (kAnimationStopAfterQuickDecrease + > kAnimationStopAfterShowingTitlebar)) / > (1.0 - (kAnimationStopAfterQuickDecrease + > kAnimationStopAfterShowingTitlebar)) > Changed from 'physical' (time past * speed) to 'mathematical' (time percentage * distance) shape, as discussed offline.
LGTM. On Tue, Dec 6, 2011 at 7:46 PM, <dimich@chromium.org> wrote: > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.cc<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.cc> > File chrome/browser/ui/panels/**panel_slide_animation.cc (right): > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode19<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.cc#newcode19> > chrome/browser/ui/panels/**panel_slide_animation.cc:19: : > ui::SlideAnimation(target), > On 2011/12/06 19:55:10, jianli wrote: > >> nit: 4 spaces for indenting. >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode41<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.cc#newcode41> > chrome/browser/ui/panels/**panel_slide_animation.cc:41: > PanelSlideAnimation::~**PanelSlideAnimation() { } > On 2011/12/06 19:55:10, jianli wrote: > >> nit: better to put into 2 lines. >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode43<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.cc#newcode43> > chrome/browser/ui/panels/**panel_slide_animation.cc:43: double > PanelSlideAnimation::**GetCurrentValue() const { > On 2011/12/06 19:55:10, jianli wrote: > >> It might be better to change this to a helper function so that it can >> > also be > >> used by Mac. Maybe a static public method defined in >> > PanelSlideAnimation, like: > >> static double GetValueFromProgress(double progress, double >> animation_stop_to_show_**titlebar, bool for_minimize); >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode59<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.cc#newcode59> > chrome/browser/ui/panels/**panel_slide_animation.cc:59: value = progress * > animation_stop_to_show_**titlebar_ / > On 2011/12/06 19:55:10, jianli wrote: > >> nit: indenting >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.h<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.h> > File chrome/browser/ui/panels/**panel_slide_animation.h (right): > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.h#newcode9<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.h#newcode9> > chrome/browser/ui/panels/**panel_slide_animation.h:9: #include > "ui/base/animation/animation_**delegate.h" > On 2011/12/06 19:55:10, jianli wrote: > >> Probably we do not need to include this. Instead, we can use forward >> declaration. >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.h#newcode18<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.h#newcode18> > chrome/browser/ui/panels/**panel_slide_animation.h:18: gfx::Rect > initial_bounds, > On 2011/12/06 19:55:10, jianli wrote: > >> nit: const & >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/9/chrome/** > browser/ui/panels/panel_slide_**animation.h#newcode19<http://codereview.chromium.org/8826002/diff/9/chrome/browser/ui/panels/panel_slide_animation.h#newcode19> > chrome/browser/ui/panels/**panel_slide_animation.h:19: gfx::Rect > final_bounds); > On 2011/12/06 19:55:10, jianli wrote: > >> ditto >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc> > File chrome/browser/ui/panels/**panel_slide_animation.cc (right): > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode9<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode9> > chrome/browser/ui/panels/**panel_slide_animation.cc:9: namespace { > > On 2011/12/06 23:21:30, prasadt wrote: > >> Empty line needed as per coding-style. >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode12<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode12> > chrome/browser/ui/panels/**panel_slide_animation.cc:12: const int > kSetBoundsAnimationMinimizeMs = 1500; > On 2011/12/06 23:21:30, prasadt wrote: > >> Empty line needed as per coding-style. >> > > Done. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode33<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode33> > chrome/browser/ui/panels/**panel_slide_animation.cc:33: if > (animation_stop_to_show_**titlebar_ > 0.7) { // Relatively big movement. > On 2011/12/06 23:21:30, prasadt wrote: > >> I find it easier to read the other way: >> > > distance_to_title_height_ratio = distance_y / hidden_title_height; >> if (distance_to_title_height_**ratio > 2.5) { >> .... >> > > Hmm. it would add another long-name variable. Why would it be easier to > read? > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode34<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode34> > chrome/browser/ui/panels/**panel_slide_animation.cc:34: for_minimize_ = > true; > On 2011/12/06 23:21:30, prasadt wrote: > >> for_minimize_ is not quite what it is. How about calling it >> > is_three_step_ which > >> is what it is? >> > > I wanted to capture "we are doing minimize animation" semantic here... > The way we do it can change (and did change few times when I was > implementing it, perhaps it will become 4 steps etc) but it still will > be 'minimize' animation. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode35<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode35> > chrome/browser/ui/panels/**panel_slide_animation.cc:35: duration = > kSetBoundsAnimationMinimizeMs; > On 2011/12/06 23:21:30, prasadt wrote: > >> Same comment - I suggest calling it kSetBoundsAnimationThreeStepMs**. >> > > Ditto. 3 steps is just current impl of it. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode51<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode51> > chrome/browser/ui/panels/**panel_slide_animation.cc:51: // Minimize > animation: > On 2011/12/06 23:21:30, prasadt wrote: > >> Minimize animation: -> Three step animation: >> > > I changed comment to mention that minimize animation is implemented as > 3-step one. Really, the 3-step-ness of it is just in the following 3 > lines, otherwise this class is about catching and playing minimize > movement in a different way, not about the way it's done. > > I understand what you mean by saying not every minimize is 3 step, > changed the variable name to better reflect that. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode60<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode60> > chrome/browser/ui/panels/**panel_slide_animation.cc:60: > kAnimationStopAfterQuickDecrea**se; > On 2011/12/06 23:21:30, prasadt wrote: > >> I think the intent comes through more clearly if you say: >> > > animation_stop_to_show_**titlebar_ * (progress / >> > kAnimationStopAfterQuickDecrea**se) > > Done. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode61<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode61> > chrome/browser/ui/panels/**panel_slide_animation.cc:61: } else if > (progress <= kAnimationStopAfterShowingTitl**ebar) { > On 2011/12/06 23:21:30, prasadt wrote: > >> Shouldn't the if condition be: >> if (progress <= (**kAnimationStopAfterQuickDecrea**se + >> kAnimationStopAfterShowingTitl**ebar)) >> > > The animation from title only to three pixels feels more sluggish than >> > the > >> numbers suggest. I think this is why. >> > > Discussed offline, changed constants names to be more descriptive. > > > http://codereview.chromium.**org/8826002/diff/5001/chrome/** > browser/ui/panels/panel_slide_**animation.cc#newcode67<http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/panel_slide_animation.cc#newcode67> > chrome/browser/ui/panels/**panel_slide_animation.cc:67: (1.0 - > kAnimationStopAfterShowingTitl**ebar); > On 2011/12/06 23:21:30, prasadt wrote: > >> The above equation doesn't seem to add up for somehow: >> > > I believe the following is the intent: >> > > animation_stop_to_show_**titlebar_ + >> (progress - animation_stop_to_show_**titlebar_) * >> (progress - (**kAnimationStopAfterQuickDecrea**se + >> kAnimationStopAfterShowingTitl**ebar)) / >> (1.0 - (**kAnimationStopAfterQuickDecrea**se + >> kAnimationStopAfterShowingTitl**ebar)) >> > > > Changed from 'physical' (time past * speed) to 'mathematical' (time > percentage * distance) shape, as discussed offline. > > http://codereview.chromium.**org/8826002/<http://codereview.chromium.org/8826... >
Just one nit. http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:33: if (animation_stop_to_show_titlebar_ > 0.7) { // Relatively big movement. On 2011/12/07 03:46:04, Dmitry Titov wrote: > On 2011/12/06 23:21:30, prasadt wrote: > > I find it easier to read the other way: > > > > distance_to_title_height_ratio = distance_y / hidden_title_height; > > if (distance_to_title_height_ratio > 2.5) { > > .... > > Hmm. it would add another long-name variable. Why would it be easier to read? distance_to_title_height_ratio tells me its a ratio of the two specified entities. animation_stop_to_show_titlebar_, I can't tell what it is without working through the math. animation_stop_to_distance_ratio_? I don't have great suggestions for an alternate name, so keep what you got if you like that best. http://codereview.chromium.org/8826002/diff/7001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_slide_animation.cc (right): http://codereview.chromium.org/8826002/diff/7001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_slide_animation.cc:13: const int kSetBoundsAnimationMinimizeMs = 1500; Nit: Change to kSetBoundsAnimationBigMinimizeMs to make it consistent with for_big_minimize_.
lgtm Meant to add lgtm with my last set of comments.
On 2011/12/07 17:59:26, prasadt wrote: > Just one nit. > > http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... > File chrome/browser/ui/panels/panel_slide_animation.cc (right): > > http://codereview.chromium.org/8826002/diff/5001/chrome/browser/ui/panels/pan... > chrome/browser/ui/panels/panel_slide_animation.cc:33: if > (animation_stop_to_show_titlebar_ > 0.7) { // Relatively big movement. > On 2011/12/07 03:46:04, Dmitry Titov wrote: > > On 2011/12/06 23:21:30, prasadt wrote: > > > I find it easier to read the other way: > > > > > > distance_to_title_height_ratio = distance_y / hidden_title_height; > > > if (distance_to_title_height_ratio > 2.5) { > > > .... > > > > Hmm. it would add another long-name variable. Why would it be easier to read? > > distance_to_title_height_ratio tells me its a ratio of the two specified > entities. > > animation_stop_to_show_titlebar_, I can't tell what it is without working > through the math. animation_stop_to_distance_ratio_? > > I don't have great suggestions for an alternate name, so keep what you got if > you like that best. Neither do I. I'll quickly lgtm it :-) > > http://codereview.chromium.org/8826002/diff/7001/chrome/browser/ui/panels/pan... > File chrome/browser/ui/panels/panel_slide_animation.cc (right): > > http://codereview.chromium.org/8826002/diff/7001/chrome/browser/ui/panels/pan... > chrome/browser/ui/panels/panel_slide_animation.cc:13: const int > kSetBoundsAnimationMinimizeMs = 1500; > Nit: Change to kSetBoundsAnimationBigMinimizeMs to make it consistent with > for_big_minimize_.
> > Nit: Change to kSetBoundsAnimationBigMinimizeMs to make it consistent with > > for_big_minimize_. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dimich@chromium.org/8826002/16001
Try job failure for 8826002-16001 (retry) on mac_rel for steps "browser_tests, ui_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dimich@chromium.org/8826002/16001 |
