views::Separator cleanup.

views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator by removing enum and using terms "height" and "width"
- Improve clarity of ash tray constants.

BUG=688505
Review-Url: https://codereview.chromium.org/2675983003
Cr-Commit-Position: refs/heads/master@{#451088}
Committed: https://chromium.googlesource.com/chromium/src/+/4a3ae74ccfbe8470cd8343ad7e93ed27156171de

[Evan Stade, 2017-02-03 20:31:25]

Description was changed from

==========
Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator
- Improve clarity of ash tray constants.

BUG=688505
==========

to

==========
views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator
- Improve clarity of ash tray constants.

BUG=688505
==========

[Evan Stade, 2017-02-03 20:31:25]

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

[Evan Stade, 2017-02-03 20:37:40]

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

[commit-bot: I haz the power, 2017-02-03 20:38:16]

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

[Evan Stade, 2017-02-03 20:38:19]

+sky for ui/views
+tdanderson for ash/

https://codereview.chromium.org/2675983003/diff/1/ash/common/system/tray/tray...
File ash/common/system/tray/tray_popup_utils.cc (left):

https://codereview.chromium.org/2675983003/diff/1/ash/common/system/tray/tray...
ash/common/system/tray/tray_popup_utils.cc:333: new
views::Separator(views::Separator::HORIZONTAL);
This was actually creating a horizontal separator (i.e. --------) that was 24px
tall and 1 px long.

There seems to be confusion about the meaning of "horizontal", whether that's a
horizontal line or something that separates horizontal elements, but it's easy
to get it backwards and still have things work ok, especially for separators of
a default size (1x1 preferred size and gets laid out by a layout manager than
ignores one preferred dimension).

[commit-bot: I haz the power, 2017-02-03 21:33:54]

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

[commit-bot: I haz the power, 2017-02-03 21:33:55]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-03 23:49:45]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
Are you sure this is the right thing? As views don't necessarily align with
pixels this won't necessarily look nice. I think you should just fillrect.

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:33: static const int kThickness;
constexpr?

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
Is there a reason you prefer length here? I like size simply because it maps
nicely with GetPreferredSize.

[Evan Stade, 2017-02-06 16:31:27]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
On 2017/02/03 23:49:45, sky wrote:
> Are you sure this is the right thing? As views don't necessarily align with
> pixels this won't necessarily look nice. I think you should just fillrect.

yes. Before/after screenshot posted to bug. If the view is not pixel aligned it
still might not look nice. But with FillRect, it looks bad pixel aligned or not.

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:33: static const int kThickness;
On 2017/02/03 23:49:45, sky wrote:
> constexpr?

I don't understand this completely but this is the only way I could get the
build to link (i.e. if this is defined inline with or without constexpr then it
won't be exported for other build components).

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
On 2017/02/03 23:49:45, sky wrote:
> Is there a reason you prefer length here? I like size simply because it maps
> nicely with GetPreferredSize.

That's exactly why I don't like size. Which dimension is it referring to?
Existing client code got it wrong, so it must not be very obvious whether "size"
means length or thickness.

[sky, 2017-02-06 17:59:21]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
On 2017/02/06 16:31:26, Evan Stade wrote:
> On 2017/02/03 23:49:45, sky wrote:
> > Are you sure this is the right thing? As views don't necessarily align with
> > pixels this won't necessarily look nice. I think you should just fillrect.
> 
> yes. Before/after screenshot posted to bug. If the view is not pixel aligned
it
> still might not look nice. But with FillRect, it looks bad pixel aligned or
not.

Do we need to turn on aa?

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
On 2017/02/06 16:31:27, Evan Stade wrote:
> On 2017/02/03 23:49:45, sky wrote:
> > Is there a reason you prefer length here? I like size simply because it maps
> > nicely with GetPreferredSize.
> 
> That's exactly why I don't like size. Which dimension is it referring to?
> Existing client code got it wrong, so it must not be very obvious whether
"size"
> means length or thickness.

I don't think there is a right answer here. I prefer size.

[Evan Stade, 2017-02-06 19:03:29]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
On 2017/02/06 17:59:21, sky wrote:
> On 2017/02/06 16:31:26, Evan Stade wrote:
> > On 2017/02/03 23:49:45, sky wrote:
> > > Are you sure this is the right thing? As views don't necessarily align
with
> > > pixels this won't necessarily look nice. I think you should just fillrect.
> > 
> > yes. Before/after screenshot posted to bug. If the view is not pixel aligned
> it
> > still might not look nice. But with FillRect, it looks bad pixel aligned or
> not.
> 
> Do we need to turn on aa?

We want an integral number of pixels. AA would give us blurry lines.

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
On 2017/02/06 17:59:21, sky wrote:
> On 2017/02/06 16:31:27, Evan Stade wrote:
> > On 2017/02/03 23:49:45, sky wrote:
> > > Is there a reason you prefer length here? I like size simply because it
maps
> > > nicely with GetPreferredSize.
> > 
> > That's exactly why I don't like size. Which dimension is it referring to?
> > Existing client code got it wrong, so it must not be very obvious whether
> "size"
> > means length or thickness.
> 
> I don't think there is a right answer here. I prefer size.

size is used to describe a multi-dimensional value. Because of the confusion
over what this meant before (it actually used to be thickness), we had a
vertical line that was being implemented as a very thick but not long horizontal
separator, i.e. instead of

|
|
|
|

it was

-
-
-
-

[tdanderson, 2017-02-06 21:19:06]

ash/ LGTM with the comments below addressed.

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (left):

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:351: kTitleRowProgressBarHeight -
kSeparatorWidth, 0, 0, 0));
There are two other instances of |kSeparatorWidth| in this class that you
probably want to replace with the new views version too.

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_popup_utils.cc (left):

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_popup_utils.cc:429: kMenuSeparatorVerticalPadding -
kSeparatorWidth,
ditto to my comment in tray_details_view.cc

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:39: gfx::Size size = orientation_ == HORIZONTAL ?
gfx::Size(length_, kThickness)
You're switching the meaning of HORIZONTAL vs VERTICAL but this CL doesn't
update every call site where a Separator is instantiated.

[Evan Stade, 2017-02-06 21:39:17]

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/tray_details_view.cc (left):

https://codereview.chromium.org/2675983003/diff/20001/ash/common/system/tray/...
ash/common/system/tray/tray_details_view.cc:351: kTitleRowProgressBarHeight -
kSeparatorWidth, 0, 0, 0));
On 2017/02/06 21:19:06, tdanderson wrote:
> There are two other instances of |kSeparatorWidth| in this class that you
> probably want to replace with the new views version too.

I intentionally did not update the uses that are not views::Separator related. I
did add a test to make sure we could use these constants interchangeably.

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:39: gfx::Size size = orientation_ == HORIZONTAL ?
gfx::Size(length_, kThickness)
On 2017/02/06 21:19:06, tdanderson wrote:
> You're switching the meaning of HORIZONTAL vs VERTICAL but this CL doesn't
> update every call site where a Separator is instantiated.

The meaning of horizontal and vertical are the same. I've changed the meaning of
"SetPreferredSize/Length" and only two places call that function, one of which
is going away soon.

Probably I could just rip out that function and the orientation enum altogether
(just always request 1x1 because it's almost always the layout that sets the
length appropriately).

