CustomButton cleanup: make protected members private, create accessors

CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org

Committed: https://crrev.com/a1dbae6db9820855aa13dea0bba989793ed7d54a
Cr-Commit-Position: refs/heads/master@{#367093}

[Evan Stade, 2015-12-18 22:43:21]

estade@chromium.org changed reviewers:
+ oshima@chromium.org, pkasting@chromium.org, sadrul@chromium.org,
tdanderson@chromium.org

[Evan Stade, 2015-12-18 22:43:22]

+oshima for ash/
+pkasting for chrome/
+sadrul for ui/

[Evan Stade, 2015-12-18 22:43:26]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[oshima, 2015-12-18 22:44:47]

ash/ lgtm

[commit-bot: I haz the power, 2015-12-18 22:44:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534303002/1

[Peter Kasting, 2015-12-19 01:22:27]

LGTM

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (left):

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:378: hover_value =
hover_animation_->GetCurrentValue();
I _think_ your changes to always trust the animation state are correct now that
I've made custom_button.cc always reset the hover animation on state
transitions, but I'm nervous enough that you should make sure you try all the
different possible state transitions.

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:588: if
(hover_animation().GetCurrentValue()) {
Nit: Store this in a temp to avoid multiple calls

[Evan Stade, 2015-12-21 20:19:28]

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (left):

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:378: hover_value =
hover_animation_->GetCurrentValue();
On 2015/12/19 01:22:27, Peter Kasting wrote:
> I _think_ your changes to always trust the animation state are correct now
that
> I've made custom_button.cc always reset the hover animation on state
> transitions, but I'm nervous enough that you should make sure you try all the
> different possible state transitions.

I've tried every combination of hovering, clicking, unhovering, etc. that I can
think of. Seems to work.

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:588: if
(hover_animation().GetCurrentValue()) {
On 2015/12/19 01:22:27, Peter Kasting wrote:
> Nit: Store this in a temp to avoid multiple calls

question: you authored this code recently. Why does it use
gfx::Tween::LinearIntValueBetween instead of Animation::CurrentValueBetween? Is
the "Linear" distinction important?

I've changed the code to a version that is slightly more concise, ptal.

[Evan Stade, 2015-12-21 20:19:34]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-21 20:19:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534303002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534303002/40001

[commit-bot: I haz the power, 2015-12-21 20:31:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-21 20:31:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2015-12-21 20:34:18]

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1534303002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:588: if
(hover_animation().GetCurrentValue()) {
On 2015/12/21 20:19:28, Evan Stade wrote:
> On 2015/12/19 01:22:27, Peter Kasting wrote:
> > Nit: Store this in a temp to avoid multiple calls
> 
> question: you authored this code recently. Why does it use
> gfx::Tween::LinearIntValueBetween instead of Animation::CurrentValueBetween?
Is
> the "Linear" distinction important?

Using LinearIntValueBetween() matched the previous behavior (I was just
refactoring it).  The difference between this and IntValueBetween() (which is
what CurrentValueBetween() will use) should not be very perceptible to the user,
so it's fine to use the other for simplicity.

https://codereview.chromium.org/1534303002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1534303002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:590: if (alpha != SK_AlphaTRANSPARENT)
I suspect that Skia detects a transparent fill and bails, so we could probably
remove this conditional if you want, but it's fine either way.

[sadrul, 2015-12-21 22:05:32]

lgtm. A couple of nits

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:149: // Hover animation.
The comments aren't probably necessary really.

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right):

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/scrol...
ui/views/controls/scrollbar/native_scroll_bar_views.cc:200: default:
Maybe remove default, and add a NOTREACHED() for STATE_COUNT instead? (ditto for
line ~137 above)

[Evan Stade, 2015-12-21 22:27:16]

estade@chromium.org changed reviewers:
+ erg@chromium.org

[Evan Stade, 2015-12-21 22:27:16]

+erg for mash/

https://codereview.chromium.org/1534303002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_strip.cc (right):

https://codereview.chromium.org/1534303002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_strip.cc:590: if (alpha != SK_AlphaTRANSPARENT)
On 2015/12/21 20:34:18, Peter Kasting wrote:
> I suspect that Skia detects a transparent fill and bails, so we could probably
> remove this conditional if you want, but it's fine either way.

I followed the callstack for a while and couldn't see any obvious early returns.
Even if it does at some deep layer do nothing, it still does a good bit of work
before figuring that out.

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/butto...
File ui/views/controls/button/custom_button.h (right):

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/butto...
ui/views/controls/button/custom_button.h:149: // Hover animation.
On 2015/12/21 22:05:31, sadrul (slow until new year) wrote:
> The comments aren't probably necessary really.

Done.

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/scrol...
File ui/views/controls/scrollbar/native_scroll_bar_views.cc (right):

https://codereview.chromium.org/1534303002/diff/40001/ui/views/controls/scrol...
ui/views/controls/scrollbar/native_scroll_bar_views.cc:200: default:
On 2015/12/21 22:05:32, sadrul (slow until new year) wrote:
> Maybe remove default, and add a NOTREACHED() for STATE_COUNT instead? (ditto
for
> line ~137 above)

Done.

[Evan Stade, 2015-12-29 18:16:55]

Description was changed from

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
==========

to

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org
==========

[Evan Stade, 2015-12-29 18:18:08]

TBR'ing mash/wm/ because the code is cloned from ash/

[Evan Stade, 2015-12-29 18:36:21]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-29 18:36:21]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org,
pkasting@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1534303002/#ps120001
(title: "dont move declaration randomly")

[commit-bot: I haz the power, 2015-12-29 18:36:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534303002/120001

[commit-bot: I haz the power, 2015-12-29 19:37:45]

Description was changed from

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org
==========

to

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org
==========

[commit-bot: I haz the power, 2015-12-29 19:37:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-29 19:38:46]

Description was changed from

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org
==========

to

==========
CustomButton cleanup: make protected members private, create accessors

Also delete some old file that's not in use.

BUG=none
TBR=erg@chromium.org

Committed: https://crrev.com/a1dbae6db9820855aa13dea0bba989793ed7d54a
Cr-Commit-Position: refs/heads/master@{#367093}
==========

[commit-bot: I haz the power, 2015-12-29 19:38:47]

Patchset 7 (id:??) landed as
https://crrev.com/a1dbae6db9820855aa13dea0bba989793ed7d54a
Cr-Commit-Position: refs/heads/master@{#367093}