[Mac] Enable MacViews bookmark bubble behind --enable-mac-views-dialogs.

[Mac] Enable MacViews bookmark bubble behind --enable-mac-views-dialogs.

This allows BrowserWindowCocoa to show the Views
BookmarkBubbleView.

Similar to WebsiteSettingsPopupView,
BookmarkBubbleView::ShowBubble() can now take an anchor rect
instead of an anchor View.

BUG=511051
Committed: https://crrev.com/ca98257e52229f425d007dfa9040de56a7ec12f8
Cr-Commit-Position: refs/heads/master@{#345758}

[jackhou1, 2015-08-21 06:44:53]

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

[jackhou1, 2015-08-21 06:44:53]

tapted, PTAL

Here's an alternative approach to https://codereview.chromium.org/1307533002/

Also see the prerequisite CL https://codereview.chromium.org/1302243002/

[tapted, 2015-08-21 07:19:18]

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
File chrome/browser/ui/bookmarks/bookmark_bubble_observer.h (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_bubble_observer.h:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
nit: probably worth updating the year since it's not a straight move

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_bubble_observer.h:12: class
BookmarkBubbleObserver {
nit: comment

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:196: class
ViewsBookmarkBubbleObserver : public bookmarks::BookmarkBubbleObserver {
This doesn't feel right in this file, but I haven't come up with something
better yet.

One possibility might be to have the startPulsingBookmarkNode calls being done
in *Cocoa's* BookmarkBubbleController also use this observer interface (i.e.
they do FOR_EACH_OBSERVER instead). Then this Observer interface isn't
views-specfiic, and there is only one caller of start/stop pulsing.

Whatever is calling lockBarVisibility on Cocoa should somehow get looped in too.

And, either way, I think this class is complex enough to move to it's own file,
even if that means exposing some methods publicly

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1825: return;
DCHECK(viewsBookmarkBubbleObserver_) ?

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:367:
chrome::ShowBookmarkEditorViews(native_window, profile,
leave this as BookmarkEditor::Show?

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:37: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
Comment about this? (shouldn't Cocoa have one of these too? - should it get
passed in?)

[jackhou1, 2015-08-24 07:56:37]

Patchset #2 (id:20001) has been deleted

[jackhou1, 2015-08-24 08:22:04]

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
File chrome/browser/ui/bookmarks/bookmark_bubble_observer.h (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_bubble_observer.h:1: // Copyright 2012 The
Chromium Authors. All rights reserved.
On 2015/08/21 07:19:18, tapted wrote:
> nit: probably worth updating the year since it's not a straight move

Done.

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/bookmarks...
chrome/browser/ui/bookmarks/bookmark_bubble_observer.h:12: class
BookmarkBubbleObserver {
On 2015/08/21 07:19:18, tapted wrote:
> nit: comment

Done.

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:196: class
ViewsBookmarkBubbleObserver : public bookmarks::BookmarkBubbleObserver {
On 2015/08/21 07:19:18, tapted wrote:
> This doesn't feel right in this file, but I haven't come up with something
> better yet.
> 
> One possibility might be to have the startPulsingBookmarkNode calls being done
> in *Cocoa's* BookmarkBubbleController also use this observer interface (i.e.
> they do FOR_EACH_OBSERVER instead). Then this Observer interface isn't
> views-specfiic, and there is only one caller of start/stop pulsing.
> 
> Whatever is calling lockBarVisibility on Cocoa should somehow get looped in
too.
> 
> And, either way, I think this class is complex enough to move to it's own
file,
> even if that means exposing some methods publicly

I decided to pull all the non-Views parts out to another CL:
https://codereview.chromium.org/1308293002/

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1825: return;
On 2015/08/21 07:19:18, tapted wrote:
> DCHECK(viewsBookmarkBubbleObserver_) ?

Done. Is this to ensure it's not closed immediately?.

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:367:
chrome::ShowBookmarkEditorViews(native_window, profile,
On 2015/08/21 07:19:18, tapted wrote:
> leave this as BookmarkEditor::Show?

Done.

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:37: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
On 2015/08/21 07:19:18, tapted wrote:
> Comment about this? (shouldn't Cocoa have one of these too? - should it get
> passed in?)

Done. 

This is the prompt at the bottom of the bookmark bubble saying "Sign in to sync
your bookmarks". I think Cocoa just doesn't have this.

[tapted, 2015-08-25 00:00:45]

Looks good! only concern is the BookmarkBubbleSignInDelegate thing we discussed
(i.e. whether there are now different versions on Mac or, if they're not
platform specific, whether that delegate is needed at all.

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:102: void
ShowBookmarkBubbleViewsAtPoint(const gfx::Point& anchor_point,
nit: add comment (e.g. I would clarify that this is shown when the bookmark star
in the omnibox is clicked or Bookmark this page... selected from a
menu/accelerator).

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:357: gfx::NativeWindow
native_window =
nit: native_window -> parent  (or native_parent) -- I think `parent` is more
descriptive -- doesn't matter that it's now a gfx::NativeWindow rather than a
Widget

[tapted, 2015-08-25 03:10:16]

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1825: return;
On 2015/08/24 08:22:04, jackhou1 wrote:
> On 2015/08/21 07:19:18, tapted wrote:
> > DCHECK(viewsBookmarkBubbleObserver_) ?
> 
> Done. Is this to ensure it's not closed immediately?.

oops - missed a question. I guess that's part of it... Although TBH I didn't
really notice that the observer is being set just above, so the DCHECK is maybe
unnecessary - up to you if you want to keep it.

The point of having it is just to "bracket" the early exit case. That is, the
DCHECK basically adds the assertion "viewsBookmarkBubbleObserver_ is always
non-null when this method returns" -- this is clear for the early exit, and the
DCHECK does something similar for the later exit.

[tapted, 2015-08-25 03:15:42]

(also cl subject: "dialgs")

[jackhou1, 2015-08-25 04:55:39]

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:37: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
On 2015/08/24 08:22:04, jackhou1 wrote:
> On 2015/08/21 07:19:18, tapted wrote:
> > Comment about this? (shouldn't Cocoa have one of these too? - should it get
> > passed in?)
> 
> Done. 
> 
> This is the prompt at the bottom of the bookmark bubble saying "Sign in to
sync
> your bookmarks". I think Cocoa just doesn't have this.

It turns out on Cocoa the sign-in prompt is managed by
BookmarkSyncPromoController, which the controller for that NSView. I don't think
it makes sense to make it use BookmarkBubbleSignInDelegate, there's not much to
gain.

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:102: void
ShowBookmarkBubbleViewsAtPoint(const gfx::Point& anchor_point,
On 2015/08/25 00:00:44, tapted wrote:
> nit: add comment (e.g. I would clarify that this is shown when the bookmark
star
> in the omnibox is clicked or Bookmark this page... selected from a
> menu/accelerator).

Done.

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right):

https://codereview.chromium.org/1308713002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:357: gfx::NativeWindow
native_window =
On 2015/08/25 00:00:45, tapted wrote:
> nit: native_window -> parent  (or native_parent) -- I think `parent` is more
> descriptive -- doesn't matter that it's now a gfx::NativeWindow rather than a
> Widget

Done.

[tapted, 2015-08-25 05:56:48]

lgtm

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/1/chrome/browser/ui/views/bro...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:37: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
On 2015/08/25 04:55:39, jackhou1 wrote:
> On 2015/08/24 08:22:04, jackhou1 wrote:
> > On 2015/08/21 07:19:18, tapted wrote:
> > > Comment about this? (shouldn't Cocoa have one of these too? - should it
get
> > > passed in?)
> > 
> > Done. 
> > 
> > This is the prompt at the bottom of the bookmark bubble saying "Sign in to
> sync
> > your bookmarks". I think Cocoa just doesn't have this.
> 
> It turns out on Cocoa the sign-in prompt is managed by
> BookmarkSyncPromoController, which the controller for that NSView. I don't
think
> it makes sense to make it use BookmarkBubbleSignInDelegate, there's not much
to
> gain.

Acknowledged. I guess it's the views code that's weird, so no changes are needed
here.

That is, it looks like BookmarkBubbleSignInDelegate is the only
BookmarkBubbleDelegate and only views code uses BookmarkBubbleDelegate even
though it's in chrome/browser/ui/bookmarks instead of
chrome/browser/ui/views/bookmarks

So BookmarkSyncPromoView doesn't really need a delegate -- it could just invoke
the signin stuff directly, except that would confuse the testing that's
happening in bookmark_sync_promo_view_unittest.cc.

Maybe the testing could remain as-is if the BookmarkBubbleSignInDelegate was
simply moved into BookmarkBubbleView -- doesn't seem necessary to have it tied
to BrowserView. But all that's orthogonal to this CL.

[jackhou1, 2015-08-25 23:43:43]

jackhou@chromium.org changed reviewers:
+ msw@chromium.org

[jackhou1, 2015-08-25 23:43:43]

msw, please review for OWNERS

(Assuming https://codereview.chromium.org/1308293002/ does not change
significantly).

[msw, 2015-08-26 00:00:47]

lgtm with nits and a q.

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:35: bool
already_bookmarked) {
optional nit: use inverted |newly_bookmarked| to match views (or flip others).

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:38: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
optional nit: inline in |delegate| decl or in ShowBubble with make_scoped_ptr.

https://codereview.chromium.org/1308713002/diff/60001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1308713002/diff/60001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1981:
'browser/ui/views/bookmarks/bookmark_sync_promo_view.cc',
Did you actually test this with the sync promo?

[jackhou1, 2015-08-26 00:44:40]

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/browser_dialogs_views_mac.cc (right):

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:35: bool
already_bookmarked) {
On 2015/08/26 00:00:47, msw wrote:
> optional nit: use inverted |newly_bookmarked| to match views (or flip others).

Flipped others. BrowserWindow::ShowBookmarkBubble has it as |already_bookmarked|
and this CL already touches browser_view.cc. Now the inversion is confined to
bookmark_bubble_view.

https://codereview.chromium.org/1308713002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/browser_dialogs_views_mac.cc:38: delegate.reset(new
BookmarkBubbleSignInDelegate(browser));
On 2015/08/26 00:00:47, msw wrote:
> optional nit: inline in |delegate| decl or in ShowBubble with make_scoped_ptr.

Done.

https://codereview.chromium.org/1308713002/diff/60001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/1308713002/diff/60001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:1981:
'browser/ui/views/bookmarks/bookmark_sync_promo_view.cc',
On 2015/08/26 00:00:47, msw wrote:
> Did you actually test this with the sync promo?

Yup, clicking on the "Sign in" link in a Views bookmark bubble opens the usual
sign in dialog.

[msw, 2015-08-26 01:52:41]

lgtm

[jackhou1, 2015-08-27 00:10:28]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-08-27 00:10:28]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1308713002/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2015-08-27 00:11:28]

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

[commit-bot: I haz the power, 2015-08-27 01:34:59]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-08-27 01:35:36]

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