[sky, 2017-02-06 22:42:55]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
On 2017/02/06 19:03:29, Evan Stade wrote:
> On 2017/02/06 17:59:21, sky wrote:
> > On 2017/02/06 16:31:26, Evan Stade wrote:
> > > On 2017/02/03 23:49:45, sky wrote:
> > > > Are you sure this is the right thing? As views don't necessarily align
> with
> > > > pixels this won't necessarily look nice. I think you should just
fillrect.
> > > 
> > > yes. Before/after screenshot posted to bug. If the view is not pixel
aligned
> > it
> > > still might not look nice. But with FillRect, it looks bad pixel aligned
or
> > not.
> > 
> > Do we need to turn on aa?
> 
> We want an integral number of pixels. AA would give us blurry lines.

I think we're going in circles. We really need views to position and size views
to pixel boundaries. Otherwise we'll continue having non-intuitive code like
this.

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
On 2017/02/06 19:03:29, Evan Stade wrote:
> On 2017/02/06 17:59:21, sky wrote:
> > On 2017/02/06 16:31:27, Evan Stade wrote:
> > > On 2017/02/03 23:49:45, sky wrote:
> > > > Is there a reason you prefer length here? I like size simply because it
> maps
> > > > nicely with GetPreferredSize.
> > > 
> > > That's exactly why I don't like size. Which dimension is it referring to?
> > > Existing client code got it wrong, so it must not be very obvious whether
> > "size"
> > > means length or thickness.
> > 
> > I don't think there is a right answer here. I prefer size.
> 
> size is used to describe a multi-dimensional value. Because of the confusion
> over what this meant before (it actually used to be thickness), we had a
> vertical line that was being implemented as a very thick but not long
horizontal
> separator, i.e. instead of
> 
> |
> |
> |
> |
> 
> it was
> 
> -
> -
> -
> -

'size' is used not inheritantly a multi-dimensional value. gfx::Size is, but
this functions takes a single int. I don't think length is more intuitive in
this case.

[Evan Stade, 2017-02-07 16:57:54]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:56: // The separator fills its bounds, but avoid
filling partial pixels.
On 2017/02/06 22:42:55, sky wrote:
> On 2017/02/06 19:03:29, Evan Stade wrote:
> > On 2017/02/06 17:59:21, sky wrote:
> > > On 2017/02/06 16:31:26, Evan Stade wrote:
> > > > On 2017/02/03 23:49:45, sky wrote:
> > > > > Are you sure this is the right thing? As views don't necessarily align
> > with
> > > > > pixels this won't necessarily look nice. I think you should just
> fillrect.
> > > > 
> > > > yes. Before/after screenshot posted to bug. If the view is not pixel
> aligned
> > > it
> > > > still might not look nice. But with FillRect, it looks bad pixel aligned
> or
> > > not.
> > > 
> > > Do we need to turn on aa?
> > 
> > We want an integral number of pixels. AA would give us blurry lines.
> 
> I think we're going in circles. We really need views to position and size
views
> to pixel boundaries. Otherwise we'll continue having non-intuitive code like
> this.

Yea, but I thought that was a long way off from becoming reality. At this point
is there a way to specify layout size in pixels? Is there another solution here
I'm missing to get a consistent and sharp separator?

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.h:42: void SetPreferredLength(int length);
On 2017/02/06 22:42:55, sky wrote:
> On 2017/02/06 19:03:29, Evan Stade wrote:
> > On 2017/02/06 17:59:21, sky wrote:
> > > On 2017/02/06 16:31:27, Evan Stade wrote:
> > > > On 2017/02/03 23:49:45, sky wrote:
> > > > > Is there a reason you prefer length here? I like size simply because
it
> > maps
> > > > > nicely with GetPreferredSize.
> > > > 
> > > > That's exactly why I don't like size. Which dimension is it referring
to?
> > > > Existing client code got it wrong, so it must not be very obvious
whether
> > > "size"
> > > > means length or thickness.
> > > 
> > > I don't think there is a right answer here. I prefer size.
> > 
> > size is used to describe a multi-dimensional value. Because of the confusion
> > over what this meant before (it actually used to be thickness), we had a
> > vertical line that was being implemented as a very thick but not long
> horizontal
> > separator, i.e. instead of
> > 
> > |
> > |
> > |
> > |
> > 
> > it was
> > 
> > -
> > -
> > -
> > -
> 
> 'size' is used not inheritantly a multi-dimensional value. gfx::Size is, but
> this functions takes a single int. I don't think length is more intuitive in
> this case.

ok, but before this cl, this "size" actually meant thickness (i.e. orthogonal to
the Orientation). Now it will mean length (i.e. parallel to the Orientation
direction). So we're changing the meaning of the function here. Which meaning is
more intuitively correlated with "size"?

In the latest patchset I've tried another tactic to make this more clear. Since
orientation is only useful if you set a preferred length/size I've combined
setting the orientation and the size, which hopefully makes it more obvious what
this function is doing, and also relieves the vast majority of callers from
needing to specify an orientation at all.

[Evan Stade, 2017-02-07 16:58:03]

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

[commit-bot: I haz the power, 2017-02-07 16:58:42]

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, 2017-02-07 17:31:50]

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

[commit-bot: I haz the power, 2017-02-07 17:31:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sky, 2017-02-07 19:02:49]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:39: gfx::Size size = orientation_ == HORIZONTAL ?
gfx::Size(length_, kThickness)
On 2017/02/06 21:39:17, Evan Stade wrote:
> On 2017/02/06 21:19:06, tdanderson wrote:
> > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL doesn't
> > update every call site where a Separator is instantiated.
> 
> The meaning of horizontal and vertical are the same. I've changed the meaning
of
> "SetPreferredSize/Length" and only two places call that function, one of which
> is going away soon.
> 
> Probably I could just rip out that function and the orientation enum
altogether
> (just always request 1x1 because it's almost always the layout that sets the
> length appropriately).

I see where you are going. I find the change to separator here confusing. When
you create a separator you have an explicit orientation in mind. Is there some
use case I'm not seeing where you know you want a separator but don't know the
orientation?

[Evan Stade, 2017-02-07 21:39:50]

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
ui/views/controls/separator.cc:39: gfx::Size size = orientation_ == HORIZONTAL ?
gfx::Size(length_, kThickness)
On 2017/02/07 19:02:49, sky wrote:
> On 2017/02/06 21:39:17, Evan Stade wrote:
> > On 2017/02/06 21:19:06, tdanderson wrote:
> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL doesn't
> > > update every call site where a Separator is instantiated.
> > 
> > The meaning of horizontal and vertical are the same. I've changed the
meaning
> of
> > "SetPreferredSize/Length" and only two places call that function, one of
which
> > is going away soon.
> > 
> > Probably I could just rip out that function and the orientation enum
> altogether
> > (just always request 1x1 because it's almost always the layout that sets the
> > length appropriately).

I looked into this but it's not convenient to actually remove this function and
rely on borders for the tray separators.

> 
> I see where you are going. I find the change to separator here confusing. When
> you create a separator you have an explicit orientation in mind. Is there some
> use case I'm not seeing where you know you want a separator but don't know the
> orientation?

No, there is not a case where you have a separator but you don't know the
orientation. The issue is that the orientation is totally ignored unless you
also call SetPreferredSize. Of all the places that create separators, only one
uses SetPreferredSize. For n - 1 instantiations, the orientation param is
redundant (with the layout manager) and irrelevant (you could change it to the
other orientation with no change in behavior). For the last instantiation ---
the one that does use SetPreferredSize --- the current code passes the wrong
orientation because SetPreferredSize controls the wrong dimension for that
code's purposes.

tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is passing
the wrong value, currently.

[sky, 2017-02-08 00:58:26]

On Tue, Feb 7, 2017 at 1:39 PM,  <estade@chromium.org> wrote:
>
>
https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
> File ui/views/controls/separator.cc (right):
>
>
https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
> ui/views/controls/separator.cc:39: gfx::Size size = orientation_ ==
> HORIZONTAL ? gfx::Size(length_, kThickness)
> On 2017/02/07 19:02:49, sky wrote:
>> On 2017/02/06 21:39:17, Evan Stade wrote:
>> > On 2017/02/06 21:19:06, tdanderson wrote:
>> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL
> doesn't
>> > > update every call site where a Separator is instantiated.
>> >
>> > The meaning of horizontal and vertical are the same. I've changed
> the meaning
>> of
>> > "SetPreferredSize/Length" and only two places call that function,
> one of which
>> > is going away soon.
>> >
>> > Probably I could just rip out that function and the orientation enum
>> altogether
>> > (just always request 1x1 because it's almost always the layout that
> sets the
>> > length appropriately).
>
> I looked into this but it's not convenient to actually remove this
> function and rely on borders for the tray separators.
>
>>
>> I see where you are going. I find the change to separator here
> confusing. When
>> you create a separator you have an explicit orientation in mind. Is
> there some
>> use case I'm not seeing where you know you want a separator but don't
> know the
>> orientation?
>
> No, there is not a case where you have a separator but you don't know
> the orientation. The issue is that the orientation is totally ignored
> unless you also call SetPreferredSize.

That seems like the bug. It should have a reasonable default, say 1 or 2.

> Of all the places that create
> separators, only one uses SetPreferredSize. For n - 1 instantiations,
> the orientation param is redundant (with the layout manager) and
> irrelevant (you could change it to the other orientation with no change
> in behavior). For the last instantiation --- the one that does use
> SetPreferredSize --- the current code passes the wrong orientation
> because SetPreferredSize controls the wrong dimension for that code's
> purposes.

How about SetPreferredSize(base::Optional<int> width,
base::Optional<int> height) ? If you don't specify one of the value
the default is chosen.

  -Scott

>
> tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is
> passing the wrong value, currently.
>
> https://codereview.chromium.org/2675983003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-02-08 16:21:30]

On 2017/02/08 00:58:26, sky wrote:
> On Tue, Feb 7, 2017 at 1:39 PM,  <mailto:estade@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
> > File ui/views/controls/separator.cc (right):
> >
> >
>
https://codereview.chromium.org/2675983003/diff/20001/ui/views/controls/separ...
> > ui/views/controls/separator.cc:39: gfx::Size size = orientation_ ==
> > HORIZONTAL ? gfx::Size(length_, kThickness)
> > On 2017/02/07 19:02:49, sky wrote:
> >> On 2017/02/06 21:39:17, Evan Stade wrote:
> >> > On 2017/02/06 21:19:06, tdanderson wrote:
> >> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL
> > doesn't
> >> > > update every call site where a Separator is instantiated.
> >> >
> >> > The meaning of horizontal and vertical are the same. I've changed
> > the meaning
> >> of
> >> > "SetPreferredSize/Length" and only two places call that function,
> > one of which
> >> > is going away soon.
> >> >
> >> > Probably I could just rip out that function and the orientation enum
> >> altogether
> >> > (just always request 1x1 because it's almost always the layout that
> > sets the
> >> > length appropriately).
> >
> > I looked into this but it's not convenient to actually remove this
> > function and rely on borders for the tray separators.
> >
> >>
> >> I see where you are going. I find the change to separator here
> > confusing. When
> >> you create a separator you have an explicit orientation in mind. Is
> > there some
> >> use case I'm not seeing where you know you want a separator but don't
> > know the
> >> orientation?
> >
> > No, there is not a case where you have a separator but you don't know
> > the orientation. The issue is that the orientation is totally ignored
> > unless you also call SetPreferredSize.
> 
> That seems like the bug. It should have a reasonable default, say 1 or 2.

By "it" I think you mean the preferred length? It does have a default: 1 (same
as the other dimension until you call SetPreferredSize).

> 
> > Of all the places that create
> > separators, only one uses SetPreferredSize. For n - 1 instantiations,
> > the orientation param is redundant (with the layout manager) and
> > irrelevant (you could change it to the other orientation with no change
> > in behavior). For the last instantiation --- the one that does use
> > SetPreferredSize --- the current code passes the wrong orientation
> > because SetPreferredSize controls the wrong dimension for that code's
> > purposes.
> 
> How about SetPreferredSize(base::Optional<int> width,
> base::Optional<int> height) ? If you don't specify one of the value
> the default is chosen.

While removing orientation completely? This seems ok --- not too dissimilar from
SetPrefSize(orientation, int) --- but there are no clients that actually want to
set both dimensions. I'm a little hesitant to allow setting the thickness in
this way because it invites inconsistencies (I don't think we actually want any
extra-thick separators anywhere).

> 
>   -Scott
> 
> >
> > tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is
> > passing the wrong value, currently.
> >
> > https://codereview.chromium.org/2675983003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
>

[sky, 2017-02-08 17:26:14]

Here's the list of concerns I have with the API you've created:

. I find it confusing that you don't explicitly specify the orientation. It
doesn't really matter the way you have it now because the separator is a
fill. I don't think it's a valid assumption though. It's entirely possible
we would want the separator to look different depending upon the axis.

. SetPreferredSize() is overloading the meaning of orientation to mean a
particular axis.

I prefer keeping the orientation in the constructor, it makes usage clear.
I prefer either SetPreferredSize with two optionals, or
SetPreferredHeight/SetPreferredWidth. Either of these give the flexibility
of specifying the preferred size without overloading HORIZONTRAL/VERTICAL.


On Wed, Feb 8, 2017 at 8:21 AM, <estade@chromium.org> wrote:

> On 2017/02/08 00:58:26, sky wrote:
>
> > On Tue, Feb 7, 2017 at 1:39 PM, <mailto:estade@chromium.org> wrote:
> > >
> > >
> >
> https://codereview.chromium.org/2675983003/diff/20001/ui/
> views/controls/separator.cc
> > > File ui/views/controls/separator.cc (right):
> > >
> > >
> >
> https://codereview.chromium.org/2675983003/diff/20001/ui/
> views/controls/separator.cc#newcode39
> > > ui/views/controls/separator.cc:39: gfx::Size size = orientation_ ==
> > > HORIZONTAL ? gfx::Size(length_, kThickness)
> > > On 2017/02/07 19:02:49, sky wrote:
> > >> On 2017/02/06 21:39:17, Evan Stade wrote:
> > >> > On 2017/02/06 21:19:06, tdanderson wrote:
> > >> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL
> > > doesn't
> > >> > > update every call site where a Separator is instantiated.
> > >> >
> > >> > The meaning of horizontal and vertical are the same. I've changed
> > > the meaning
> > >> of
> > >> > "SetPreferredSize/Length" and only two places call that function,
> > > one of which
> > >> > is going away soon.
> > >> >
> > >> > Probably I could just rip out that function and the orientation enum
> > >> altogether
> > >> > (just always request 1x1 because it's almost always the layout that
> > > sets the
> > >> > length appropriately).
> > >
> > > I looked into this but it's not convenient to actually remove this
> > > function and rely on borders for the tray separators.
> > >
> > >>
> > >> I see where you are going. I find the change to separator here
> > > confusing. When
> > >> you create a separator you have an explicit orientation in mind. Is
> > > there some
> > >> use case I'm not seeing where you know you want a separator but don't
> > > know the
> > >> orientation?
> > >
> > > No, there is not a case where you have a separator but you don't know
> > > the orientation. The issue is that the orientation is totally ignored
> > > unless you also call SetPreferredSize.
> >
> > That seems like the bug. It should have a reasonable default, say 1 or 2.
>
> By "it" I think you mean the preferred length? It does have a default: 1
> (same
> as the other dimension until you call SetPreferredSize).
>
> >
> > > Of all the places that create
> > > separators, only one uses SetPreferredSize. For n - 1 instantiations,
> > > the orientation param is redundant (with the layout manager) and
> > > irrelevant (you could change it to the other orientation with no change
> > > in behavior). For the last instantiation --- the one that does use
> > > SetPreferredSize --- the current code passes the wrong orientation
> > > because SetPreferredSize controls the wrong dimension for that code's
> > > purposes.
> >
> > How about SetPreferredSize(base::Optional<int> width,
> > base::Optional<int> height) ? If you don't specify one of the value
> > the default is chosen.
>
> While removing orientation completely? This seems ok --- not too
> dissimilar from
> SetPrefSize(orientation, int) --- but there are no clients that actually
> want to
> set both dimensions. I'm a little hesitant to allow setting the thickness
> in
> this way because it invites inconsistencies (I don't think we actually
> want any
> extra-thick separators anywhere).
>
> >
> > -Scott
> >
> > >
> > > tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is
> > > passing the wrong value, currently.
> > >
> > > https://codereview.chromium.org/2675983003/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
>
>
>
> https://codereview.chromium.org/2675983003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-02-08 17:35:53]

On 2017/02/08 17:26:14, sky wrote:
> Here's the list of concerns I have with the API you've created:
> 
> . I find it confusing that you don't explicitly specify the orientation. It
> doesn't really matter the way you have it now because the separator is a
> fill. I don't think it's a valid assumption though. It's entirely possible
> we would want the separator to look different depending upon the axis.
> 
> . SetPreferredSize() is overloading the meaning of orientation to mean a
> particular axis.
> 
> I prefer keeping the orientation in the constructor, it makes usage clear.
> I prefer either SetPreferredSize with two optionals, or
> SetPreferredHeight/SetPreferredWidth. Either of these give the flexibility
> of specifying the preferred size without overloading HORIZONTRAL/VERTICAL.

When do we use orientation then? If I understand your suggestion correctly, the
compiler complains:

../../ui/views/controls/separator.h:49:21: error: private field 'orientation_'
is not used [-Werror,-Wunused-private-field]
  const Orientation orientation_;


> 
> 
> On Wed, Feb 8, 2017 at 8:21 AM, <mailto:estade@chromium.org> wrote:
> 
> > On 2017/02/08 00:58:26, sky wrote:
> >
> > > On Tue, Feb 7, 2017 at 1:39 PM, <mailto:estade@chromium.org> wrote:
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2675983003/diff/20001/ui/
> > views/controls/separator.cc
> > > > File ui/views/controls/separator.cc (right):
> > > >
> > > >
> > >
> > https://codereview.chromium.org/2675983003/diff/20001/ui/
> > views/controls/separator.cc#newcode39
> > > > ui/views/controls/separator.cc:39: gfx::Size size = orientation_ ==
> > > > HORIZONTAL ? gfx::Size(length_, kThickness)
> > > > On 2017/02/07 19:02:49, sky wrote:
> > > >> On 2017/02/06 21:39:17, Evan Stade wrote:
> > > >> > On 2017/02/06 21:19:06, tdanderson wrote:
> > > >> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this CL
> > > > doesn't
> > > >> > > update every call site where a Separator is instantiated.
> > > >> >
> > > >> > The meaning of horizontal and vertical are the same. I've changed
> > > > the meaning
> > > >> of
> > > >> > "SetPreferredSize/Length" and only two places call that function,
> > > > one of which
> > > >> > is going away soon.
> > > >> >
> > > >> > Probably I could just rip out that function and the orientation enum
> > > >> altogether
> > > >> > (just always request 1x1 because it's almost always the layout that
> > > > sets the
> > > >> > length appropriately).
> > > >
> > > > I looked into this but it's not convenient to actually remove this
> > > > function and rely on borders for the tray separators.
> > > >
> > > >>
> > > >> I see where you are going. I find the change to separator here
> > > > confusing. When
> > > >> you create a separator you have an explicit orientation in mind. Is
> > > > there some
> > > >> use case I'm not seeing where you know you want a separator but don't
> > > > know the
> > > >> orientation?
> > > >
> > > > No, there is not a case where you have a separator but you don't know
> > > > the orientation. The issue is that the orientation is totally ignored
> > > > unless you also call SetPreferredSize.
> > >
> > > That seems like the bug. It should have a reasonable default, say 1 or 2.
> >
> > By "it" I think you mean the preferred length? It does have a default: 1
> > (same
> > as the other dimension until you call SetPreferredSize).
> >
> > >
> > > > Of all the places that create
> > > > separators, only one uses SetPreferredSize. For n - 1 instantiations,
> > > > the orientation param is redundant (with the layout manager) and
> > > > irrelevant (you could change it to the other orientation with no change
> > > > in behavior). For the last instantiation --- the one that does use
> > > > SetPreferredSize --- the current code passes the wrong orientation
> > > > because SetPreferredSize controls the wrong dimension for that code's
> > > > purposes.
> > >
> > > How about SetPreferredSize(base::Optional<int> width,
> > > base::Optional<int> height) ? If you don't specify one of the value
> > > the default is chosen.
> >
> > While removing orientation completely? This seems ok --- not too
> > dissimilar from
> > SetPrefSize(orientation, int) --- but there are no clients that actually
> > want to
> > set both dimensions. I'm a little hesitant to allow setting the thickness
> > in
> > this way because it invites inconsistencies (I don't think we actually
> > want any
> > extra-thick separators anywhere).
> >
> > >
> > > -Scott
> > >
> > > >
> > > > tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite is
> > > > passing the wrong value, currently.
> > > >
> > > > https://codereview.chromium.org/2675983003/
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > >
> >
> >
> >
> > https://codereview.chromium.org/2675983003/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-02-08 18:27:51]

GAH! Ok, you're right. How about we remove the enum altogether and
have SetPreferredHeight/SetPreferredWidth?

