views: Make View::set_border() take a scoped_ptr<>.

views: Make View::set_border() take a scoped_ptr<>.

This makes the ownership situation much clearer. It also renames the function to SetBorder() (since it is not a simple accessor and can free previous instances of Border), and creates a Border::NullBorder() method (since "SetBorder(scoped_ptr<>())" is not clear).

BUG=none
R=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246990

[Elliot Glaysher, 2014-01-23 02:09:08]

Touches 112 files in the end.

(Though the renaming of methods probably exasperated that.)

[sky, 2014-01-23 15:30:52]

https://codereview.chromium.org/145033006/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc (right):

https://codereview.chromium.org/145033006/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_sync_promo_view.cc:38: 0,
nit: fix indentation.

https://codereview.chromium.org/145033006/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/message_center/message_center_frame_view.cc
(right):

https://codereview.chromium.org/145033006/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/message_center/message_center_frame_view.cc:22:
message_center::kMessageCenterShadowColor,
nit: fix indentation

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h#newcode517
ui/views/view.h:517: void ClearBorder();
Why do we need ClearBorder? I get that it saves some code, but from an API
standpoint I don't like it. I prefer callers use SetBorder with an empty
scopedptr.

[Elliot Glaysher, 2014-01-23 18:55:25]

ran "git cl format" on the patch, which I just realized was pushed out to
developers machines...

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h#newcode517
ui/views/view.h:517: void ClearBorder();
On 2014/01/23 15:30:54, sky wrote:
> Why do we need ClearBorder? I get that it saves some code, but from an API
> standpoint I don't like it. I prefer callers use SetBorder with an empty
> scopedptr.

Because it's really visually ugly and not clear at a glance.

It was a common pattern in our code to do "set_border(NULL);" to clear a border.
That's not great semantically, but is still tolerable.

"SetBorder(scoped_ptr<views::Border>());" isn't immediately clear like NULL is,
and it's generally better to have function names document what they're doing (in
this case, we're almost always clearing a border set by our base class).

[sky, 2014-01-23 19:39:34]

+ben for a third opinion. Elliot wants to add a ClearBorder() function, which is
the same as SetBorder(scoped_ptr<Border>()); See thread for arguments for and
against.

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h
File ui/views/view.h (right):

https://codereview.chromium.org/145033006/diff/60001/ui/views/view.h#newcode517
ui/views/view.h:517: void ClearBorder();
On 2014/01/23 18:55:26, Elliot Glaysher wrote:
> On 2014/01/23 15:30:54, sky wrote:
> > Why do we need ClearBorder? I get that it saves some code, but from an API
> > standpoint I don't like it. I prefer callers use SetBorder with an empty
> > scopedptr.
> 
> Because it's really visually ugly and not clear at a glance.

That argument applies anywhere we take a scoped_ptr now. To my knowledge we are
not adding explicit 'clear' methods every where.

> It was a common pattern in our code to do "set_border(NULL);" to clear a
border.
> That's not great semantically, but is still tolerable.
> 
> "SetBorder(scoped_ptr<views::Border>());" isn't immediately clear like NULL
is,
> and it's generally better to have function names document what they're doing
(in
> this case, we're almost always clearing a border set by our base class).

I'm still against ClearBorder. I don't like bloating the API.

[Ben Goodger (Google), 2014-01-23 22:26:10]

I am also against ClearBorder.

How about defining a static on views::Border that returns a scoped_ptr, so you
could then do set_border(views::Border::NullBorder());

-Ben

[Elliot Glaysher, 2014-01-24 01:50:38]

ptal

Moved to SetBorder(views::Border::NullBorder());

[sky, 2014-01-24 16:33:46]

LGTM

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

https://codereview.chromium.org/145033006/diff/240001/ui/views/border.h#newco...
ui/views/border.h:47: // Documents that you are not drawing a border.
Convenience for creating a scoped_ptr with no Border.

[Elliot Glaysher, 2014-01-24 22:49:49]

Committed patchset #8 manually as r246990 (presubmit successful).