Views: factor out Combobox background and const-ify style

Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
Committed: https://crrev.com/9412ffff054dd142ec06804e1e1303d3f6f064cf
Cr-Commit-Position: refs/heads/master@{#394250}

[Elly Fong-Jones, 2016-05-12 20:36:28]

Description was changed from

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac not reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

to

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac not reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

[Elly Fong-Jones, 2016-05-12 20:36:28]

ellyjones@chromium.org changed reviewers:
+ tapted@chromium.org

[Elly Fong-Jones, 2016-05-12 20:36:42]

tapted: ptal? This is the Combobox part of
https://codereview.chromium.org/1904753002/

[tapted, 2016-05-13 07:51:02]

looks good - we just need to check that alignment. I'm pretty sure we will need
to account for it properly. Or at the very least comment that we are
intentionally changing things.

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (left):

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:61: const int
kDisclosureArrowButtonRightPadding = 12;
So there's still the same concern that the alignment could now differ slightly -
we need to pixel-test on an aura build.

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:736:
arrow_bounds.ClampToCenteredSize(ArrowSize());
e.g. after this might need a 1-pixel adjustment to ensure the alignment is
unchanged

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:884: return ArrowSize().width() + 2 *
padding;
e.g. This will now be 22px for action-style combos, where before it was 23px.

[Elly Fong-Jones, 2016-05-16 21:45:44]

tapted: ptal?

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (left):

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:61: const int
kDisclosureArrowButtonRightPadding = 12;
On 2016/05/13 07:51:02, tapted wrote:
> So there's still the same concern that the alignment could now differ slightly
-
> we need to pixel-test on an aura build.

Done.

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:736:
arrow_bounds.ClampToCenteredSize(ArrowSize());
On 2016/05/13 07:51:01, tapted wrote:
> e.g. after this might need a 1-pixel adjustment to ensure the alignment is
> unchanged

Done.

https://codereview.chromium.org/1971333002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:884: return ArrowSize().width() + 2 *
padding;
On 2016/05/13 07:51:01, tapted wrote:
> e.g. This will now be 22px for action-style combos, where before it was 23px.

Done.

[tapted, 2016-05-17 08:10:39]

https://codereview.chromium.org/1971333002/diff/20001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1971333002/diff/20001/ui/gfx/gfx.gyp#newcode281
ui/gfx/gfx.gyp:281: 'scoped_canvas.cc',
looks like this is needed in ui/gfx/BUILD.gn as well

https://codereview.chromium.org/1971333002/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:90: if (shoulder_bounds.width() % 2 == 1)
{
I think this is relying too much on coincidence -- if the padding needs to
change such that the delta is ever != 1 then it will need reworking. I like the
name (i.e. 
"PositionArrowWithinShoulder") and callsite, but I think it should be a member
function that checks for `style_ == STYLE_NORMAL` and centers, or reads from
kAction{Left,Right}Padding, which will need to move to an anonymous namespace.

[Elly Fong-Jones, 2016-05-17 14:00:55]

ellyjones@chromium.org changed reviewers:
+ sky@chromium.org

[Elly Fong-Jones, 2016-05-17 14:00:55]

tapted, sky: ptal? :)

https://codereview.chromium.org/1971333002/diff/20001/ui/gfx/gfx.gyp
File ui/gfx/gfx.gyp (right):

https://codereview.chromium.org/1971333002/diff/20001/ui/gfx/gfx.gyp#newcode281
ui/gfx/gfx.gyp:281: 'scoped_canvas.cc',
On 2016/05/17 08:10:39, tapted wrote:
> looks like this is needed in ui/gfx/BUILD.gn as well

I am going to be so thrilled when we don't have two build systems.

https://codereview.chromium.org/1971333002/diff/20001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/20001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:90: if (shoulder_bounds.width() % 2 == 1)
{
On 2016/05/17 08:10:39, tapted wrote:
> I think this is relying too much on coincidence -- if the padding needs to
> change such that the delta is ever != 1 then it will need reworking. I like
the
> name (i.e. 
> "PositionArrowWithinShoulder") and callsite, but I think it should be a member
> function that checks for `style_ == STYLE_NORMAL` and centers, or reads from
> kAction{Left,Right}Padding, which will need to move to an anonymous namespace.

I knew I was being too cute with this :) I changed it to explicitly position for
action style.

[sky, 2016-05-17 16:49:22]

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:90: gfx::Rect
PositionArrowWithinShoulder(const gfx::Rect& shoulder_bounds,
similar comment about naming.

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
nit: move to initializer on 364.

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:148: int GetShoulderWidth() const;
Shoulder? Why not ArrowWidth? Or ArrowContainerWidth?

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:196: void InitCombobox(const
std::set<int>* separators, Combobox::Style style) {
If you're going to use a default for the constructor, why not here too?

https://codereview.chromium.org/1971333002/diff/40001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:26: // The |shoulder_width|
argument is the width of the "arrows" section from the
similar naming comment.

[Elly Fong-Jones, 2016-05-17 17:35:05]

sky: ptal? :)

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.cc (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:90: gfx::Rect
PositionArrowWithinShoulder(const gfx::Rect& shoulder_bounds,
On 2016/05/17 16:49:21, sky wrote:
> similar comment about naming.

-> PositionArrowWithinContainer

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:370: if (style_ == STYLE_ACTION)
On 2016/05/17 16:49:21, sky wrote:
> nit: move to initializer on 364.

Done.

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox.h (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.h:148: int GetShoulderWidth() const;
On 2016/05/17 16:49:21, sky wrote:
> Shoulder? Why not ArrowWidth? Or ArrowContainerWidth?

tapted and I just liked "shoulder" more than "arrow container", and "arrow" is
just the arrow image itself, I think. I can change it to "arrow container"
though.

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
File ui/views/controls/combobox/combobox_unittest.cc (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox_unittest.cc:196: void InitCombobox(const
std::set<int>* separators, Combobox::Style style) {
On 2016/05/17 16:49:21, sky wrote:
> If you're going to use a default for the constructor, why not here too?

I left it un-defaulted to make sure I had updated all the call sites to reflect
the type of combo box the test was expecting, and then I decided to leave it
like this so it's clear when reading a test what combobox it uses. I don't
really have strong feelings about it and I can change it if you like.

https://codereview.chromium.org/1971333002/diff/40001/ui/views/style/mac/comb...
File ui/views/style/mac/combobox_background_mac.h (right):

https://codereview.chromium.org/1971333002/diff/40001/ui/views/style/mac/comb...
ui/views/style/mac/combobox_background_mac.h:26: // The |shoulder_width|
argument is the width of the "arrows" section from the
On 2016/05/17 16:49:22, sky wrote:
> similar naming comment.

Done.

[Elly Fong-Jones, 2016-05-17 17:35:20]

Description was changed from

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac not reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

to

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

[sky, 2016-05-17 19:21:57]

LGTM

[Elly Fong-Jones, 2016-05-17 19:38:04]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2016-05-17 19:38:55]

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

[commit-bot: I haz the power, 2016-05-17 19:51:20]

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

[commit-bot: I haz the power, 2016-05-17 19:51:21]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Elly Fong-Jones, 2016-05-17 20:32:16]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2016-05-17 20:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1971333002/#ps80001
(title: "fix ios build break")

[commit-bot: I haz the power, 2016-05-17 20:33:06]

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

[commit-bot: I haz the power, 2016-05-17 22:41:20]

Description was changed from

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

to

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

[commit-bot: I haz the power, 2016-05-17 22:41:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-17 22:44:01]

Description was changed from

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161
==========

to

==========
Views: factor out Combobox background and const-ify style

This change:
* Factors background sizing out of Combobox
* Makes ComboboxBackgroundMac reusable for other controls
* Removes the ability to change the style of comboboxes dynamically
* Introduces caching of arrow images for comboboxes

BUG=605161

Committed: https://crrev.com/9412ffff054dd142ec06804e1e1303d3f6f064cf
Cr-Commit-Position: refs/heads/master@{#394250}
==========

[commit-bot: I haz the power, 2016-05-17 22:44:02]

Patchset 5 (id:??) landed as
https://crrev.com/9412ffff054dd142ec06804e1e1303d3f6f064cf
Cr-Commit-Position: refs/heads/master@{#394250}