Cleanup BookmarkBubbleView, remove LocationBarBubbleDelegateView::GetDialogButtons()

Cleanup BookmarkBubbleView, remove LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187
Review-Url: https://codereview.chromium.org/2905243003
Cr-Commit-Position: refs/heads/master@{#475259}
Committed: https://chromium.googlesource.com/chromium/src/+/85c36f9e4420de87c3b5cff31d090f158a5252d0

[tapted, 2017-05-26 05:36:11]

Description was changed from

==========
cldormst


Such refactoring

BUG=
==========

to

==========
Cleanup BookmarkBubbleView

BUG=726187
==========

[tapted, 2017-05-26 05:36:20]

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

[commit-bot: I haz the power, 2017-05-26 05:37:21]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 06:44:44]

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

[commit-bot: I haz the power, 2017-05-26 06:45:15]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 06:46:30]

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

[commit-bot: I haz the power, 2017-05-26 06:47:10]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 06:53:54]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-26 07:12:43]

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

[commit-bot: I haz the power, 2017-05-26 07:12:44]

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

[tapted, 2017-05-26 08:59:21]

Description was changed from

==========
Cleanup BookmarkBubbleView

BUG=726187
==========

to

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:

Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.

Renames close_button_ to save_button_ so it's not confused with the
close [x].

Renames bookmark_details_view_ to bookmark_contents_view_ (there's only
one level of detail). Also don't use unique_ptr for a View that's also
owned by the View hierarchy.

Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.

Sorts methods in inheritance order.

Removes ScopedAllowIO from the browsertest and reduces boilerplate.



BUG=726187
==========

[tapted, 2017-05-26 09:01:36]

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

[commit-bot: I haz the power, 2017-05-26 09:01:47]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 09:04:51]

Description was changed from

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:

Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.

Renames close_button_ to save_button_ so it's not confused with the
close [x].

Renames bookmark_details_view_ to bookmark_contents_view_ (there's only
one level of detail). Also don't use unique_ptr for a View that's also
owned by the View hierarchy.

Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.

Sorts methods in inheritance order.

Removes ScopedAllowIO from the browsertest and reduces boilerplate.



BUG=726187
==========

to

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Sorts methods in inheritance order.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187
==========

[tapted, 2017-05-26 09:04:51]

tapted@chromium.org changed reviewers:
+ pkasting@chromium.org

[tapted, 2017-05-26 09:05:15]

Patchset #2 (id:40001) has been deleted

[tapted, 2017-05-26 09:12:08]

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

[commit-bot: I haz the power, 2017-05-26 09:12:17]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 09:13:45]

Patchset #2 (id:60001) has been deleted

[tapted, 2017-05-26 09:53:08]

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

[commit-bot: I haz the power, 2017-05-26 09:53:20]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-26 10:01:54]

Patchset #2 (id:80001) has been deleted

[tapted, 2017-05-26 10:02:15]

Hi Peter, please take a look

[commit-bot: I haz the power, 2017-05-26 10:41:00]

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

[commit-bot: I haz the power, 2017-05-26 10:41:00]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2017-05-26 16:29:18]

Is it possible to split the bookmark bubble view function reordering changes
into a precursor patch?  It's hard to verify that the (minor) non-reordering
changes you did to that file aren't problematic since the diffs are large.

BTW, if you're reordering these to "make them more private", note that recently
on chromium-dev we basically decided not to do that, and that the rule should be
"the access level of an override should match that of the parent".

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:43: // on
the UI thread, which would need ScopedAllow-IO).
Nit: No hyphen

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:70: if
("ios_promotion" == name) {
Nit: Prefer (name == "ios_promotion") (see
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
)

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:244: // DialogDelegate,
but it does not want DialogClientView to create it.
I'm not really sure what this TODO is saying we should do.  The Zoom bubble is
basically a toast.  The reset button is right in the middle of a row of other
controls.  It seems reasonable to return DIALOG_BUTTON_NONE here.

[tapted, 2017-05-27 00:16:05]

Patchset #3 (id:120001) has been deleted

[tapted, 2017-05-27 00:17:38]

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

[commit-bot: I haz the power, 2017-05-27 00:17:49]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-27 00:18:49]