On Wed, Feb 8, 2017 at 9:35 AM,  <estade@chromium.org> wrote:
> On 2017/02/08 17:26:14, sky wrote:
>> Here's the list of concerns I have with the API you've created:
>>
>> . I find it confusing that you don't explicitly specify the orientation.
>> It
>> doesn't really matter the way you have it now because the separator is a
>> fill. I don't think it's a valid assumption though. It's entirely possible
>> we would want the separator to look different depending upon the axis.
>>
>> . SetPreferredSize() is overloading the meaning of orientation to mean a
>> particular axis.
>>
>> I prefer keeping the orientation in the constructor, it makes usage clear.
>> I prefer either SetPreferredSize with two optionals, or
>> SetPreferredHeight/SetPreferredWidth. Either of these give the flexibility
>> of specifying the preferred size without overloading HORIZONTRAL/VERTICAL.
>
> When do we use orientation then? If I understand your suggestion correctly,
> the
> compiler complains:
>
> ../../ui/views/controls/separator.h:49:21: error: private field
> 'orientation_'
> is not used [-Werror,-Wunused-private-field]
> const Orientation orientation_;
>
>
>
>>
>>
>> On Wed, Feb 8, 2017 at 8:21 AM, <mailto:estade@chromium.org> wrote:
>>
>> > On 2017/02/08 00:58:26, sky wrote:
>> >
>> > > On Tue, Feb 7, 2017 at 1:39 PM, <mailto:estade@chromium.org> wrote:
>> > > >
>> > > >
>> > >
>> > https://codereview.chromium.org/2675983003/diff/20001/ui/
>> > views/controls/separator.cc
>> > > > File ui/views/controls/separator.cc (right):
>> > > >
>> > > >
>> > >
>> > https://codereview.chromium.org/2675983003/diff/20001/ui/
>> > views/controls/separator.cc#newcode39
>> > > > ui/views/controls/separator.cc:39: gfx::Size size = orientation_ ==
>> > > > HORIZONTAL ? gfx::Size(length_, kThickness)
>> > > > On 2017/02/07 19:02:49, sky wrote:
>> > > >> On 2017/02/06 21:39:17, Evan Stade wrote:
>> > > >> > On 2017/02/06 21:19:06, tdanderson wrote:
>> > > >> > > You're switching the meaning of HORIZONTAL vs VERTICAL but this
>> > > >> > > CL
>> > > > doesn't
>> > > >> > > update every call site where a Separator is instantiated.
>> > > >> >
>> > > >> > The meaning of horizontal and vertical are the same. I've changed
>> > > > the meaning
>> > > >> of
>> > > >> > "SetPreferredSize/Length" and only two places call that function,
>> > > > one of which
>> > > >> > is going away soon.
>> > > >> >
>> > > >> > Probably I could just rip out that function and the orientation
>> > > >> > enum
>> > > >> altogether
>> > > >> > (just always request 1x1 because it's almost always the layout
>> > > >> > that
>> > > > sets the
>> > > >> > length appropriately).
>> > > >
>> > > > I looked into this but it's not convenient to actually remove this
>> > > > function and rely on borders for the tray separators.
>> > > >
>> > > >>
>> > > >> I see where you are going. I find the change to separator here
>> > > > confusing. When
>> > > >> you create a separator you have an explicit orientation in mind. Is
>> > > > there some
>> > > >> use case I'm not seeing where you know you want a separator but
>> > > >> don't
>> > > > know the
>> > > >> orientation?
>> > > >
>> > > > No, there is not a case where you have a separator but you don't
>> > > > know
>> > > > the orientation. The issue is that the orientation is totally
>> > > > ignored
>> > > > unless you also call SetPreferredSize.
>> > >
>> > > That seems like the bug. It should have a reasonable default, say 1 or
>> > > 2.
>> >
>> > By "it" I think you mean the preferred length? It does have a default: 1
>> > (same
>> > as the other dimension until you call SetPreferredSize).
>> >
>> > >
>> > > > Of all the places that create
>> > > > separators, only one uses SetPreferredSize. For n - 1
>> > > > instantiations,
>> > > > the orientation param is redundant (with the layout manager) and
>> > > > irrelevant (you could change it to the other orientation with no
>> > > > change
>> > > > in behavior). For the last instantiation --- the one that does use
>> > > > SetPreferredSize --- the current code passes the wrong orientation
>> > > > because SetPreferredSize controls the wrong dimension for that
>> > > > code's
>> > > > purposes.
>> > >
>> > > How about SetPreferredSize(base::Optional<int> width,
>> > > base::Optional<int> height) ? If you don't specify one of the value
>> > > the default is chosen.
>> >
>> > While removing orientation completely? This seems ok --- not too
>> > dissimilar from
>> > SetPrefSize(orientation, int) --- but there are no clients that actually
>> > want to
>> > set both dimensions. I'm a little hesitant to allow setting the
>> > thickness
>> > in
>> > this way because it invites inconsistencies (I don't think we actually
>> > want any
>> > extra-thick separators anywhere).
>> >
>> > >
>> > > -Scott
>> > >
>> > > >
>> > > > tl;dr n - 1 callsites are passing an irrelevant param and 1 callsite
>> > > > is
>> > > > passing the wrong value, currently.
>> > > >
>> > > > https://codereview.chromium.org/2675983003/
>> > >
>> > > --
>> > > You received this message because you are subscribed to the Google
>> > > Groups
>> > > "Chromium-reviews" group.
>> > > To unsubscribe from this group and stop receiving emails from it, send
>> > > an
>> > email
>> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
>> > >
>> >
>> >
>> >
>> > https://codereview.chromium.org/2675983003/
>> >
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2675983003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2017-02-08 20:15:21]

On 2017/02/08 18:27:51, sky wrote:
> GAH! Ok, you're right. How about we remove the enum altogether and
> have SetPreferredHeight/SetPreferredWidth?

Maybe SetPreferredLength(), which looks at orientation and decides whether the
length means width or height?

[Evan Stade, 2017-02-08 21:19:41]

On 2017/02/08 20:15:21, sadrul wrote:
> On 2017/02/08 18:27:51, sky wrote:
> > GAH! Ok, you're right. How about we remove the enum altogether and
> > have SetPreferredHeight/SetPreferredWidth?
> 
> Maybe SetPreferredLength(), which looks at orientation and decides whether the
> length means width or height?

heh -- there are two versions of that here: ps2 or ps3 both do what you say in
slightly different ways.

I'm ok with ps2, ps3 or Scott's suggestion.

[sky, 2017-02-08 23:23:43]

I favor Scott's suggestion;) (and thanks for the patience here!)

  -Scott

On Wed, Feb 8, 2017 at 1:19 PM,  <estade@chromium.org> wrote:
> On 2017/02/08 20:15:21, sadrul wrote:
>> On 2017/02/08 18:27:51, sky wrote:
>> > GAH! Ok, you're right. How about we remove the enum altogether and
>> > have SetPreferredHeight/SetPreferredWidth?
>>
>> Maybe SetPreferredLength(), which looks at orientation and decides whether
>> the
>> length means width or height?
>
> heh -- there are two versions of that here: ps2 or ps3 both do what you say
> in
> slightly different ways.
>
> I'm ok with ps2, ps3 or Scott's suggestion.
>
> https://codereview.chromium.org/2675983003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-02-09 01:30:09]

Description was changed from

==========
views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator
- Improve clarity of ash tray constants.

BUG=688505
==========

to

==========
views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator by removing enum and using terms "height" and "width"
- Improve clarity of ash tray constants.

BUG=688505
==========

[Evan Stade, 2017-02-09 01:35:37]

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

[Evan Stade, 2017-02-09 01:36:07]

done, ptal

[commit-bot: I haz the power, 2017-02-09 01:36:31]

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

[sky, 2017-02-09 03:16:09]

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.cc:29: if (width == preferred_size_.width())
WDYT of calling a common (private) SetPreferredSize. I'm specifically suggesting
the two functions become something like:

SetPreferredSize(gfx::Size(width, preferred_size_.height()));

The early out and other code moves to said function.

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.h:24: // The separator's thickness in dip.
optional: Generally values are assumed to be dip unless otherwise stated, so I
wouldn't bother mentioning dip here.

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.h:28: // common case where sizing will be controlled
by the layout manager.
Please document the preferred size is kThickness in both directions.

