views::Border: Added CreateRoundedRectBorder.

views::Border: Added CreateRoundedRectBorder.

This draws a rounded rectangle, as opposed to CreateSolidBorder which
draws square corners.

Also adds a floating-point overload to Canvas::DrawRoundRect.

BUG=548552, 553726
Committed: https://crrev.com/9a713fe13ad0501f25c981ef8c2c449bb4da235e
Cr-Commit-Position: refs/heads/master@{#358767}

[Matt Giuca, 2015-10-28 08:08:59]

mgiuca@chromium.org changed reviewers:
+ sadrul@chromium.org

[sadrul, 2015-10-29 06:03:26]

lgtm

https://codereview.chromium.org/1428623005/diff/20001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/20001/ui/views/border.cc#newc...
ui/views/border.cc:104: return gfx::Size(thickness_, thickness_);
thickness_ * 2, right?

[Matt Giuca, 2015-10-29 06:40:43]

https://codereview.chromium.org/1428623005/diff/20001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/20001/ui/views/border.cc#newc...
ui/views/border.cc:104: return gfx::Size(thickness_, thickness_);
On 2015/10/29 06:03:25, sadrul wrote:
> thickness_ * 2, right?

Done. Thanks.

[Evan Stade, 2015-10-29 18:36:18]

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

[Evan Stade, 2015-10-29 18:36:18]

in my non-OWNER opinion, you should put this in the file where you need it until
you need it more broadly across Chrome.

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
I don't think this is correct. For thickness_ == 1, you actually want to inset
by 0.5, not 0.

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:95: view.height() - thickness_),
I would do

gfx::Rect bounds = view.GetLocalBounds();
bounds.Inset(half_thickness, half_thickness);

[Matt Giuca, 2015-10-29 21:59:24]

On 2015/10/29 18:36:18, Evan Stade wrote:
> in my non-OWNER opinion, you should put this in the file where you need it
until
> you need it more broadly across Chrome.

The problem is that *I* am not likely to need it, but it seems generally useful.

I understand the basic YAGNI principle, but this seems so fundamental
(especially now that MD is mandating rounded rectangles everywhere) that it
should be in Border. I was surprised when I went to look here that I didn't find
it.

The problem if everyone takes the attitude of "you should put this where you
need it until it's needed more broadly" is that it makes it very hard to find
other implementations of the same thing, so you end up with duplicated code.

[sadrul, 2015-10-29 22:01:45]

On 2015/10/29 21:59:24, Matt Giuca wrote:
> On 2015/10/29 18:36:18, Evan Stade wrote:
> > in my non-OWNER opinion, you should put this in the file where you need it
> until
> > you need it more broadly across Chrome.
> 
> The problem is that *I* am not likely to need it, but it seems generally
useful.
> 

Oh, I don't think we should add this now unless we already have a need for this.

> I understand the basic YAGNI principle, but this seems so fundamental
> (especially now that MD is mandating rounded rectangles everywhere) that it
> should be in Border. I was surprised when I went to look here that I didn't
find
> it.
> 
> The problem if everyone takes the attitude of "you should put this where you
> need it until it's needed more broadly" is that it makes it very hard to find
> other implementations of the same thing, so you end up with duplicated code.

[Evan Stade, 2015-10-29 22:28:13]

> The problem if everyone takes the attitude of "you should put this where you
> need it until it's needed more broadly" is that it makes it very hard to find
> other implementations of the same thing, so you end up with duplicated code.

Yes that's a drawback, but hopefully people look around at the UI and say "oh
that's what I want to do" then they go find the code for that and refactor as
necessary. This one reason OWNERs exist to point out code an engineer may not
have thought of or found.

> Oh, I don't think we should add this now unless we already have a need for
this.

We do have a need for it in exactly one place at the moment (maybe more later,
but unclear).

[Matt Giuca, 2015-11-02 03:30:49]

On 2015/10/29 22:28:13, Evan Stade (ooo till Monday) wrote:
> Yes that's a drawback, but hopefully people look around at the UI and say "oh
> that's what I want to do" then they go find the code for that and refactor as
> necessary. This one reason OWNERs exist to point out code an engineer may not
> have thought of or found.