On 2017/05/26 16:29:18, Peter Kasting wrote:
> Is it possible to split the bookmark bubble view function reordering changes
> into a precursor patch?  It's hard to verify that the (minor) non-reordering
> changes you did to that file aren't problematic since the diffs are large.

Done -> https://codereview.chromium.org/2904333002/

> BTW, if you're reordering these to "make them more private", note that
recently
> on chromium-dev we basically decided not to do that, and that the rule should
be
> "the access level of an override should match that of the parent".

I'm down with that - done (in the precursor)

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:43: // on
the UI thread, which would need ScopedAllow-IO).
On 2017/05/26 16:29:18, Peter Kasting wrote:
> Nit: No hyphen

ah, without a hyphen I get a presubmit error that ScopedAllowIO is banned ಠ_ಠ

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:70: if
("ios_promotion" == name) {
On 2017/05/26 16:29:18, Peter Kasting wrote:
> Nit: Prefer (name == "ios_promotion") (see
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
> )

Oh cool - I didn't know we had this in the guide (I'm not a fan of yoda
syntax..). Done.

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:244: // DialogDelegate,
but it does not want DialogClientView to create it.
On 2017/05/26 16:29:18, Peter Kasting wrote:
> I'm not really sure what this TODO is saying we should do.  The Zoom bubble is
> basically a toast.  The reset button is right in the middle of a row of other
> controls.  It seems reasonable to return DIALOG_BUTTON_NONE here.

Hm. This would be a consistency thing - exposing reset to the ui::DialogModel
allows it to interact with framework code. E.g. automatically mapping
accelerators or a11y roles, automated testing codepaths (e.g.
DialowFrameView->CancelWindow() flows), putting buttons on the TouchBar. But
'Reset' may be special in other ways that makes this tricky. I've removed the
comment and I'll investigate more.

[tapted, 2017-05-27 00:19:13]

Description was changed from

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Sorts methods in inheritance order.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187
==========

to

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187
==========

[commit-bot: I haz the power, 2017-05-27 00:20:07]

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

[commit-bot: I haz the power, 2017-05-27 00:20:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-05-27 00:24:07]

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

[commit-bot: I haz the power, 2017-05-27 00:24:29]

Dry run: CQ is trying da patch.

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

[Peter Kasting, 2017-05-27 00:28:45]

LGTM.

The things that look like ownership model changes in bookmark_bubble_view.cc
stand out -- I want to understand whether the old code was wrong, or I'm just
misunderstanding something.  (I omitted "the new code is wrong", because it
looks to me like it follows the standard views convention of letting
AddChildView() actually do an ownership transfer despite passing a raw pointer,
which *ugh*.)

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:43: // on
the UI thread, which would need ScopedAllow-IO).
On 2017/05/27 00:18:48, tapted wrote:
> On 2017/05/26 16:29:18, Peter Kasting wrote:
> > Nit: No hyphen
> 
> ah, without a hyphen I get a presubmit error that ScopedAllowIO is banned ಠ_ಠ

... /facepalm

I wonder if the presubmit can be made smarter (file a bug?).  For now, any
reasonable route is fine -- leave it triggering a presubmit warning, change the
comment to remove the parenthetical, whatever

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:244: // DialogDelegate,
but it does not want DialogClientView to create it.
On 2017/05/27 00:18:49, tapted wrote:
> On 2017/05/26 16:29:18, Peter Kasting wrote:
> > I'm not really sure what this TODO is saying we should do.  The Zoom bubble
is
> > basically a toast.  The reset button is right in the middle of a row of
other
> > controls.  It seems reasonable to return DIALOG_BUTTON_NONE here.
> 
> Hm. This would be a consistency thing - exposing reset to the ui::DialogModel
> allows it to interact with framework code. E.g. automatically mapping
> accelerators or a11y roles, automated testing codepaths (e.g.
> DialowFrameView->CancelWindow() flows), putting buttons on the TouchBar. But
> 'Reset' may be special in other ways that makes this tricky. I've removed the
> comment and I'll investigate more.

