Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15111002:
Allow STATE_NORMAL text buttons to animate (Closed)
|
Created:
7 years, 7 months ago by ckocagil Modified:
7 years, 6 months ago CC:
chromium-reviews, tfarina Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAllow STATE_NORMAL text buttons to animate
Fixes a regression from http://crrev.com/88149
BUG=234472
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203111
Patch Set 1 #
Total comments: 9
Patch Set 2 : 2 #
Total comments: 2
Patch Set 3 : 3 #Messages
Total messages: 17 (0 generated)
pkasting: please review. I'm not sure why |normal_has_border| or |copy_normal_set_to_hot_set| (both of which were removed) were needed in the first place. Checking the animation state as I do here seems to work.
https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:135: // handle the case of having a non-NULL |normal_set_|. There doesn't seem to be a |normal_set_| anymore, should I remove this TODO?
LGTM; thanks! Regarding your Q: the typical TextButton appearance doesn't have a border in 'normal' mode, so there's spotty support for that case, and the animation isn't correct, as the TODO alludes to; LabelButton should be better all around, and I'm looking forward to eliminating TextButton. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:135: // handle the case of having a non-NULL |normal_set_|. On 2013/05/12 16:50:46, ckocagil wrote: > There doesn't seem to be a |normal_set_| anymore, should I remove this TODO? The TODO is still valid, just rename |normal_set_| to |normal_painter_|.
LGTM https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:127: (state == TextButton::STATE_HOVERED || Nit: Please leave in place parens around all binary subexpressions (and add more to new code). Among other things this will make the && vs. || order explicit. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:129: state == TextButton::STATE_NORMAL && animating)) { Nit: Add a comment about why the animating flag means we should switch from the normal painter to the hot painter. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:133: if (painter && animating) { Nit: Don't flatten the old nested conditional this way.
https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:127: (state == TextButton::STATE_HOVERED || On 2013/05/13 22:40:35, Peter Kasting wrote: > Nit: Please leave in place parens around all binary subexpressions (and add more > to new code). Among other things this will make the && vs. || order explicit. Done. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:129: state == TextButton::STATE_NORMAL && animating)) { On 2013/05/13 22:40:35, Peter Kasting wrote: > Nit: Add a comment about why the animating flag means we should switch from the > normal painter to the hot painter. Done. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:133: if (painter && animating) { On 2013/05/13 22:40:35, Peter Kasting wrote: > Nit: Don't flatten the old nested conditional this way. Done. https://codereview.chromium.org/15111002/diff/1/ui/views/controls/button/text... ui/views/controls/button/text_button.cc:135: // handle the case of having a non-NULL |normal_set_|. On 2013/05/12 21:40:36, msw wrote: > On 2013/05/12 16:50:46, ckocagil wrote: > > There doesn't seem to be a |normal_set_| anymore, should I remove this TODO? > > The TODO is still valid, just rename |normal_set_| to |normal_painter_|. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/15111002/6001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
ben: Can we have an owners review/stamp?
Please don't propagate all the constness inside views. It's atypical.
On 2013/05/14 16:51:21, Ben Goodger (Google) wrote: > Please don't propagate all the constness inside views. It's atypical. Just to make sure: should I not const qualify those local variables? If so, is it something to avoid in views code? I'm asking because I've been encouraged to use const in other parts of the code.
Regarding constness, I'm a big fan of as-const-as-possible, but other people have demanded that we not use const on local in the past, so in general I haven't done it in frontend code outside the omnibox. However, lately I've started seeing a resurgence of this pattern in code reviews for a variety of places. I don't really know what to say about it. https://codereview.chromium.org/15111002/diff/6001/ui/views/controls/button/t... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/6001/ui/views/controls/button/t... ui/views/controls/button/text_button.cc:131: // to allow subclasses to have things like the bookmark throb animation. Nit: It would be nice to go even further to explain how using the hot painter is necessary for this animation. Also I'd put this comment above the conditional instead of inside it.
ben: Removed the const qualifiers, could you review again? https://codereview.chromium.org/15111002/diff/6001/ui/views/controls/button/t... File ui/views/controls/button/text_button.cc (right): https://codereview.chromium.org/15111002/diff/6001/ui/views/controls/button/t... ui/views/controls/button/text_button.cc:131: // to allow subclasses to have things like the bookmark throb animation. On 2013/05/15 01:18:48, Peter Kasting wrote: > Nit: It would be nice to go even further to explain how using the hot painter is > necessary for this animation. > > Also I'd put this comment above the conditional instead of inside it. Done(?)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/15111002/17001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckocagil@chromium.org/15111002/17001
Message was sent while issue was closed.
Change committed as 203111 |
