Add BubbleManager to manage bubbles and ChromeBubbleManager for events.

components/bubble/bubble_manager.h

+// 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.
+
+#ifndef COMPONENTS_BUBBLE_BUBBLE_MANAGER_H__

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

nit: remove extra trailing underscore here and below*2.

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

Done.

+#define COMPONENTS_BUBBLE_BUBBLE_MANAGER_H__
+
+#include "base/callback_forward.h"
+#include "base/memory/scoped_vector.h"
+#include "base/memory/weak_ptr.h"
+#include "components/bubble/bubble_controller.h"
+
+class BubbleDelegate;
+
+typedef base::WeakPtr<BubbleController> BubbleReference;
+
+// Showing bubbles should go through the BubbleManager.

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

nit: consider more prescriptive behavior like: "Use the BubbleManager class to
show, update, and close bubbles."

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

Done.

+//
+// This class assumes that we won't be showing a lot of bubbles simultaneously.
+// If this assumption ever stops being true, please file a bug against whatever

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

I already filed http://crbug.com/366937 a long time ago; perhaps direct readers
to that bug (and/or some meta bug about the bubble manager work), and optionally
rephrase this as a "TODO(hcarmona): Handle requests to show multiple bubbles
simultaneously or in succession." It might also be worth mentioning the latest
plans/thoughts on managing attempts to show bubbles and dialogs simultaneously
or in succession.

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

Done.

+// is bombarding the user with many bubbles at the same time.
+class BubbleManager {
+ public:
+  virtual ~BubbleManager();
+
+  BubbleReference ShowBubble(scoped_ptr<BubbleDelegate> bubble);

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

nit: explicitly include the scoped_ptr header too.

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

Done.

+  void CloseBubble(BubbleReference bubble);

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

q: should this class offer a HideBubble function that avoids closing it?

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

I don't think it should. It's bad enough that PermissionBubbles abuse the hiding
pattern to achieve some sort of tab-modality, but I really don't want to
encourage that pattern.

A bubble should either get a user interaction, or be dismissed. There really
isn't a good case to be made for "temporarily suspended"

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

I agree, I hope we can eliminate hiding altogether, but wondered about API
consistency (Users can Hide bubbles via the controller anyway).

+  bool ShouldUpdateBubble(BubbleReference bubble);

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

nit: ditto on explanatory comments, especially when it comes to the preferred
API for showing/updating/closing bubbles... what does it mean to update a
bubble?

+  void UpdateBubble(BubbleReference bubble);
+
+ protected:
+  enum CloseOption {
+    ALLOW_HIDE,
+    FORCE_CLOSE,
+  };
+
+  // This class should be subclassed to hook into all the appropriate events.

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

Should you just omit a default constructor if this is an abstract class?

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

This class is "complex", so it needs a constructor.

+  BubbleManager();
+
+  void MassUpdateBubbles(

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

nit: consider renaming this UpdateMatchingBubbles (to fit the
CloseMatchingBubbles pattern), UpdateBubblesMatchingContext,
[RunCallback]ForEachMatchingBubble, or similar.

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

Acknowledged.

+      void* context,
+      base::Callback<void(base::WeakPtr<BubbleController>)> callback);
+
+  // For mass updating of bubbles:

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

Did you mean for this comment to go with MassUpdateBubbles? The comment itself
isn't very enlightening.

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

Yes, but no longer necessary.

+  void CloseMatchingBubbles(void* context,
+                            CloseOption close_option,

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

nit: a simple |allow_hide| or |force_close| bool (plus an optional function
comment) might suffice here, insead of adding the CloseOption enum.

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

Using the enum makes it more clear what will happen when this function
is called without having to refer back to documentation.

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

Acknowledged.

+                            BubbleCloseReason reason);
+
+  // Override and call Show on the controller when it's convenient.
+  void ShowBubbleUI(base::WeakPtr<BubbleController> controller);
+  void UpdateBubbleUI(base::WeakPtr<BubbleController> controller);
+
+ private:
+  ScopedVector<BubbleController> controllers_;
+
+  DISALLOW_COPY_AND_ASSIGN(BubbleManager);
+};
+
+#endif  // COMPONENTS_BUBBLE_BUBBLE_MANAGER_H__