But the problem is that if I add this in an obscure location
(ExclusiveAccessBubble) and someone wants a rounded rect border in a completely
different view, there may not even be any overlapping OWNERS who would know to
look in ExclusiveAccessBubble. Whereas if it was in views::Border, I would have
found it during a quick search to see if anything already satisfies what I want.

> > Oh, I don't think we should add this now unless we already have a need for
> this.
> 
> We do have a need for it in exactly one place at the moment (maybe more later,
> but unclear).

sadrul@: It's not clear if you mean "unless we have a need for it outside of
your immediate use case" (which we don't, yet), or if you mean "unless we have a
need for it at all" (which we do --
https://codereview.chromium.org/1421813005/).

[sadrul, 2015-11-03 19:20:00]

On 2015/11/02 03:30:49, Matt Giuca wrote:
> On 2015/10/29 22:28:13, Evan Stade (ooo till Monday) wrote:
> > Yes that's a drawback, but hopefully people look around at the UI and say
"oh
> > that's what I want to do" then they go find the code for that and refactor
as
> > necessary. This one reason OWNERs exist to point out code an engineer may
not
> > have thought of or found.
> 
> But the problem is that if I add this in an obscure location
> (ExclusiveAccessBubble) and someone wants a rounded rect border in a
completely
> different view, there may not even be any overlapping OWNERS who would know to
> look in ExclusiveAccessBubble. Whereas if it was in views::Border, I would
have
> found it during a quick search to see if anything already satisfies what I
want.
> 
> > > Oh, I don't think we should add this now unless we already have a need for
> > this.
> > 
> > We do have a need for it in exactly one place at the moment (maybe more
later,
> > but unclear).
> 
> sadrul@: It's not clear if you mean "unless we have a need for it outside of
> your immediate use case" (which we don't, yet), or if you mean "unless we have
a
> need for it at all" (which we do --
> https://codereview.chromium.org/1421813005/).

I meant the latter. If this is going to be used at least somewhere, then I do
not object to adding it in //ui/views/. Views already contains quite a bit of
code that doesn't necessarily have to be in views core, e.g. the various other
border/background/layout etc. implementations, various controls, etc. could be
separate smaller components, that users can selectively chose to include. I
share the concern about discoverability if this lives in chrome/ (especially
because views comes with a number of other implementations for Border).

[Matt Giuca, 2015-11-03 23:11:15]

On 2015/11/03 19:20:00, sadrul wrote:
> I meant the latter. If this is going to be used at least somewhere, then I do
> not object to adding it in //ui/views/. Views already contains quite a bit of
> code that doesn't necessarily have to be in views core, e.g. the various other
> border/background/layout etc. implementations, various controls, etc. could be
> separate smaller components, that users can selectively chose to include. I
> share the concern about discoverability if this lives in chrome/ (especially
> because views comes with a number of other implementations for Border).

Thanks for the explanation. I will land it then.

[Matt Giuca, 2015-11-03 23:11:21]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-11-03 23:11:21]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1428623005/#ps40001
(title: "Fix GetMinimumSize.")

[commit-bot: I haz the power, 2015-11-03 23:11:56]

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

[commit-bot: I haz the power, 2015-11-03 23:11:57]

This CL has an open dependency (Issue 1422703004 Patch 1). Please resolve the
dependency and try again.

[Evan Stade, 2015-11-03 23:46:42]

I had a couple comments on the implementation that I think might warrant
addressing.

[Matt Giuca, 2015-11-04 02:13:53]

On 2015/11/03 23:46:42, Evan Stade (ooo) wrote:
> I had a couple comments on the implementation that I think might warrant
> addressing.

Oh sorry I missed those.

[Matt Giuca, 2015-11-09 04:44:57]

Will await estade's lg (though not required for OWNERS).

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
On 2015/10/29 18:36:18, Evan Stade wrote:
> I don't think this is correct. For thickness_ == 1, you actually want to inset
> by 0.5, not 0.

Ah good catch. Well I can't use floating point numbers (DrawRoundRect takes an
int Rect). Instead just rounding UP instead of down.

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:95: view.height() - thickness_),
On 2015/10/29 18:36:18, Evan Stade wrote:
> I would do
> 
> gfx::Rect bounds = view.GetLocalBounds();
> bounds.Inset(half_thickness, half_thickness);

That's neat. Done.

