[Mac] Refactor bookmark pulsing into BookmarkBubbleObserverCocoa.

[Mac] Refactor bookmark pulsing into BookmarkBubbleObserverCocoa.

This moves c/b/ui/views/bookmarks/bookmark_bubble_view_observer.h to
c/b/ui/bookmarks/bookmark_bubble_observer.h since there was nothing
Views-specific about it. We can now use it with the Cocoa
BookmarkBubbleController.

Benefits:
 - Consumers of the platform-independent code in chrome/browser/ui/bookmarks
   become more consistent.
 - Further decoupling of the bookmark bubble from the browser window
   (e.g. allows the browser window to not care whether it shows a Cocoa
   bubble or a Views bubble).

It also allows the pair of notifications (NSWindowWillCloseNotification
on the bubble window and kPulseBookmarkButtonNotification) to be
consolidated into an observer.

BookmarkBubbleObserverCocoa encapsulates locking toolbar visibility and
pulsing the currently edited bookmark during the bubble's lifetime.
This allows moving the logic out of BookmarkBubbleController.

Logic to pulse the bookmark now resides entirely in
BookmarkBarController and can be better tested.

This also removes kPulseBookmarkButtonNotification, which was only used
by BookmarkBubbleController to inform BookmarkBarController to pulse a
button in the bar.