[commit-bot: I haz the power, 2017-02-09 04:55:42]

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

[commit-bot: I haz the power, 2017-02-09 04:55:43]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-09 15:46:11]

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.cc:29: if (width == preferred_size_.width())
On 2017/02/09 03:16:09, sky wrote:
> WDYT of calling a common (private) SetPreferredSize. I'm specifically
suggesting
> the two functions become something like:
> 
> SetPreferredSize(gfx::Size(width, preferred_size_.height()));
> 
> The early out and other code moves to said function.

I was on the fence about creating a shared private function. The early out is
actually a bit silly given how this is used (it's not now and unlikely to ever
be called more than once, much less many times).

As far as allowing callers to set both height and width, this is purely
philosophical since no callers actually want to do so right now (in fact we
don't even have any callers of SetPreferredWidth at the moment). But I erred on
the side of not supporting something that doesn't yet have a use case, because
adding a new behavior such as a double-wide separator IMO should have extra
friction (i.e. updating this class).

We could always circumvent this issue by removing SetPreferredWidth and adding a
note that says "add it if you need it".

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.h:24: // The separator's thickness in dip.
On 2017/02/09 03:16:09, sky wrote:
> optional: Generally values are assumed to be dip unless otherwise stated, so I
> wouldn't bother mentioning dip here.

Yea, you're right, my desire for extra clarity here is because MD borders and
other lines are often a single pixel.

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.h:28: // common case where sizing will be controlled
by the layout manager.
On 2017/02/09 03:16:09, sky wrote:
> Please document the preferred size is kThickness in both directions.

will do

[sky, 2017-02-09 20:32:50]

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
File ui/views/controls/separator.cc (right):

https://codereview.chromium.org/2675983003/diff/80001/ui/views/controls/separ...
ui/views/controls/separator.cc:29: if (width == preferred_size_.width())
On 2017/02/09 15:46:11, Evan Stade wrote:
> On 2017/02/09 03:16:09, sky wrote:
> > WDYT of calling a common (private) SetPreferredSize. I'm specifically
> suggesting
> > the two functions become something like:
> > 
> > SetPreferredSize(gfx::Size(width, preferred_size_.height()));
> > 
> > The early out and other code moves to said function.
> 
> I was on the fence about creating a shared private function. The early out is
> actually a bit silly given how this is used (it's not now and unlikely to ever
> be called more than once, much less many times).
> 
> As far as allowing callers to set both height and width, this is purely
> philosophical since no callers actually want to do so right now (in fact we
> don't even have any callers of SetPreferredWidth at the moment). But I erred
on
> the side of not supporting something that doesn't yet have a use case, because
> adding a new behavior such as a double-wide separator IMO should have extra
> friction (i.e. updating this class).
> 
> We could always circumvent this issue by removing SetPreferredWidth and adding
a
> note that says "add it if you need it".

I'm ok with only what we need and no note. If someone needs the functionality
they'll figure they need to add it.

[Evan Stade, 2017-02-10 03:26:13]

> I'm ok with only what we need and no note. If someone needs the functionality
> they'll figure they need to add it.

done in patchset 7.

I also thought of a way to get rid of all the preferred size setters, which is
done in patchset 6. Perhaps a bit hacky.

[Evan Stade, 2017-02-10 03:26:20]

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

[commit-bot: I haz the power, 2017-02-10 03:26:51]

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, 2017-02-10 05:22:30]

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

[commit-bot: I haz the power, 2017-02-10 05:22:33]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-10 18:12:04]

LayoutManagerForSeparator is overkill.

https://codereview.chromium.org/2675983003/diff/120001/ui/views/controls/sepa...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/120001/ui/views/controls/sepa...
ui/views/controls/separator.h:40: gfx::Size preferred_size_;
At this point I wouldn't bother with a size here, instead preferred_height_.

[Evan Stade, 2017-02-10 23:23:25]

https://codereview.chromium.org/2675983003/diff/120001/ui/views/controls/sepa...
File ui/views/controls/separator.h (right):

https://codereview.chromium.org/2675983003/diff/120001/ui/views/controls/sepa...
ui/views/controls/separator.h:40: gfx::Size preferred_size_;
On 2017/02/10 18:12:04, sky wrote:
> At this point I wouldn't bother with a size here, instead preferred_height_.

Done.

[Evan Stade, 2017-02-10 23:27:45]

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

[commit-bot: I haz the power, 2017-02-10 23:28:18]

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, 2017-02-11 00:23:30]

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

[commit-bot: I haz the power, 2017-02-11 00:23:31]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-13 16:32:34]

LGTM - thanks for the simplification and seeing this through.

[Evan Stade, 2017-02-13 17:13:51]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-02-13 17:13:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2675983003/#ps140001
(title: ".")

[commit-bot: I haz the power, 2017-02-13 17:14:30]

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, 2017-02-13 18:05:23]

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

[commit-bot: I haz the power, 2017-02-13 18:05:25]

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

[Evan Stade, 2017-02-13 19:04:58]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-02-13 19:05:32]

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, 2017-02-13 20:14:55]

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

[commit-bot: I haz the power, 2017-02-13 20:14:56]

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

[Evan Stade, 2017-02-13 23:25:28]

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

[commit-bot: I haz the power, 2017-02-13 23:26:41]

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, 2017-02-14 02:09:32]

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

[commit-bot: I haz the power, 2017-02-14 02:09:33]

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

[Evan Stade, 2017-02-14 18:50:36]