[Evan Stade, 2015-11-09 18:41:48]

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
On 2015/11/09 04:44:57, Matt Giuca wrote:
> On 2015/10/29 18:36:18, Evan Stade wrote:
> > I don't think this is correct. For thickness_ == 1, you actually want to
inset
> > by 0.5, not 0.
> 
> Ah good catch. Well I can't use floating point numbers (DrawRoundRect takes an
> int Rect). Instead just rounding UP instead of down.

No, you really do need to use floats, or any odd-numbered thickness value will
be uncrisp (e.g. 1px/black will be two grey pixels instead of 1 black pixel).
Luckily, DrawRoundRect is just a thin wrapper around drawRoundRect, which takes
floats.

gfx::RectF bounds(view.GetLocalBounds());
bounds.Inset(...);
canvas->sk_canvas()->drawRoundRect(...);

[Matt Giuca, 2015-11-10 00:15:06]

sadrul@: PTAL for Canvas changes. (See discussion below.)

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
On 2015/11/09 18:41:48, Evan Stade wrote:
> On 2015/11/09 04:44:57, Matt Giuca wrote:
> > On 2015/10/29 18:36:18, Evan Stade wrote:
> > > I don't think this is correct. For thickness_ == 1, you actually want to
> inset
> > > by 0.5, not 0.
> > 
> > Ah good catch. Well I can't use floating point numbers (DrawRoundRect takes
an
> > int Rect). Instead just rounding UP instead of down.
> 
> No, you really do need to use floats, or any odd-numbered thickness value will
> be uncrisp (e.g. 1px/black will be two grey pixels instead of 1 black pixel).
> Luckily, DrawRoundRect is just a thin wrapper around drawRoundRect, which
takes
> floats.
> 
> gfx::RectF bounds(view.GetLocalBounds());
> bounds.Inset(...);
> canvas->sk_canvas()->drawRoundRect(...);

Oh, yeah that fixes the 1px problem I was having (on the other CL). I looked for
a DrawRoundRect overload for this yesterday but didn't find one in Canvas.

It's actually kind of bad that all of these methods in Canvas take ints and
convert them to floats internally. At least for DrawRoundRect (but I suspect for
others), that is *never* desirable because of the above issue. So I decided to
add a float overload of DrawRoundRect rather than just bypassing the abstraction
layer.

[Evan Stade, 2015-11-10 00:22:06]

lgtm

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
On 2015/11/10 00:15:06, Matt Giuca wrote:
> On 2015/11/09 18:41:48, Evan Stade wrote:
> > On 2015/11/09 04:44:57, Matt Giuca wrote:
> > > On 2015/10/29 18:36:18, Evan Stade wrote:
> > > > I don't think this is correct. For thickness_ == 1, you actually want to
> > inset
> > > > by 0.5, not 0.
> > > 
> > > Ah good catch. Well I can't use floating point numbers (DrawRoundRect
takes
> an
> > > int Rect). Instead just rounding UP instead of down.
> > 
> > No, you really do need to use floats, or any odd-numbered thickness value
will
> > be uncrisp (e.g. 1px/black will be two grey pixels instead of 1 black
pixel).
> > Luckily, DrawRoundRect is just a thin wrapper around drawRoundRect, which
> takes
> > floats.
> > 
> > gfx::RectF bounds(view.GetLocalBounds());
> > bounds.Inset(...);
> > canvas->sk_canvas()->drawRoundRect(...);
> 
> Oh, yeah that fixes the 1px problem I was having (on the other CL). I looked
for
> a DrawRoundRect overload for this yesterday but didn't find one in Canvas.
> 
> It's actually kind of bad that all of these methods in Canvas take ints and
> convert them to floats internally. At least for DrawRoundRect (but I suspect
for
> others), that is *never* desirable because of the above issue. So I decided to
> add a float overload of DrawRoundRect rather than just bypassing the
abstraction
> layer.

Yea, I was thinking the same thing, although I'd take it a step further and
deprecate the int version.

[Matt Giuca, 2015-11-10 00:28:29]

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc
File ui/views/border.cc (right):

