Limit overlap between BubbleManager and BaseBubbleController.

Limit overlap between BubbleManager and BaseBubbleController.

The BubbleManager will be responsible for notifying bubbles of events
that can cause a bubble to close. The BaseBubbleController also notifies
the UI. This CL removes duplicate notifications on cocoa.

BUG=496955
Committed: https://crrev.com/f85b918c99d6cf6d3e0464106f7722660460f113
Cr-Commit-Position: refs/heads/master@{#348231}

[hcarmona, 2015-09-04 18:55:58]

hcarmona@chromium.org changed reviewers:
+ groby@chromium.org

[hcarmona, 2015-09-04 18:55:58]

PTAL

[groby-ooo-7-16, 2015-09-04 19:13:38]

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:37: -
(void)closeBubbleWithoutReason;
Just call it closeBubble - it's not like we're actually closing without having a
reason for it :)

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:270: -
(void)closeBubbleWithoutReason {
That's rather... icky. This means that we won't close at all if we have a
bubbleReference. Not sure this makes sense to me?

[hcarmona, 2015-09-04 20:58:02]

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:270: -
(void)closeBubbleWithoutReason {
On 2015/09/04 19:13:38, groby wrote:
> That's rather... icky. This means that we won't close at all if we have a
> bubbleReference. Not sure this makes sense to me?

This should only be called where it's redundant for bubbles managed by
the bubble manager. I _could_ close the bubbles if we have a bubble
reference too, but then we'd be doing it in two places. Shouldn't be the
end of the world because we'd be removing it from the manager, but we'd
have duplicate BubbleClose for the same reason.

[groby-ooo-7-16, 2015-09-04 21:38:03]

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:270: -
(void)closeBubbleWithoutReason {
On 2015/09/04 20:58:01, Hector Carmona wrote:
> On 2015/09/04 19:13:38, groby wrote:
> > That's rather... icky. This means that we won't close at all if we have a
> > bubbleReference. Not sure this makes sense to me?
> 
> This should only be called where it's redundant for bubbles managed by
> the bubble manager. I _could_ close the bubbles if we have a bubble
> reference too, but then we'd be doing it in two places. Shouldn't be the
> end of the world because we'd be removing it from the manager, but we'd
> have duplicate BubbleClose for the same reason.

Can we kill that duplication?

[hcarmona, 2015-09-04 21:49:39]

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:270: -
(void)closeBubbleWithoutReason {
On 2015/09/04 21:38:03, groby wrote:
> On 2015/09/04 20:58:01, Hector Carmona wrote:
> > On 2015/09/04 19:13:38, groby wrote:
> > > That's rather... icky. This means that we won't close at all if we have a
> > > bubbleReference. Not sure this makes sense to me?
> > 
> > This should only be called where it's redundant for bubbles managed by
> > the bubble manager. I _could_ close the bubbles if we have a bubble
> > reference too, but then we'd be doing it in two places. Shouldn't be the
> > end of the world because we'd be removing it from the manager, but we'd
> > have duplicate BubbleClose for the same reason.
> 
> Can we kill that duplication?

That's what this is attempting to do by not calling |CloseBubble| on the
bubble reference. The BubbleManager will notify the bubble to close when
the parent closes, when fullscreen is toggled, and when a tab changes.
Those functions should go away from this class when all bubbles are
using the bubble API.

[hcarmona, 2015-09-09 23:33:01]

hcarmona@chromium.org changed reviewers:
+ rsesek@chromium.org

[hcarmona, 2015-09-09 23:33:02]

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
File chrome/browser/ui/cocoa/base_bubble_controller.mm (right):

https://codereview.chromium.org/1328873002/diff/1/chrome/browser/ui/cocoa/bas...
chrome/browser/ui/cocoa/base_bubble_controller.mm:37: -
(void)closeBubbleWithoutReason;
On 2015/09/04 19:13:38, groby wrote:
> Just call it closeBubble - it's not like we're actually closing without having
a
> reason for it :)

Done.

[Robert Sesek, 2015-09-10 19:02:07]

rs lgtm

[hcarmona, 2015-09-10 20:56:47]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2015-09-10 20:57:16]

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

[commit-bot: I haz the power, 2015-09-10 21:05:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-09-10 21:06:08]

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

[commit-bot: I haz the power, 2015-09-23 12:14:06]

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