On 2017/02/13 20:14:56, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
> (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

ugh, this doesn't seem to be flake but the output is mysterious. Do you have any
idea what this might indicate? Is it just crashing?

[4158:4159:0213/170659.469937:949966851:ERROR:kill_posix.cc(84)] Unable to
terminate process group 4160: No such process
[4158:4158:0213/170659.471105:949967939:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.CaptureAutoReset
[4158:4158:0213/170659.471146:949967977:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ResetCaptureOnGestureEnd
[4158:4158:0213/170659.471159:949967990:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.DisableCaptureWidgetFromMousePress
[4158:4158:0213/170659.471169:949967999:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.CheckResizeControllerEvents
[4158:4158:0213/170659.471228:949968059:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ViewFocusOnWidgetActivationChanges
[4158:4158:0213/170659.471240:949968070:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.CanActivateFlagIsHonored
[4158:4158:0213/170659.471246:949968077:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated
[4158:4158:0213/170659.471255:949968085:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.DisableViewDoesNotActivateWidget
[4158:4158:0213/170659.471261:949968091:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ShowCreatesActiveWindow
[4158:4158:0213/170659.471271:949968101:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ShowInactive
[1/49] WidgetTestInteractive.CaptureAutoReset (UNKNOWN)
[2/49] WidgetTestInteractive.ResetCaptureOnGestureEnd (UNKNOWN)
[3/49] WidgetTestInteractive.DisableCaptureWidgetFromMousePress (UNKNOWN)
[4/49] WidgetTestInteractive.CheckResizeControllerEvents (UNKNOWN)
[5/49] WidgetTestInteractive.ViewFocusOnWidgetActivationChanges (UNKNOWN)
[6/49] WidgetTestInteractive.CanActivateFlagIsHonored (UNKNOWN)
[7/49] WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated (UNKNOWN)
[8/49] WidgetTestInteractive.DisableViewDoesNotActivateWidget (UNKNOWN)
[9/49] WidgetTestInteractive.ShowCreatesActiveWindow (UNKNOWN)
[10/49] WidgetTestInteractive.ShowInactive (UNKNOWN)
[4158:4159:0213/170659.484873:949981708:ERROR:kill_posix.cc(84)] Unable to
terminate process group 4161: No such process
[4158:4158:0213/170659.485876:949982711:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.InactiveBeforeShow
[4158:4158:0213/170659.485917:949982748:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ShowInactiveAfterShow
[4158:4158:0213/170659.485928:949982758:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ShowAfterShowInactive
[4158:4158:0213/170659.485937:949982768:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.ExitFullscreenRestoreState
[4158:4158:0213/170659.485946:949982776:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetTestInteractive.InitialFocus
[4158:4158:0213/170659.485957:949982787:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetCaptureTest.Capture
[4158:4158:0213/170659.485967:949982797:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetCaptureTest.DestroyWithCapture_CloseNow
[4158:4158:0213/170659.485977:949982807:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetCaptureTest.DestroyWithCapture_Close
[4158:4158:0213/170659.485985:949982815:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget
[4158:4158:0213/170659.485998:949982828:ERROR:unit_test_launcher.cc(324)] no
test result for WidgetCaptureTest.FailedCaptureRequestIsNoop
[11/49] WidgetTestInteractive.InactiveBeforeShow (UNKNOWN)
[12/49] WidgetTestInteractive.ShowInactiveAfterShow (UNKNOWN)
[13/49] WidgetTestInteractive.ShowAfterShowInactive (UNKNOWN)
[14/49] WidgetTestInteractive.ExitFullscreenRestoreState (UNKNOWN)
[15/49] WidgetTestInteractive.InitialFocus (UNKNOWN)
[16/49] WidgetCaptureTest.Capture (UNKNOWN)
[17/49] WidgetCaptureTest.DestroyWithCapture_CloseNow (UNKNOWN)
[18/49] WidgetCaptureTest.DestroyWithCapture_Close (UNKNOWN)
[19/49] WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget (UNKNOWN)
[20/49] WidgetCaptureTest.FailedCaptureRequestIsNoop (UNKNOWN)
Too many badly broken tests (20), exiting now.
Sending SIGTERM to 1 child processes... done.
Giving processes a chance to terminate cleanly... done.
Sending SIGKILL to 1 child processes... done.

I'll try and see if the use of GetNativeTheme() is somehow the issue unless you
have a better insight.

[sky, 2017-02-14 22:46:35]

That output isn't helpful at all! I'm not sure if something hung, or
something crashed triggering all the other failures. If you get stuck
let me know and I can try your patch.

  -Scott

On Tue, Feb 14, 2017 at 10:50 AM,  <estade@chromium.org> wrote:
> On 2017/02/13 20:14:56, commit-bot: I haz the power wrote:
>> Try jobs failed on following builders:
>> linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
>> (JOB_FAILED,
>>
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>
> ugh, this doesn't seem to be flake but the output is mysterious. Do you have
> any
> idea what this might indicate? Is it just crashing?
>
> [4158:4159:0213/170659.469937:949966851:ERROR:kill_posix.cc(84)] Unable to
> terminate process group 4160: No such process
> [4158:4158:0213/170659.471105:949967939:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.CaptureAutoReset
> [4158:4158:0213/170659.471146:949967977:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ResetCaptureOnGestureEnd
> [4158:4158:0213/170659.471159:949967990:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.DisableCaptureWidgetFromMousePress
> [4158:4158:0213/170659.471169:949967999:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.CheckResizeControllerEvents
> [4158:4158:0213/170659.471228:949968059:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ViewFocusOnWidgetActivationChanges
> [4158:4158:0213/170659.471240:949968070:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.CanActivateFlagIsHonored
> [4158:4158:0213/170659.471246:949968077:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated
> [4158:4158:0213/170659.471255:949968085:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.DisableViewDoesNotActivateWidget
> [4158:4158:0213/170659.471261:949968091:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ShowCreatesActiveWindow
> [4158:4158:0213/170659.471271:949968101:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ShowInactive
> [1/49] WidgetTestInteractive.CaptureAutoReset (UNKNOWN)
> [2/49] WidgetTestInteractive.ResetCaptureOnGestureEnd (UNKNOWN)
> [3/49] WidgetTestInteractive.DisableCaptureWidgetFromMousePress (UNKNOWN)
> [4/49] WidgetTestInteractive.CheckResizeControllerEvents (UNKNOWN)
> [5/49] WidgetTestInteractive.ViewFocusOnWidgetActivationChanges (UNKNOWN)
> [6/49] WidgetTestInteractive.CanActivateFlagIsHonored (UNKNOWN)
> [7/49] WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated (UNKNOWN)
> [8/49] WidgetTestInteractive.DisableViewDoesNotActivateWidget (UNKNOWN)
> [9/49] WidgetTestInteractive.ShowCreatesActiveWindow (UNKNOWN)
> [10/49] WidgetTestInteractive.ShowInactive (UNKNOWN)
> [4158:4159:0213/170659.484873:949981708:ERROR:kill_posix.cc(84)] Unable to
> terminate process group 4161: No such process
> [4158:4158:0213/170659.485876:949982711:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.InactiveBeforeShow
> [4158:4158:0213/170659.485917:949982748:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ShowInactiveAfterShow
> [4158:4158:0213/170659.485928:949982758:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ShowAfterShowInactive
> [4158:4158:0213/170659.485937:949982768:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.ExitFullscreenRestoreState
> [4158:4158:0213/170659.485946:949982776:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetTestInteractive.InitialFocus
> [4158:4158:0213/170659.485957:949982787:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetCaptureTest.Capture
> [4158:4158:0213/170659.485967:949982797:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetCaptureTest.DestroyWithCapture_CloseNow
> [4158:4158:0213/170659.485977:949982807:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetCaptureTest.DestroyWithCapture_Close
> [4158:4158:0213/170659.485985:949982815:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget
> [4158:4158:0213/170659.485998:949982828:ERROR:unit_test_launcher.cc(324)] no
> test result for WidgetCaptureTest.FailedCaptureRequestIsNoop
> [11/49] WidgetTestInteractive.InactiveBeforeShow (UNKNOWN)
> [12/49] WidgetTestInteractive.ShowInactiveAfterShow (UNKNOWN)
> [13/49] WidgetTestInteractive.ShowAfterShowInactive (UNKNOWN)
> [14/49] WidgetTestInteractive.ExitFullscreenRestoreState (UNKNOWN)
> [15/49] WidgetTestInteractive.InitialFocus (UNKNOWN)
> [16/49] WidgetCaptureTest.Capture (UNKNOWN)
> [17/49] WidgetCaptureTest.DestroyWithCapture_CloseNow (UNKNOWN)
> [18/49] WidgetCaptureTest.DestroyWithCapture_Close (UNKNOWN)
> [19/49] WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget
> (UNKNOWN)
> [20/49] WidgetCaptureTest.FailedCaptureRequestIsNoop (UNKNOWN)
> Too many badly broken tests (20), exiting now.
> Sending SIGTERM to 1 child processes... done.
> Giving processes a chance to terminate cleanly... done.
> Sending SIGKILL to 1 child processes... done.
>
> I'll try and see if the use of GetNativeTheme() is somehow the issue unless
> you
> have a better insight.
>
> https://codereview.chromium.org/2675983003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2017-02-15 00:51:33]

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

[commit-bot: I haz the power, 2017-02-15 00:52:33]

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, 2017-02-15 03:12:22]

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

[commit-bot: I haz the power, 2017-02-15 03:12:23]

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

[Evan Stade, 2017-02-15 16:27:45]

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

[Evan Stade, 2017-02-15 16:27:58]

On 2017/02/14 22:46:35, sky wrote:
> That output isn't helpful at all! I'm not sure if something hung, or
> something crashed triggering all the other failures. If you get stuck
> let me know and I can try your patch.
> 
>   -Scott
> 
> On Tue, Feb 14, 2017 at 10:50 AM,  <mailto:estade@chromium.org> wrote:
> > On 2017/02/13 20:14:56, commit-bot: I haz the power wrote:
> >> Try jobs failed on following builders:
> >> linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
> >> (JOB_FAILED,
> >>
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> >
> > ugh, this doesn't seem to be flake but the output is mysterious. Do you have
> > any
> > idea what this might indicate? Is it just crashing?
> >
> > [4158:4159:0213/170659.469937:949966851:ERROR:kill_posix.cc(84)] Unable to
> > terminate process group 4160: No such process
> > [4158:4158:0213/170659.471105:949967939:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.CaptureAutoReset
> > [4158:4158:0213/170659.471146:949967977:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ResetCaptureOnGestureEnd
> > [4158:4158:0213/170659.471159:949967990:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.DisableCaptureWidgetFromMousePress
> > [4158:4158:0213/170659.471169:949967999:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.CheckResizeControllerEvents
> > [4158:4158:0213/170659.471228:949968059:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ViewFocusOnWidgetActivationChanges
> > [4158:4158:0213/170659.471240:949968070:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.CanActivateFlagIsHonored
> > [4158:4158:0213/170659.471246:949968077:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated
> > [4158:4158:0213/170659.471255:949968085:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.DisableViewDoesNotActivateWidget
> > [4158:4158:0213/170659.471261:949968091:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ShowCreatesActiveWindow
> > [4158:4158:0213/170659.471271:949968101:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ShowInactive
> > [1/49] WidgetTestInteractive.CaptureAutoReset (UNKNOWN)
> > [2/49] WidgetTestInteractive.ResetCaptureOnGestureEnd (UNKNOWN)
> > [3/49] WidgetTestInteractive.DisableCaptureWidgetFromMousePress (UNKNOWN)
> > [4/49] WidgetTestInteractive.CheckResizeControllerEvents (UNKNOWN)
> > [5/49] WidgetTestInteractive.ViewFocusOnWidgetActivationChanges (UNKNOWN)
> > [6/49] WidgetTestInteractive.CanActivateFlagIsHonored (UNKNOWN)
> > [7/49] WidgetTestInteractive.TouchSelectionQuickMenuIsNotActivated (UNKNOWN)
> > [8/49] WidgetTestInteractive.DisableViewDoesNotActivateWidget (UNKNOWN)
> > [9/49] WidgetTestInteractive.ShowCreatesActiveWindow (UNKNOWN)
> > [10/49] WidgetTestInteractive.ShowInactive (UNKNOWN)
> > [4158:4159:0213/170659.484873:949981708:ERROR:kill_posix.cc(84)] Unable to
> > terminate process group 4161: No such process
> > [4158:4158:0213/170659.485876:949982711:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.InactiveBeforeShow
> > [4158:4158:0213/170659.485917:949982748:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ShowInactiveAfterShow
> > [4158:4158:0213/170659.485928:949982758:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ShowAfterShowInactive
> > [4158:4158:0213/170659.485937:949982768:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.ExitFullscreenRestoreState
> > [4158:4158:0213/170659.485946:949982776:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetTestInteractive.InitialFocus
> > [4158:4158:0213/170659.485957:949982787:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetCaptureTest.Capture
> > [4158:4158:0213/170659.485967:949982797:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetCaptureTest.DestroyWithCapture_CloseNow
> > [4158:4158:0213/170659.485977:949982807:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetCaptureTest.DestroyWithCapture_Close
> > [4158:4158:0213/170659.485985:949982815:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget
> > [4158:4158:0213/170659.485998:949982828:ERROR:unit_test_launcher.cc(324)] no
> > test result for WidgetCaptureTest.FailedCaptureRequestIsNoop
> > [11/49] WidgetTestInteractive.InactiveBeforeShow (UNKNOWN)
> > [12/49] WidgetTestInteractive.ShowInactiveAfterShow (UNKNOWN)
> > [13/49] WidgetTestInteractive.ShowAfterShowInactive (UNKNOWN)
> > [14/49] WidgetTestInteractive.ExitFullscreenRestoreState (UNKNOWN)
> > [15/49] WidgetTestInteractive.InitialFocus (UNKNOWN)
> > [16/49] WidgetCaptureTest.Capture (UNKNOWN)
> > [17/49] WidgetCaptureTest.DestroyWithCapture_CloseNow (UNKNOWN)
> > [18/49] WidgetCaptureTest.DestroyWithCapture_Close (UNKNOWN)
> > [19/49] WidgetCaptureTest.DestroyWithCapture_WidgetOwnsNativeWidget
> > (UNKNOWN)
> > [20/49] WidgetCaptureTest.FailedCaptureRequestIsNoop (UNKNOWN)
> > Too many badly broken tests (20), exiting now.
> > Sending SIGTERM to 1 child processes... done.
> > Giving processes a chance to terminate cleanly... done.
> > Sending SIGKILL to 1 child processes... done.
> >
> > I'll try and see if the use of GetNativeTheme() is somehow the issue unless
> > you
> > have a better insight.
> >
> > https://codereview.chromium.org/2675983003/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

well, removing the GetNativeTheme call from OnPaint did green the ozone bot, but
I don't know why. When I build with ozone locally the tests pass. I restored the
GetNativeTheme call and re-ran the patch against ozone and now it's failing on
the re-compile should-be-no-op step.

[commit-bot: I haz the power, 2017-02-15 16:28:13]

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, 2017-02-15 16:55:54]

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

[commit-bot: I haz the power, 2017-02-15 16:55:56]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-16 02:44:22]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2017-02-16 02:44:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2675983003/#ps180001
(title: "revert test change")

[commit-bot: I haz the power, 2017-02-16 02:46:09]

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, 2017-02-16 03:45:18]

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

[commit-bot: I haz the power, 2017-02-16 03:45:20]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Evan Stade, 2017-02-16 19:28:49]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2017-02-16 19:30:04]

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, 2017-02-16 20:35:12]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487273329441030,
"parent_rev": "9ef301c52015be2aefde760bc560de502b2b94e1", "commit_rev":
"4a3ae74ccfbe8470cd8343ad7e93ed27156171de"}

[commit-bot: I haz the power, 2017-02-16 20:35:47]

Description was changed from

==========
views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator by removing enum and using terms "height" and "width"
- Improve clarity of ash tray constants.

BUG=688505
==========

to

==========
views::Separator cleanup.

- Make views::Separators always draw an integral number of pixels (round
  down for fractional scale factors).
- Improve clarity of "size", "horizontal", and "vertical" in
  views::Separator by removing enum and using terms "height" and "width"
- Improve clarity of ash tray constants.

BUG=688505

Review-Url: https://codereview.chromium.org/2675983003
Cr-Commit-Position: refs/heads/master@{#451088}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a3ae74ccfbe8470cd8343ad7e93...
==========

[commit-bot: I haz the power, 2017-02-16 20:35:52]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/4a3ae74ccfbe8470cd8343ad7e93...