aura: brightness and volume bubble.

chrome/browser/chromeos/setting_level_bubble.h

 // Copyright (c) 2011 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 CHROME_BROWSER_CHROMEOS_SETTING_LEVEL_BUBBLE_H_
 #define CHROME_BROWSER_CHROMEOS_SETTING_LEVEL_BUBBLE_H_
 #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;
 
 namespace chromeos {
 
 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
   // still visible from a previous call to ShowBubble().
   //
   // This can be used when the setting has been changed automatically and we
   // want to make sure that it's animated from the correct position the next
   // time that the bubble is shown.  For example:
   //
   // 1. Brightness is at 50%.
   // 2. Power manager dims brightness to 25% automatically.
   // 3. User hits the "increase brightness" button, setting brightness to 30%.
   //
   // If we didn't update our internal state to 25% after 2), then the animation
   // displayed in response to 3) would show the bubble animating from 50% down
   // 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,
   // also stopping the animation if we've reached the target.
   void OnAnimationTimeout();
 
   // Animates towards |percent|.  Updates |target_percent_| and starts
   // |animation_timer_| if it's not already running.  If this is the first time
   // that the level is being set, we just update |view_| immediately and don't
   // animate.
@@ skipping 14 matching lines @@
   base::TimeTicks last_animation_update_time_;
 
   // Time at which |target_percent_| was last updated.
   base::TimeTicks last_target_update_time_;
 
   // Icons displayed in the bubble when increasing or decreasing the level or
   // when it's disabled.  Not owned by us.
   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_;
 
   // Timer to hide the bubble.
   base::OneShotTimer<SettingLevelBubble> hide_timer_;
 
   // Timer to animate the currently-shown percent.  We use a timer instead of
   // ui::Animation since our animations are frequently interrupted by additional
   // changes to the level, and ui::Animation doesn't provide much control over
   // in-progress animations, leading to mega-jank.
   base::RepeatingTimer<SettingLevelBubble> animation_timer_;
 
   // 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);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_SETTING_LEVEL_BUBBLE_H_