If we want this button to work like a cancel button, then it does seem like
maybe a good idea to do this.

Another argument is that this button is really a "set to 100%" button rather
than "reset"/"cancel", so it's closer to + and - ("set to some other value"),
and the answer to the cases above is "we want all the buttons to be handled the
same way regarding these issues" -- which would lead to the question of e.g.
"can other buttons easily be placed on the TouchBar".

Anyway, I trust you to figure it out :)

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:338:
AddChildView(bookmark_contents_view_);
Wow, did the old code double-free?  I don't see a set_owned_by_parent() call
anywhere.

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:460: // Hide the
contents, but don't delete. It's needed in the destructor.
Where?

[commit-bot: I haz the power, 2017-05-27 00:34:07]

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

[commit-bot: I haz the power, 2017-05-27 00:34:08]

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

[tapted, 2017-05-27 01:02:35]

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

[commit-bot: I haz the power, 2017-05-27 01:02:53]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-27 01:08:24]

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

[commit-bot: I haz the power, 2017-05-27 01:08:40]

Dry run: CQ is trying da patch.

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

[tapted, 2017-05-27 01:09:39]

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view_browsertest.cc:43: // on
the UI thread, which would need ScopedAllow-IO).
On 2017/05/27 00:28:45, Peter Kasting wrote:
> On 2017/05/27 00:18:48, tapted wrote:
> > On 2017/05/26 16:29:18, Peter Kasting wrote:
> > > Nit: No hyphen
> > 
> > ah, without a hyphen I get a presubmit error that ScopedAllowIO is banned
ಠ_ಠ
> 
> ... /facepalm
> 
> I wonder if the presubmit can be made smarter (file a bug?).  For now, any
> reasonable route is fine -- leave it triggering a presubmit warning, change
the
> comment to remove the parenthetical, whatever

Since it's a presubmit error instead of a warning, I think CQ will block on it.
The change that added the lines ( http://crrev.com/467438 ) was landed
NOPRESUBMIT=true.

Rephrased the comment to say it would violate base::ThreadRestrictions. I think
we could make the presubmit smarter, but there's a computation cost (and
presubmits already take too long..).

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:244: // DialogDelegate,
but it does not want DialogClientView to create it.
On 2017/05/27 00:28:45, Peter Kasting wrote:
> On 2017/05/27 00:18:49, tapted wrote:
> > On 2017/05/26 16:29:18, Peter Kasting wrote:
> > > I'm not really sure what this TODO is saying we should do.  The Zoom
bubble
> is
> > > basically a toast.  The reset button is right in the middle of a row of
> other
> > > controls.  It seems reasonable to return DIALOG_BUTTON_NONE here.
> > 
> > Hm. This would be a consistency thing - exposing reset to the
ui::DialogModel
> > allows it to interact with framework code. E.g. automatically mapping
> > accelerators or a11y roles, automated testing codepaths (e.g.
> > DialowFrameView->CancelWindow() flows), putting buttons on the TouchBar. But
> > 'Reset' may be special in other ways that makes this tricky. I've removed
the
> > comment and I'll investigate more.
> 
> If we want this button to work like a cancel button, then it does seem like
> maybe a good idea to do this.
> 
> Another argument is that this button is really a "set to 100%" button rather
> than "reset"/"cancel", so it's closer to + and - ("set to some other value"),
> and the answer to the cases above is "we want all the buttons to be handled
the
> same way regarding these issues" -- which would lead to the question of e.g.
> "can other buttons easily be placed on the TouchBar".
> 
> Anyway, I trust you to figure it out :)

