Harmony - Use FocusableBorder for comboboxes on Mac

Harmony - Use FocusableBorder for comboboxes on Mac

BUG=653207
Committed: https://crrev.com/ca213587627e1b6dfd80040398db3cff98131579
Cr-Commit-Position: refs/heads/master@{#424224}

[Evan Stade, 2016-10-05 19:21:49]

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

[Elly Fong-Jones, 2016-10-06 14:15:36]

lgtm - I put some screenshots on the bug, too

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

https://codereview.chromium.org/2391393002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:771: UseMd() ? new FocusableBorder() :
PlatformStyle::CreateComboboxBorder());
this won't build - FocusableBorder* and unique_ptr<FocusableBorder> don't match

[Evan Stade, 2016-10-06 15:51:08]

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

[Evan Stade, 2016-10-06 15:51:08]

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

https://codereview.chromium.org/2391393002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:771: UseMd() ? new FocusableBorder() :
PlatformStyle::CreateComboboxBorder());
On 2016/10/06 14:15:36, Elly Fong-Jones wrote:
> this won't build - FocusableBorder* and unique_ptr<FocusableBorder> don't
match

good call, fixed.

It also occurs to me we probably don't want a different BG on mac, right? Since
for whatever reason we are only setting that in the UseMd case, we can go ahead
and ditch the mac combobox bg, yes? I've made that change as well, ptal.

[Evan Stade, 2016-10-06 15:52:02]

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

[commit-bot: I haz the power, 2016-10-06 15:52:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Elly Fong-Jones, 2016-10-06 15:54:27]

On 2016/10/06 15:51:08, Evan Stade wrote:
>
https://codereview.chromium.org/2391393002/diff/1/ui/views/controls/combobox/...
> File ui/views/controls/combobox/combobox.cc (right):
> 
>
https://codereview.chromium.org/2391393002/diff/1/ui/views/controls/combobox/...
> ui/views/controls/combobox/combobox.cc:771: UseMd() ? new FocusableBorder() :
> PlatformStyle::CreateComboboxBorder());
> On 2016/10/06 14:15:36, Elly Fong-Jones wrote:
> > this won't build - FocusableBorder* and unique_ptr<FocusableBorder> don't
> match
> 
> good call, fixed.
> 
> It also occurs to me we probably don't want a different BG on mac, right?
Since
> for whatever reason we are only setting that in the UseMd case, we can go
ahead
> and ditch the mac combobox bg, yes? I've made that change as well, ptal.

lgtm :)

[Evan Stade, 2016-10-06 15:56:37]

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

https://codereview.chromium.org/2391393002/diff/40001/ui/views/controls/combo...
ui/views/controls/combobox/combobox.cc:547:
set_background(Background::CreateBackgroundPainter(
ah, look --- the bg was being overridden anyway

[Evan Stade, 2016-10-06 15:57:11]

estade@chromium.org changed reviewers:
+ sadrul@chromium.org
- sky@chromium.org

[Evan Stade, 2016-10-06 15:57:11]

to balance review load, -sky, +sadrul

[commit-bot: I haz the power, 2016-10-06 16:27:11]

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

[commit-bot: I haz the power, 2016-10-06 16:27:11]

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_...)

[Evan Stade, 2016-10-06 21:07:19]

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

[commit-bot: I haz the power, 2016-10-06 21:08:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 21:54:21]

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

[commit-bot: I haz the power, 2016-10-06 21:54:22]

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_...)

[sadrul, 2016-10-07 00:48:23]

lgtm (the build failure on mac bots are related, but looks easily fixable)

[Elly Fong-Jones, 2016-10-10 13:22:17]

On 2016/10/07 00:48:23, sadrul wrote:
> lgtm (the build failure on mac bots are related, but looks easily fixable)

We can actually drop PlatformStyle::CreateComboboxBorder(), I think, since Mac
no longer enables Shiny Modern. That way we can create the border directly
inside Combobox.

[Evan Stade, 2016-10-10 17:17:01]

On 2016/10/10 13:22:17, Elly Fong-Jones wrote:
> On 2016/10/07 00:48:23, sadrul wrote:
> > lgtm (the build failure on mac bots are related, but looks easily fixable)
> 
> We can actually drop PlatformStyle::CreateComboboxBorder(), I think, since Mac
> no longer enables Shiny Modern. That way we can create the border directly
> inside Combobox.

done

[Evan Stade, 2016-10-10 17:17:06]

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

[commit-bot: I haz the power, 2016-10-10 17:17:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-10-10 17:46:15]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-10-10 17:46:18]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-10 17:46:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2391393002/#ps80001
(title: "delete border")

[commit-bot: I haz the power, 2016-10-10 17:46:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 18:12:20]

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

[commit-bot: I haz the power, 2016-10-10 18:12:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2016-10-10 18:36:11]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-10 18:36:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
ellyjones@chromium.org, sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2391393002/#ps100001
(title: "fix mac")

[commit-bot: I haz the power, 2016-10-10 18:36:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 20:33:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-10 20:35:32]

Description was changed from

==========
Harmony - Use FocusableBorder for comboboxes on Mac

BUG=653207
==========

to

==========
Harmony - Use FocusableBorder for comboboxes on Mac

BUG=653207

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

[commit-bot: I haz the power, 2016-10-10 20:35:33]

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