BUG=524320
Committed: https://crrev.com/5c23020388bcf81ea3b69d70bdd879671519231b
Cr-Commit-Position: refs/heads/master@{#345716}

[jackhou1, 2015-08-24 08:32:19]

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

[jackhou1, 2015-08-24 08:32:20]

tapted, PTAL

[tapted, 2015-08-25 01:13:21]

I really like this - those notifications make for some very confusing codepaths.

The BUG= should probably point to something

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:27: @class
BookmarkContextMenuCocoaController;
is this used here?

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:294: BookmarkButton*
pulsingButton_;
(maybe move this to be a member of BookmarkModelObserverForCocoa)

But also I don't think it should be weak -- typically weak pointers to NSObject
are only chosen over scoped_nsobject if the latter would create a cycle.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:308: -
(BookmarkButton*)bookmarkButtonForNode:(const BookmarkNode*)node {
declare this in BookmarkBarController(Private)? - probably next to buttonForNode
(should probably have a comment, and the definition may need to move to keep the
order consistent)

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:310:
BookmarkButton* result = nil;
I think it's more chromeystyle to omit `result` -- instead: `while (node)` and
e.g. `return button;` Then `return nil;` at the end (still preceded by the
NOTREACHED() perhaps).

Although I guess then then it falls into `no else after return` as well, so the
`else if` would become regular `if`, and then they could lose their curlies too.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:323: } else if
([managedBookmarksButton_ bookmarkNode] == node) {
a thought: would the managedBookmarksButton_'s parent ever be
`bookmarkModel_->bookmark_bar_node()`, potentially making this clause get
skipped since the first `if` is true?

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:338: if
(pulsingButton_)
Here maybe just start with

pulsingBookmarkObserver_.reset();

the destructor of the Cocoa observer can be responsible for calling
stopPulsingBookmarkNode - OR, if pulsingBookmarkObserver_ holds the
scoped_nsobject to the |pulsingButton_| instead of BookmarkBarController it
might would make
stopPulsingBookmarkNode even simpler: just pulsingBookmarkObserver_.reset(); --
pulsingBookmarkObserver_::~destroy could just call [setIsContinuousPulsing:NO],
and the ObjC block below wouldn't be needed

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:60:
observer:(bookmarks::BookmarkBubbleObserver*)observer
nit: observer -> bubbleObserver (since there is also a ModelObserver in
|bookmarkObserver_|

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm:167:
bookmarkObserver_.reset(new BookmarkModelObserverForCocoa(model_, ^(BOOL) {
is the BOOL argument to the closure still needed?

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h:10: #include
"base/mac/scoped_nsobject.h"
nit: import

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h:14: namespace
bookmarks {
nit: probably shouldn't be in namespace bookmarks - that's not usually done for
UI-specific code (it's Ok to inherit from bookmarks:: though)

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1780:
observer:bookmarkBubbleObserver_.get()
It might be nicer for the BookmarkBubbleObserverCocoa to alloc/init the
BookmarkBubbleController instead and pass |this| as the observer.


(Also this pattern looks more like a delegate than an observer.. but I guess
views is already like this so maybe it doesn't matter. Otherwise, it could call
AddObserver on the bubblecontroller (and BookmarkBubbleController has an
observer list rather than a single observer pointer).

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1792:
bookmarkBubbleController_ = nil;
I think |bookmarkBubbleController_| can be removed from BrowserWindowController
completely now -- it's never read from, just set to nil AFAIK

[jackhou1, 2015-08-25 04:32:29]

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:27: @class
BookmarkContextMenuCocoaController;
On 2015/08/25 01:13:20, tapted wrote:
> is this used here?

Yup, I was just rearranging to fix the order.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:294: BookmarkButton*
pulsingButton_;
On 2015/08/25 01:13:20, tapted wrote:
> (maybe move this to be a member of BookmarkModelObserverForCocoa)
> 
> But also I don't think it should be weak -- typically weak pointers to
NSObject
> are only chosen over scoped_nsobject if the latter would create a cycle.

I don't think this fits well in BookmarkModelObserverForCocoa (not to be
confused with BookmarkBubbleObserverCocoa). Also it would expose pulsing logic
to something outside of BookmarkBarController, which I was hoping wouldn't be
necessary. At the moment BookmarkBarController just exposes two methods.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:308: -
(BookmarkButton*)bookmarkButtonForNode:(const BookmarkNode*)node {
On 2015/08/25 01:13:20, tapted wrote:
> declare this in BookmarkBarController(Private)? - probably next to
buttonForNode
> (should probably have a comment, and the definition may need to move to keep
the
> order consistent)

Added declaration. The order is pretty inconsistent in this file, so I think it
makes more sense to have it here above -startPulsingBookmarkNode.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:310:
BookmarkButton* result = nil;
On 2015/08/25 01:13:20, tapted wrote:
> I think it's more chromeystyle to omit `result` -- instead: `while (node)` and
> e.g. `return button;` Then `return nil;` at the end (still preceded by the
> NOTREACHED() perhaps).
> 
> Although I guess then then it falls into `no else after return` as well, so
the
> `else if` would become regular `if`, and then they could lose their curlies
too.

Done.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:323: } else if
([managedBookmarksButton_ bookmarkNode] == node) {
On 2015/08/25 01:13:20, tapted wrote:
> a thought: would the managedBookmarksButton_'s parent ever be
> `bookmarkModel_->bookmark_bar_node()`, potentially making this clause get
> skipped since the first `if` is true?

I don't think so, since it works with the otherBookmarksButton_. I'm not sure
how to test it with managed/supervised. In any case, changed the order so that
it should be covered. Normal/other/off-the-side still work.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:338: if
(pulsingButton_)
On 2015/08/25 01:13:20, tapted wrote:
> Here maybe just start with
> 
> pulsingBookmarkObserver_.reset();
> 
> the destructor of the Cocoa observer can be responsible for calling
> stopPulsingBookmarkNode - OR, if pulsingBookmarkObserver_ holds the
> scoped_nsobject to the |pulsingButton_| instead of BookmarkBarController it
> might would make
> stopPulsingBookmarkNode even simpler: just pulsingBookmarkObserver_.reset();
--
> pulsingBookmarkObserver_::~destroy could just call
[setIsContinuousPulsing:NO],
> and the ObjC block below wouldn't be needed

As per previous comment, I'd prefer keep |pulsingButton_| private in
BookmarkBarController.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:60:
observer:(bookmarks::BookmarkBubbleObserver*)observer
On 2015/08/25 01:13:20, tapted wrote:
> nit: observer -> bubbleObserver (since there is also a ModelObserver in
> |bookmarkObserver_|

Done.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.mm:167:
bookmarkObserver_.reset(new BookmarkModelObserverForCocoa(model_, ^(BOOL) {
On 2015/08/25 01:13:21, tapted wrote:
> is the BOOL argument to the closure still needed?

Nope, removed.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h:10: #include
"base/mac/scoped_nsobject.h"
On 2015/08/25 01:13:21, tapted wrote:
> nit: import

Done.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_observer_cocoa.h:14: namespace
bookmarks {
On 2015/08/25 01:13:21, tapted wrote:
> nit: probably shouldn't be in namespace bookmarks - that's not usually done
for
> UI-specific code (it's Ok to inherit from bookmarks:: though)

Done.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1780:
observer:bookmarkBubbleObserver_.get()
On 2015/08/25 01:13:21, tapted wrote:
> It might be nicer for the BookmarkBubbleObserverCocoa to alloc/init the
> BookmarkBubbleController instead and pass |this| as the observer.
>
>
> (Also this pattern looks more like a delegate than an observer.. but I guess
> views is already like this so maybe it doesn't matter. Otherwise, it could
call
> AddObserver on the bubblecontroller (and BookmarkBubbleController has an
> observer list rather than a single observer pointer).

Yeah, I went with this because it's the same as Views. Of your suggestions, I'd
prefer the AddObserver pattern, but I think it's simpler to leave this as is.
How strongly do you feel about it?

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1792:
bookmarkBubbleController_ = nil;
On 2015/08/25 01:13:21, tapted wrote:
> I think |bookmarkBubbleController_| can be removed from
BrowserWindowController
> completely now -- it's never read from, just set to nil AFAIK

It's used in browser_window_controller_private.mm.

[tapted, 2015-08-25 05:46:34]

lgtm (although there are some compile errors).

I think it's worth adding to the CL description a bit, after "We can now use it
with the Cocoa BookmarkBubbleController.". Something like:


Benefits:
 - Consumers of the platform-independent code in chrome/browser/ui/bookmarks
   become more consistent.
 - Further decoupling of the bookmark bubble from the browser window (e.g.
allows
   the browser window to not care whether it shows a Cocoa bubble or a Views
bubble).

It also allows the pair of notifications (NSWindowWillCloseNotification on
the bubble window and kPulseBookmarkButtonNotification), to be consolidated
into an observer.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.h:294: BookmarkButton*
pulsingButton_;
On 2015/08/25 04:32:29, jackhou1 wrote:
> On 2015/08/25 01:13:20, tapted wrote:
> > (maybe move this to be a member of BookmarkModelObserverForCocoa)
> > 
> > But also I don't think it should be weak -- typically weak pointers to
> NSObject
> > are only chosen over scoped_nsobject if the latter would create a cycle.
> 
> I don't think this fits well in BookmarkModelObserverForCocoa (not to be
> confused with BookmarkBubbleObserverCocoa). Also it would expose pulsing logic
> to something outside of BookmarkBarController, which I was hoping wouldn't be
> necessary. At the moment BookmarkBarController just exposes two methods.

Acknowledged.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:308: -
(BookmarkButton*)bookmarkButtonForNode:(const BookmarkNode*)node {
On 2015/08/25 04:32:29, jackhou1 wrote:
> On 2015/08/25 01:13:20, tapted wrote:
> > declare this in BookmarkBarController(Private)? - probably next to
> buttonForNode
> > (should probably have a comment, and the definition may need to move to keep
> the
> > order consistent)
> 
> Added declaration. The order is pretty inconsistent in this file, so I think
it
> makes more sense to have it here above -startPulsingBookmarkNode.

Acknowledged.

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1780:
observer:bookmarkBubbleObserver_.get()
On 2015/08/25 04:32:29, jackhou1 wrote:
> On 2015/08/25 01:13:21, tapted wrote:
> > It might be nicer for the BookmarkBubbleObserverCocoa to alloc/init the
> > BookmarkBubbleController instead and pass |this| as the observer.
> >
> >
> > (Also this pattern looks more like a delegate than an observer.. but I guess
> > views is already like this so maybe it doesn't matter. Otherwise, it could
> call
> > AddObserver on the bubblecontroller (and BookmarkBubbleController has an
> > observer list rather than a single observer pointer).
> 
> Yeah, I went with this because it's the same as Views. Of your suggestions,
I'd
> prefer the AddObserver pattern, but I think it's simpler to leave this as is.
> How strongly do you feel about it?

Not at all strongly - it's just an itch

https://codereview.chromium.org/1308293002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1792:
bookmarkBubbleController_ = nil;
On 2015/08/25 04:32:29, jackhou1 wrote:
> On 2015/08/25 01:13:21, tapted wrote:
> > I think |bookmarkBubbleController_| can be removed from
> BrowserWindowController
> > completely now -- it's never read from, just set to nil AFAIK
> 
> It's used in browser_window_controller_private.mm.

Ah.. immersive fullscreen :/.  If it's just the [bookmarkBubbleController_
ok:self]; call, I *think* that's redundant, but I'm not confident.

I think "regular" fullscreen closes a bubble by way of -[BaseBubbleController
parentWindowWillToggleFullScreen:].

"immersive" fullscreen should close a bubble since a new window is created,
which should cause the bubble to lose key status and dismiss itself. Maybe that
doesn't happen. Anyway - something to explore in a follow-up perhaps.

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2342: 
nit: remove added line

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h
(right):

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h:33: //
Callback called on a significant model change. |nodeWasDeleted| will
update comment

[jackhou1, 2015-08-25 07:01:10]

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm (right):

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bar_controller.mm:2342: 
On 2015/08/25 05:46:34, tapted wrote:
> nit: remove added line

Done.

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h
(right):

https://codereview.chromium.org/1308293002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_model_observer_for_cocoa.h:33: //
Callback called on a significant model change. |nodeWasDeleted| will
On 2015/08/25 05:46:34, tapted wrote:
> update comment

Done.

[jackhou1, 2015-08-25 22:40:52]

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

[jackhou1, 2015-08-25 22:40:53]

asvitkine, please review for OWNERS of c/b/ui/cocoa/bookmarks/

msw, please review for OWNERS of c/b/ui/views/

[msw, 2015-08-25 23:14:48]

c/b/ui/views/ lgtm

[Alexei Svitkine (slow), 2015-08-26 15:43:01]

lgtm

[jackhou1, 2015-08-26 22:15:40]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-08-26 22:15:40]

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/1308293002/#ps60001
(title: "Address comments. Update unit_tests.")

[commit-bot: I haz the power, 2015-08-26 22:16:29]

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

[commit-bot: I haz the power, 2015-08-26 23:14:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-26 23:15:01]

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