views combobox: use MD padding on Mac in MD mode

views: use MD padding on Mac in MD mode

There is one major unresolved question, marked with TODO(ellyjones).

BUG=644709

[Elly Fong-Jones, 2016-09-07 12:32:18]

Description was changed from

==========
views: use MD padding on Mac in MD mode

There is one major unresolved question, marked with TODO(ellyjones).

BUG=644709
==========

to

==========
views: use MD padding on Mac in MD mode

There is one major unresolved question, marked with TODO(ellyjones).

BUG=644709
==========

[Elly Fong-Jones, 2016-09-07 12:32:18]

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

[Elly Fong-Jones, 2016-09-07 12:33:27]

estade: ptal?

I have a TODO in here which is puzzling me. It doesn't seem like I should have
to manually clear the area where the text will go. This problem only manifests
when we're in MD mode, and I think it's because of SetPaintToLayer().

Any ideas?

[Evan Stade, 2016-09-07 16:50:38]

On 2016/09/07 12:33:27, Elly Jones wrote:
> estade: ptal?
> 
> I have a TODO in here which is puzzling me. It doesn't seem like I should have
> to manually clear the area where the text will go. This problem only manifests
> when we're in MD mode, and I think it's because of SetPaintToLayer().
> 
> Any ideas?

ah blimey. This is due to me adding a layer to the combobox yesterday. One bad
fix is to disable subpixel anti-aliasing, but instead we should actually draw a
background (instead of a null background and letting the view underneath bleed
through). You can see what textfield.cc does. (Feel free to kick this bug over
to me since I broke it.)

[Elly Fong-Jones, 2016-09-07 16:55:55]

On 2016/09/07 16:50:38, Evan Stade wrote:
> On 2016/09/07 12:33:27, Elly Jones wrote:
> > estade: ptal?
> > 
> > I have a TODO in here which is puzzling me. It doesn't seem like I should
have
> > to manually clear the area where the text will go. This problem only
manifests
> > when we're in MD mode, and I think it's because of SetPaintToLayer().
> > 
> > Any ideas?
> 
> ah blimey. This is due to me adding a layer to the combobox yesterday. One bad
> fix is to disable subpixel anti-aliasing, but instead we should actually draw
a
> background (instead of a null background and letting the view underneath bleed
> through). You can see what textfield.cc does. (Feel free to kick this bug over
> to me since I broke it.)

Got it. How about the other changes in this CL?

[Evan Stade, 2016-09-07 17:24:43]

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

https://codereview.chromium.org/2317033002/diff/1/ui/views/controls/combobox/...
ui/views/controls/combobox/combobox.cc:979: const int kMdPaddingWidth = 8;
this change seems fine

[Elly Fong-Jones, 2016-09-07 17:27:19]

On 2016/09/07 17:24:43, Evan Stade wrote:
>
https://codereview.chromium.org/2317033002/diff/1/ui/views/controls/combobox/...
> File ui/views/controls/combobox/combobox.cc (right):
> 
>
https://codereview.chromium.org/2317033002/diff/1/ui/views/controls/combobox/...
> ui/views/controls/combobox/combobox.cc:979: const int kMdPaddingWidth = 8;
> this change seems fine

Is the CL as a whole lgtm, with the TODO? Or I can close this CL and you can put
a CL up that fixes the text drawing problem and the other two cosmetic things
from the bug, your call.

[Evan Stade, 2016-09-07 19:13:55]

On 2016/09/07 17:27:19, Elly Jones wrote:
> On 2016/09/07 17:24:43, Evan Stade wrote:
> >
>
https://codereview.chromium.org/2317033002/diff/1/ui/views/controls/combobox/...
> > File ui/views/controls/combobox/combobox.cc (right):
> > 
> >
>
https://codereview.chromium.org/2317033002/diff/1/ui/views/controls/combobox/...
> > ui/views/controls/combobox/combobox.cc:979: const int kMdPaddingWidth = 8;
> > this change seems fine
> 
> Is the CL as a whole lgtm, with the TODO? Or I can close this CL and you can
put
> a CL up that fixes the text drawing problem and the other two cosmetic things
> from the bug, your call.

The arrow spacing change doesn't seem to be related to the bug, is it? That part
looks reasonable. I thought that was what you were asking. The other two blocks
of change aren't necessary in light of my upcoming fix for the text rendering,
so they can be removed from this CL, or you can close this CL and open a new one
for just the arrow padding.