https://codereview.chromium.org/1428623005/diff/40001/ui/views/border.cc#newc...
ui/views/border.cc:92: int half_thickness = thickness_ / 2;
On 2015/11/10 00:22:06, Evan Stade wrote:
> On 2015/11/10 00:15:06, Matt Giuca wrote:
> > On 2015/11/09 18:41:48, Evan Stade wrote:
> > > On 2015/11/09 04:44:57, Matt Giuca wrote:
> > > > On 2015/10/29 18:36:18, Evan Stade wrote:
> > > > > I don't think this is correct. For thickness_ == 1, you actually want
to
> > > inset
> > > > > by 0.5, not 0.
> > > > 
> > > > Ah good catch. Well I can't use floating point numbers (DrawRoundRect
> takes
> > an
> > > > int Rect). Instead just rounding UP instead of down.
> > > 
> > > No, you really do need to use floats, or any odd-numbered thickness value
> will
> > > be uncrisp (e.g. 1px/black will be two grey pixels instead of 1 black
> pixel).
> > > Luckily, DrawRoundRect is just a thin wrapper around drawRoundRect, which
> > takes
> > > floats.
> > > 
> > > gfx::RectF bounds(view.GetLocalBounds());
> > > bounds.Inset(...);
> > > canvas->sk_canvas()->drawRoundRect(...);
> > 
> > Oh, yeah that fixes the 1px problem I was having (on the other CL). I looked
> for
> > a DrawRoundRect overload for this yesterday but didn't find one in Canvas.
> > 
> > It's actually kind of bad that all of these methods in Canvas take ints and
> > convert them to floats internally. At least for DrawRoundRect (but I suspect
> for
> > others), that is *never* desirable because of the above issue. So I decided
to
> > add a float overload of DrawRoundRect rather than just bypassing the
> abstraction
> > layer.
> 
> Yea, I was thinking the same thing, although I'd take it a step further and
> deprecate the int version.

OK, I am "weakly" deprecating it by adding a comment recommending to use the
float version. Is the comment too cryptic? (Trying to keep it terse.)

[Evan Stade, 2015-11-10 00:34:32]

https://codereview.chromium.org/1428623005/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/1428623005/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:254: // odd-numbered thickness.
I don't even think you need to write this much. I'd put:

// DEPRECATED in favor of the RectF version below. TODO(mgiuca): remove this.

(don't worry, I don't think TODO(mgiuca) means you're signing up to fix all of
Chrome.) Also if you file a bug about this it's fairly likely an external
contributor will pick it up.

[sadrul, 2015-11-10 00:48:58]

Some notes for follow-up cleanup candidates, but otherwise lgtm.

. I suppose we can't just remove the Rect version because we don't have explicit
Rect->RectF conversion anymore? Looks like there's only a handful uses of
DrawRoundRect(). So updating shouldn't be hard.

. A small number of places directly use canvas->sk_canvas()->drawRoundRect().
Would be nice to fix those to use canvas->DrawRoundRect() instead.

. Looking at the current uses of DrawRoundRect(), it looks like some of these
uses are essentially drawing a rounded border ... perhaps we could use this new
RoundedRectBorder in those places too (e.g. ExitWarningWidgetDelegateView,
StickyKeysOverlayView, IdleAppNameNotificationDelegateView etc.)

[Matt Giuca, 2015-11-10 00:53:15]

Thanks for the follow-up review, Sadrul. I'll paste those notes onto the bug.

I'm a few yaks deep right now so I won't commit to that cleanup, but I might
take a look when I get some spare cycles.

https://codereview.chromium.org/1428623005/diff/100001/ui/gfx/canvas.h
File ui/gfx/canvas.h (right):

https://codereview.chromium.org/1428623005/diff/100001/ui/gfx/canvas.h#newcod...
ui/gfx/canvas.h:254: // odd-numbered thickness.
On 2015/11/10 00:34:32, Evan Stade wrote:
> I don't even think you need to write this much. I'd put:
> 
> // DEPRECATED in favor of the RectF version below. TODO(mgiuca): remove this.
> 
> (don't worry, I don't think TODO(mgiuca) means you're signing up to fix all of
> Chrome.) Also if you file a bug about this it's fairly likely an external
> contributor will pick it up.

Done.

[Matt Giuca, 2015-11-10 00:53:54]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2015-11-10 00:53:55]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/1428623005/#ps120001
(title: "Reword deprecated notice.")

[commit-bot: I haz the power, 2015-11-10 00:54:42]

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

[commit-bot: I haz the power, 2015-11-10 02:53:38]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-10 02:54:19]

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