Yeah.. I'll likely want to play with DialogModel some more - cancel/ok buttons
map neatly to DialogClientView::Cancel/AcceptWindow, but there isn't a neat
route to activate other buttons yet (short of walking the view hierarchy looking
for buttons.. which is probably not robust)

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:338:
AddChildView(bookmark_contents_view_);
On 2017/05/27 00:28:45, Peter Kasting wrote:
> Wow, did the old code double-free?  I don't see a set_owned_by_parent() call
> anywhere.

I don't think so, but it's really subtle. Since the unique_ptr was a member of a
DialogDelegateView, instead of being deleted by View::DoRemoveChildView(), it
gets deleted by DialogDelegateView::DeleteDelegate(). This means the unique_ptr
destructor called at the end of ~BookmarkBubbleView() triggers a separate
removal of the contents, and by the time unwinding gets to the parent class,
~View() destructor, the child view will be removed.

So this isn't to fix a bug, but just to use more conventional patterns for
manipulating Views.

(effectively, it was leaning on the set_owned_by_parent() call invoked by the
DialogDelegateView constructor)

https://codereview.chromium.org/2905243003/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:460: // Hide the
contents, but don't delete. It's needed in the destructor.
On 2017/05/27 00:28:45, Peter Kasting wrote:
> Where?

updated comment

  // Hide the contents, but don't delete. Its child views are accessed in the
  // destructor if there are edits to apply.

[commit-bot: I haz the power, 2017-05-27 01:56:38]

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

[commit-bot: I haz the power, 2017-05-27 01:56:39]

Dry run: This issue passed the CQ dry run.

[tapted, 2017-05-28 06:45:43]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-28 06:45:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2905243003/#ps200001
(title: "better comments")

[commit-bot: I haz the power, 2017-05-28 06:45:47]

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-05-28 06:47:56]

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

[commit-bot: I haz the power, 2017-05-28 06:47:58]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tapted, 2017-05-28 07:55:46]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-05-28 07:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2905243003/#ps220001
(title: "Rebase for r475249")

[commit-bot: I haz the power, 2017-05-28 07:55:57]

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-05-28 11:08:25]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495958146596480,
"parent_rev": "0068568e614b22de3ade3c2a7e6490d04ceb951c", "commit_rev":
"85c36f9e4420de87c3b5cff31d090f158a5252d0"}

[commit-bot: I haz the power, 2017-05-28 11:08:38]

Description was changed from

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187
==========

to

==========
Cleanup BookmarkBubbleView, remove
LocationBarBubbleDelegateView::GetDialogButtons().

We want BookmarkBubbleView to be consistent with other dialogs. E.g. it
should use GetDialogButtons(), but LocationBarBubbleDelegateView's
override gets in the way, remove it. Most LocationBarBubbleDelegateView
will not want it, but don't break them yet.

Then, cleanups for BookmarkBubbleView:
-  Renames GetTitle() to GetBookmarkName() so it's not confused with the
dialog title.
-  Renames close_button_ to save_button_ so it's not confused with the
close [x].
-  Renames bookmark_details_view_ to bookmark_contents_view_ (there's
only one level of detail). Also don't use unique_ptr for a View that's
also owned by the View hierarchy.
-  Removes some FRIEND_TEST_ALL_PREFIXES; use neater techniques.
-  Removes ScopedAllowIO from the browsertest and reduces boilerplate.

BUG=726187

Review-Url: https://codereview.chromium.org/2905243003
Cr-Commit-Position: refs/heads/master@{#475259}
Committed:
https://chromium.googlesource.com/chromium/src/+/85c36f9e4420de87c3b5cff31d09...
==========

[commit-bot: I haz the power, 2017-05-28 11:08:40]

Committed patchset #7 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/85c36f9e4420de87c3b5cff31d09...