Add ability to specify extra insets for a view via its border.

Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none

Committed: https://crrev.com/90ff5242b9641a6c0263f65b6c830ba754d96f39
Cr-Commit-Position: refs/heads/master@{#430178}

[Evan Stade, 2016-11-03 17:58:45]

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

[Evan Stade, 2016-11-03 17:58:45]

+sky, wdyt?

At first I created a function that composed two borders (and painted them both),
but I realized that didn't make a whole lot of sense because given how Paint()
works, they'd paint on top of each other. This is probably not what most people
want. We'd have to change Paint() to take a canvas and bounds, but that's
wouldn't be that easy. I figured this approach (effectively composing any border
with an "empty" border) would be a good start.

[sky, 2016-11-03 19:24:49]

https://codereview.chromium.org/2475033003/diff/1/ui/views/border.h
File ui/views/border.h (right):

https://codereview.chromium.org/2475033003/diff/1/ui/views/border.h#newcode76
ui/views/border.h:76: // Adds extra, unpainted insets to |border|.
This seems like a nice way to avoid changing all borders. How about a better
comment though with an example?

[Evan Stade, 2016-11-04 03:16:15]

Description was changed from

==========
Add ability to specify extra insets for a view via its border.

BUG=none
==========

to

==========
Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none
==========

[Evan Stade, 2016-11-04 03:16:39]

https://codereview.chromium.org/2475033003/diff/1/ui/views/border.h
File ui/views/border.h (right):

https://codereview.chromium.org/2475033003/diff/1/ui/views/border.h#newcode76
ui/views/border.h:76: // Adds extra, unpainted insets to |border|.
On 2016/11/03 19:24:49, sky wrote:
> This seems like a nice way to avoid changing all borders. How about a better
> comment though with an example?

OK, I tried (I also tried some ASCII art but gave up after flailing for a bit).

[Evan Stade, 2016-11-04 03:16:43]

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

[commit-bot: I haz the power, 2016-11-04 03:17:38]

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

[commit-bot: I haz the power, 2016-11-04 03:20:48]

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

[commit-bot: I haz the power, 2016-11-04 03:20:49]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sky, 2016-11-04 15:32:53]

LGTM

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

https://codereview.chromium.org/2475033003/diff/20001/ui/views/border.h#newco...
ui/views/border.h:77: // adding an EmptyBorder and then |border|. Example:
optional:
I would take this second sentence to mean |border| is painted on the inside of
the EmptyBorder, which isn't quite what is happening. Maybe something like:

Creates a new border that draws |border| and adds additional padding. This is
equivalent to changing the insets of |border| without changing how or what it
paints.

I favor a name of WrapBorderWithPadding, but I realize naming is hard and leave
it to you. I would actually prefer Create in the name, but it's hard to get
something that sounds reasonable.

[Evan Stade, 2016-11-04 15:50:16]

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

https://codereview.chromium.org/2475033003/diff/20001/ui/views/border.h#newco...
ui/views/border.h:77: // adding an EmptyBorder and then |border|. Example:
On 2016/11/04 15:32:53, sky wrote:
> optional:
> I would take this second sentence to mean |border| is painted on the inside of
> the EmptyBorder, which isn't quite what is happening. Maybe something like:
> 
> Creates a new border that draws |border| and adds additional padding. This is
> equivalent to changing the insets of |border| without changing how or what it
> paints.

ok

> 
> I favor a name of WrapBorderWithPadding, but I realize naming is hard and
leave
> it to you. I would actually prefer Create in the name, but it's hard to get
> something that sounds reasonable.

CreatePaddedBorder?

while we're at it, how can we make this less verbose:

  views::Border::CreateRoundedRectBorder

Options that come to mind:

a) views::CreateRoundedRectBorder (promote to standalone static fn in views
namespace)
b) views::Border::CreateRoundedRect (remove trailing "Border")

[sky, 2016-11-04 16:04:38]

On Fri, Nov 4, 2016 at 8:50 AM, <estade@chromium.org> wrote:

>
> https://codereview.chromium.org/2475033003/diff/20001/ui/views/border.h
> File ui/views/border.h (right):
>
> https://codereview.chromium.org/2475033003/diff/20001/ui/
> views/border.h#newcode77
> ui/views/border.h:77: // adding an EmptyBorder and then |border|.
> Example:
> On 2016/11/04 15:32:53, sky wrote:
> > optional:
> > I would take this second sentence to mean |border| is painted on the
> inside of
> > the EmptyBorder, which isn't quite what is happening. Maybe something
> like:
> >
> > Creates a new border that draws |border| and adds additional padding.
> This is
> > equivalent to changing the insets of |border| without changing how or
> what it
> > paints.
>
> ok
>
> >
> > I favor a name of WrapBorderWithPadding, but I realize naming is hard
> and leave
> > it to you. I would actually prefer Create in the name, but it's hard
> to get
> > something that sounds reasonable.
>
> CreatePaddedBorder?
>
> while we're at it, how can we make this less verbose:
>
> views::Border::CreateRoundedRectBorder
>
> Options that come to mind:
>
> a) views::CreateRoundedRectBorder (promote to standalone static fn in
> views namespace)
> b) views::Border::CreateRoundedRect (remove trailing "Border")
>

I'm ok with either. I slightly prefer keeping border in the name, which
says go with a, but I'm ok with either.


>
> https://codereview.chromium.org/2475033003/
>

-- 
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, 2016-11-05 01:26:41]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-05 01:26:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2475033003/#ps40001
(title: "rebase, adopt sky's comment")

[commit-bot: I haz the power, 2016-11-05 01:27:01]

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

[commit-bot: I haz the power, 2016-11-05 02:10:29]

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

[commit-bot: I haz the power, 2016-11-05 02:10:30]

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, 2016-11-05 21:38:27]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-11-05 21:38:28]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2475033003/#ps60001
(title: "restore lost changes")

[commit-bot: I haz the power, 2016-11-05 21:38:40]

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

[commit-bot: I haz the power, 2016-11-06 03:49:53]

Description was changed from

==========
Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none
==========

to

==========
Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none
==========

[commit-bot: I haz the power, 2016-11-06 03:49:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-06 03:53:28]

Description was changed from

==========
Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none
==========

to

==========
Add ability to specify extra insets for a view via its border.

Also apply this new superpower in two places.

BUG=none

Committed: https://crrev.com/90ff5242b9641a6c0263f65b6c830ba754d96f39
Cr-Commit-Position: refs/heads/master@{#430178}
==========

[commit-bot: I haz the power, 2016-11-06 03:53:29]

Patchset 4 (id:??) landed as
https://crrev.com/90ff5242b9641a6c0263f65b6c830ba754d96f39
Cr-Commit-Position: refs/heads/master@{#430178}