Add BubbleManager to manage bubbles and ChromeBubbleManager for events.

components/bubble/bubble_manager.cc

Index: components/bubble/bubble_manager.cc
diff --git a/components/bubble/bubble_manager.cc b/components/bubble/bubble_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0f1ec8697060297c77dfdecb4a827e483cb7da08
--- /dev/null
+++ b/components/bubble/bubble_manager.cc
@@ -0,0 +1,111 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/bubble/bubble_manager.h"
+
+#include <vector>
+
+#include "components/bubble/bubble_delegate.h"
+#include "content/public/browser/browser_thread.h"
+
+BubbleManager::~BubbleManager() {
+  // The delegate should NOT outlive the manager. When a delegate is destroyed

[msw, 2015/08/13 03:37:23]

Each delegate is owned by a controller, and there are multiple controllers owned
by this class. Firstly, you might want to update this comment to reflect that.
But more importantly, I don't see what mechanism enforces the destruction of
controllers before the destruction of the manager, nor do I see where the
destruction of a delegate implies/enforces the hiding/closing/destruction of the
bubble (via delegate or UI). Shouldn't this BubbleManager dtor close all bubbles
itself (either by looping and closing each, or via calling
|controllers_.clear()| and adding a Close() call in the BubbleController dtor,
or similar on BubbleDelegate and BubbleUI)?

[hcarmona, 2015/08/15 02:03:20]

Assumption is no longer here. The controller will close the bubble if it
hasn't been closed.

+  // it should hide itself. This means that when the bubble manager is being
+  // destroyed there should be no visible bubbles.
+  DCHECK_EQ(controllers_.size(), 0);
+}
+
+BubbleReference BubbleManager::ShowBubble(scoped_ptr<BubbleDelegate> bubble) {
+  DCHECK(bubble);
+  DCHECK(bubble->GetContext());
+
+  scoped_ptr<BubbleController> controller(new BubbleController(bubble.Pass()));
+
+  BubbleReference bubble_ref = controller->AsWeakPtr();
+  ShowBubbleUI(bubble_ref);
+
+  controllers_.push_back(controller.Pass());
+  return bubble_ref;

[msw, 2015/08/13 03:37:23]

By exposing a BubbleReference, don't you allow users to directly call public
BubbleController functions, like Show/Hide/Close? Wouldn't that potentially
interfere with your attempt to manage these bubbles? (eg.
BubbleManager::CloseBubble assumes that the bubble wasn't yet hidden or closed)

[hcarmona, 2015/08/13 20:50:35]

True, do we have an existing pattern that would allow us to give a
reference that's just a token so we can find the bubble with the ability
to invalidate later?

[msw, 2015/08/13 21:41:37]

Maybe remove the controller functions or make them private, only let users call
BubbleManager::CloseBubble, etc.

[hcarmona, 2015/08/15 02:03:20]

Added a TODO item, I'll look into this next week.

+}
+
+void BubbleManager::CloseBubble(BubbleReference bubble) {

[msw, 2015/08/13 03:37:23]

q: might bubblemanager users ever want to specify a BubbleCloseReason here? You
even offer that as part of CloseMatchingBubbles.

[hcarmona, 2015/08/15 02:03:20]

Now they can :D

+  for (auto iter = controllers_.begin(); iter != controllers_.end(); ++iter) {
+    if ((*iter) == bubble.get()) {

[msw, 2015/08/13 03:37:23]

nit: parens around *iter aren't really necessary.

[hcarmona, 2015/08/15 02:03:20]

Done.

+      BubbleController* controller = *iter;
+      controllers_.weak_erase(iter);
+
+      controller->Hide(BUBBLE_CLOSE_IGNORE);
+      controller->Close();

[msw, 2015/08/13 03:37:23]

nit: perhaps closing a bubble should first hide it internally? See the pattern
for [Native]Widgets.

[hcarmona, 2015/08/15 02:03:20]

No longer necessary.

+
+      // Dancing around this because some bubbles will chain another on close.

[msw, 2015/08/13 03:37:23]

I'm not quite sure what you're saying here. Does closing one bubble cause
another bubble to close sometimes? (when? maybe give an example?) Can you revise
this comment to explain more clearly what you mean by dancing and chaining? 

[hcarmona, 2015/08/13 20:50:35]

Permission bubbles will sometimes show another bubble on close which
interferes with iterating.

[msw, 2015/08/13 21:41:37]

Acknowledged; maybe rephrase the comment to make that clearer.

[hcarmona, 2015/08/15 02:03:20]

Done. Better comment.

+      delete controller;
+      return;
+    }
+  }
+
+  // Hidden/unmanaged bubbles should not be hidden: this could indicate a bug.

[msw, 2015/08/13 03:37:23]

nit: replace the colon with a semicolon or a comma.

[hcarmona, 2015/08/15 02:03:20]

Acknowledged.

+  NOTREACHED();
+}
+
+bool BubbleManager::ShouldUpdateBubble(BubbleReference bubble) {
+  return bubble->ShouldUpdateBubble();
+}
+
+void BubbleManager::UpdateBubble(BubbleReference bubble) {
+  if (bubble->ShouldUpdateBubble()) {
+    bubble->Hide(BUBBLE_CLOSE_IGNORE);
+    ShowBubbleUI(bubble);
+  }
+}
+
+BubbleManager::BubbleManager() {}
+
+void BubbleManager::MassUpdateBubbles(
+    void* context,
+    base::Callback<void(base::WeakPtr<BubbleController>)> callback) {
+  DCHECK(context);
+
+  for (auto controller : controllers_) {
+    if (controller->MatchesContext(context))
+      callback.Run(controller->AsWeakPtr());
+  }
+}
+
+void BubbleManager::CloseMatchingBubbles(void* context,
+                                         CloseOption close_option,
+                                         BubbleCloseReason reason) {
+  DCHECK(context);
+
+  std::vector<BubbleController*> closing;
+
+  for (auto iter = controllers_.begin(); iter != controllers_.end();) {
+    if ((*iter)->MatchesContext(context)) {
+      (*iter)->Hide(reason);
+      if ((close_option == FORCE_CLOSE) || (*iter)->ShouldClose()) {
+        // Don't call close here to avoid bubbles trying to show the next in a

[msw, 2015/08/13 03:37:23]

Ugh, horrifying... This is why BubbleManager should evolve to explicitly handle
requests to show multiple bubbles simultaneously or in succession (ie.
ShowBubble queues the bubble if another is being shown, or potentially overrides
a less important bubble). You're attacking permission bubbles in this same cl...
is adding some queueing functionality now reasonable? (perhaps it would be...
you'd need to handle changes of context, like tab switching, that should
invalidate or delay bubbles queued for that context, etc.)

[groby-ooo-7-16, 2015/08/13 18:44:38]

The problem is that bubbles will be shown _in response_ to a Close() call. Which
means queueing alone is not enough. (I suppose we could make all calls to Show()
into a PostTask, which would fix this immediate issue - but I'm not sure it
won't be asking for more pain later.)

[msw, 2015/08/13 19:18:58]

Okay, this pattern (weak_erase first then Close later) isn't such a big deal,
but permission bubbles really are awful.

+        // series and messing up iterating the controllers.
+        // I'm looking at you, permission bubbles!
+        closing.push_back(*iter);
+        iter = controllers_.weak_erase(iter);
+        continue;
+      }
+    }
+    ++iter;

[msw, 2015/08/13 03:37:23]

Shouldn't this also be called if a bubble matches the context, but fails the
inner check (ie. returns false for ShouldClose and close_option==ALLOW_HIDE),
otherwise this will loop on that iter infinitely...

[hcarmona, 2015/08/13 20:50:35]

Yes, it's outside both checks.

[msw, 2015/08/13 21:41:37]

Oh duh, you're right, sorry.

+  }
+
+  for (auto iter : closing) {
+    iter->Close();
+    // No need to remove from the vector.
+    delete iter;

[msw, 2015/08/13 03:37:22]

ditto: dtor should call close or vice versa.

[hcarmona, 2015/08/15 02:03:20]

Acknowledged.

+  }
+}
+
+void BubbleManager::ShowBubbleUI(base::WeakPtr<BubbleController> controller) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[msw, 2015/08/13 03:37:23]

As I noted in the DEPS change, this seems like a job for ChromeBubbleManager.

[hcarmona, 2015/08/15 02:03:20]

Done.

+  controller->Show();
+}
+
+void BubbleManager::UpdateBubbleUI(base::WeakPtr<BubbleController> controller) {
+  controller->UpdateAnchorPosition();

[msw, 2015/08/13 03:37:23]

nit: unify naming... the layers of controllers and delegation is bad enough
without slightly distinct names to make parsing this code more difficult.

[hcarmona, 2015/08/15 02:03:20]

Done.

+}