aura: brightness and volume bubble.

chrome/browser/chromeos/setting_level_bubble.h

Index: chrome/browser/chromeos/setting_level_bubble.h
diff --git a/chrome/browser/chromeos/setting_level_bubble.h b/chrome/browser/chromeos/setting_level_bubble.h
index 6496dde84c82efd9bbfa3c422986c87b0355c434..501d3098116a8be5c303d3c89d15af07ae36b543 100644
--- a/chrome/browser/chromeos/setting_level_bubble.h
+++ b/chrome/browser/chromeos/setting_level_bubble.h
@@ -7,9 +7,10 @@
 #pragma once
 
 #include "base/basictypes.h"
+#include "base/gtest_prod_util.h"
 #include "base/time.h"
 #include "base/timer.h"
-#include "chrome/browser/ui/views/bubble/bubble.h"
+#include "views/bubble/bubble_delegate.h"
 
 class SkBitmap;
 
@@ -19,11 +20,28 @@ class SettingLevelBubbleView;
 
 // Singleton class controlling a bubble displaying a level-based setting like
 // volume or brightness.
-class SettingLevelBubble : public BubbleDelegate {
+class SettingLevelBubble : public views::BubbleDelegateView {
  public:
-  // Shows the bubble.  |percent| should be in the range [0.0, 100.0].
-  void ShowBubble(double percent, bool enabled);
-  void HideBubble();
+  // Create the bubble delegate and view.
+  // |percent| should be in the range [0.0, 100.0].
+  static views::Widget* CreateBubble(SkBitmap* increase_icon,
+                                     SkBitmap* decrease_icon,
+                                     SkBitmap* zero_icon,
+                                     double percent,
+                                     bool enabled);
+
+  // Show the setting level bubble for this |widget|.
+  static void ShowBubble(views::Widget* widget,

[Daniel Erat, 2011/10/24 19:53:10]

Sorry to keep this review dragging on. :-(

The pattern here seems pretty complicated to me now.  If I'm not
misunderstanding, SettingLevelBubble's static CreateBubble() creates a
SettingLevelBubble and returns a Widget that holds a pointer back to the SLB. 
SLB::ShowBubble() takes the Widget as a parameter, but there's also a non-static
UpdateWithoutShowingBubble() method, which means that a caller needs to reach
into the View to get its delegate and then cast it to an SLB.

Maybe some of this complexity is unavoidable, but I'd prefer that it at least
didn't leak outside of the SettingLevelBubble class.  In other words, perhaps
SLB could expose a simple interface like the following:

class SettingLevelBubble {
 public:
  SettingLevelBubble(increase_icon, decrease_icon, etc.);
  void ShowBubble(double percent, bool enabled);
  void UpdateWithoutShowingBubble(double percent, bool enabled);
  void HideBubble();

 private:
  views::Widget* widget_;
};

Within setting_level_bubble.cc, a separate delegate class that calls back into
the SLB could be defined if needed (I'm assuming that SLB can't itself be a
delegate with this design, since the delegate might be owned by the Widget and
get deleted when the Widget is closed -- bubble_delegate.h probably ought to
have a comment describing who owns the BubbleDelegateView* that gets passed to
CreateBubble()).

[alicet1, 2011/10/25 00:51:39]

sure, we can talk about it tomorrow. Just to mention, the caller code for
VolumeBubble and BrightnessBubble was untouched with this change.

here's how chrome/browser/chromeos/brightness_observer.cc calls it:

void BrightnessObserver::BrightnessChanged(int level, bool user_initiated) {
  if (user_initiated)
    BrightnessBubble::GetInstance()->ShowBubble(level, true);
  else
    BrightnessBubble::GetInstance()->UpdateWithoutShowingBubble(level, true);

  VolumeBubble::GetInstance()->HideBubble();
}

and here's how system_key_event_listener.cc calls the bubble:
void SystemKeyEventListener::ShowVolumeBubble() {
  AudioHandler* audio_handler = GetAudioHandler();
  if (audio_handler) {
    VolumeBubble::GetInstance()->ShowBubble(
        audio_handler->GetVolumePercent(),
        !audio_handler->IsMuted());
  }
  BrightnessBubble::GetInstance()->HideBubble();
}

Those are the only usages of SettingLevelBubble in the code base.

The view/bubble creates bubbles as a widget type, with controlling
functionalities in its delegate and the contents in children views of it. I
actually don't think there was much casting e.g. in VolumeBubble to make this
awkward to work with, I can also push as much into SettingLevelBubble if you
think that improves the code.

+                         double percent,
+                         bool enabled);
+
+  virtual ~SettingLevelBubble();
+
+  // BubbleDelegate overrides:
+  virtual gfx::Point GetAnchorPoint() const OVERRIDE;
+
+  // WidgetDelegate overrides:
+  virtual void WindowClosing() OVERRIDE;
 
   // Updates the bubble's current level without showing the bubble onscreen.
   // We _do_ still animate the level moving to |percent| in case the bubble is
@@ -42,19 +60,20 @@ class SettingLevelBubble : public BubbleDelegate {
   // to 30%, rather than from 25% up to 30%.
   void UpdateWithoutShowingBubble(double percent, bool enabled);
 
+  // Start the |hide_timer_| for this bubble.
+  void StartHideTimer();
+
  protected:
+  // BubbleDelegate overrides:
+  virtual void Init() OVERRIDE;
+
   SettingLevelBubble(SkBitmap* increase_icon,
                      SkBitmap* decrease_icon,
-                     SkBitmap* zero_icon);
-  virtual ~SettingLevelBubble();
+                     SkBitmap* zero_icon,
+                     const gfx::Rect& monitor_area);
 
  private:
-  // Overridden from BubbleDelegate.
-  virtual void BubbleClosing(Bubble* bubble, bool closed_by_escape) OVERRIDE;
-  virtual bool CloseOnEscape() OVERRIDE;
-  virtual bool FadeInOnShow() OVERRIDE;
-
-  // Callback for |hide_timer_|.  Closes the bubble.
+  // Callback for |hide_timer_|.  Starts fading out.
   void OnHideTimeout();
 
   // Callback for |animation_timer_|.  Updates the level displayed by the view,
@@ -89,9 +108,10 @@ class SettingLevelBubble : public BubbleDelegate {
   SkBitmap* increase_icon_;
   SkBitmap* decrease_icon_;
   SkBitmap* disabled_icon_;
+  SkBitmap* current_icon_;
 
-  // Currently shown bubble or NULL.
-  Bubble* bubble_;
+  // Whether the content view, when shown, should be set to |enabled|.
+  bool enabled_;
 
   // Contents view owned by Bubble.
   SettingLevelBubbleView* view_;
@@ -108,6 +128,15 @@ class SettingLevelBubble : public BubbleDelegate {
   // Is |animation_timer_| currently running?
   bool is_animating_;
 
+  // Monitor area associated with the top level widget.
+  gfx::Rect monitor_area_;
+
+  // Size of the child view.
+  gfx::Size view_size_;
+
+  FRIEND_TEST_ALL_PREFIXES(SettingLevelBubbleTest, CreateAndUpdate);
+  FRIEND_TEST_ALL_PREFIXES(SettingLevelBubbleTest, ShowBubble);
+
   DISALLOW_COPY_AND_ASSIGN(SettingLevelBubble);
 };