Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8319008:
aura: brightness and volume bubble. (Closed)
|
Created:
9 years, 2 months ago by alicet1 Modified:
9 years, 1 month ago CC:
chromium-reviews, stevenjb, nkostylev+watch_chromium.org, derat+watch_chromium.org, tfarina, davemoore+watch_chromium.org, dhollowa Visibility:
Public. |
Descriptionaura: brightness and volume bubble.
BUG=98322
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107821
Patch Set 1 #
Total comments: 4
Patch Set 2 : update. #Patch Set 3 : minor cleanup. #
Total comments: 18
Patch Set 4 : updates #
Total comments: 40
Patch Set 5 : updated #Patch Set 6 : update #
Total comments: 34
Patch Set 7 : udpate #Patch Set 8 : update #Patch Set 9 : update the icon #
Total comments: 18
Patch Set 10 : update, and added tests. #
Total comments: 8
Patch Set 11 : update #
Total comments: 2
Patch Set 12 : update #
Total comments: 2
Patch Set 13 : reworked. #Patch Set 14 : styles #
Total comments: 10
Patch Set 15 : add reset fade. #Patch Set 16 : update init and tests. #Patch Set 17 : styles #
Total comments: 2
Patch Set 18 : update comments #
Total comments: 20
Patch Set 19 : style #Patch Set 20 : dcheck #Patch Set 21 : update. #
Total comments: 2
Patch Set 22 : update #Patch Set 23 : update #Messages
Total messages: 44 (0 generated)
hi mike, this is using the new bubble changes you have that is still under review -- I'm using this to test out what we are missing in the api. The only extra I need from bubble_delegate/bubble_view is to optionalize the use of accelerators, since these bubbles go by timers instead. thanx, alice
> optionalize the use of accelerators Okay, I'll work on that and fading in the bubble now.
http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/brightn... File chrome/browser/chromeos/brightness_bubble_view_views.cc (right): http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/brightn... chrome/browser/chromeos/brightness_bubble_view_views.cc:21: ResourceBundle::GetSharedInstance().GetBitmapNamed( nit: indent four spaces, not six http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/system_... File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/system_... chrome/browser/chromeos/system_key_event_listener.cc:24: #include "chrome/browser/chromeos/brightness_bubble_view_views.h" I'm finding this pretty confusing (probably in large part because of the naming -- the previous version of the brightness/volume/etc. bubble is already Views-specific, no?). I'd prefer that you just made the existing version of the code use your new bubble class (maybe behind an ifdef, if you're not confident in your class yet). Duplicating all of the code like this seems messy.
http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/brightn... File chrome/browser/chromeos/brightness_bubble_view_views.cc (right): http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/brightn... chrome/browser/chromeos/brightness_bubble_view_views.cc:21: ResourceBundle::GetSharedInstance().GetBitmapNamed( On 2011/10/18 17:55:12, Daniel Erat wrote: > nit: indent four spaces, not six Done. http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/system_... File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/8319008/diff/1/chrome/browser/chromeos/system_... chrome/browser/chromeos/system_key_event_listener.cc:24: #include "chrome/browser/chromeos/brightness_bubble_view_views.h" On 2011/10/18 17:55:12, Daniel Erat wrote: > I'm finding this pretty confusing (probably in large part because of the naming > -- the previous version of the brightness/volume/etc. bubble is already > Views-specific, no?). > > I'd prefer that you just made the existing version of the code use your new > bubble class (maybe behind an ifdef, if you're not confident in your class yet). > Duplicating all of the code like this seems messy. yes, removed, and replaced the original *_bubbles.
adding saintlou as an fyi for the comment in GetToplevelWidget of setting_level_bubble.cc thanx, alice
Hi Alice,
I was going to submit the following patch after you pointed out that TODO to me.
But feel free to roll it in this CL:
diff --git a/chrome/browser/chromeos/setting_level_bubble.cc
b/chrome/browser/chromeos/setting_level_bubble.cc
index bb8df23..2a083eb 100644
--- a/chrome/browser/chromeos/setting_level_bubble.cc
+++ b/chrome/browser/chromeos/setting_level_bubble.cc
@@ -57,11 +57,7 @@ namespace chromeos {
// TODO(glotov): remove this in favor of enabling Bubble class act
// without |parent| specified. crosbug.com/4025
static views::Widget* GetToplevelWidget() {
-#if defined(USE_AURA)
- // TODO(saintlou): Need to fix in PureViews.
- return WebUILoginDisplay::GetLoginWindow();
-#else
- GtkWindow* window = NULL;
+ gfx::NativeWindow window = NULL;
// We just use the default profile here -- this gets overridden as needed
// in Chrome OS depending on whether the user is logged in or not.
@@ -70,19 +66,23 @@ static views::Widget* GetToplevelWidget() {
ProfileManager::GetDefaultProfile(),
true); // match_incognito
if (browser) {
- window = GTK_WINDOW(browser->window()->GetNativeHandle());
- } else {
+ window = browser->window()->GetNativeHandle();
+ }
+#if defined(USE_AURA)
+ // TODO(saintlou): Unsure what to do for the Aura background.
+#else
+ else {
// Otherwise, see if there's a background window that we can use.
BackgroundView* background = LoginUtils::Get()->GetBackgroundView();
if (background)
window = GTK_WINDOW(background->GetNativeWindow());
}
+#endif
if (window)
return views::Widget::GetWidgetForNativeWindow(window);
else
return WebUILoginDisplay::GetLoginWindow();
-#endif
}
SettingLevelBubble::SettingLevelBubble(SkBitmap* increase_icon,
hi saintlou,
please submit the patch first, I will update this issue when you're done.
thanx,
alice
On 2011/10/19 16:47:38, Emmanuel Saint-loubert wrote:
> Hi Alice,
>
> I was going to submit the following patch after you pointed out that TODO to
me.
> But feel free to roll it in this CL:
>
>
> diff --git a/chrome/browser/chromeos/setting_level_bubble.cc
> b/chrome/browser/chromeos/setting_level_bubble.cc
> index bb8df23..2a083eb 100644
> --- a/chrome/browser/chromeos/setting_level_bubble.cc
> +++ b/chrome/browser/chromeos/setting_level_bubble.cc
> @@ -57,11 +57,7 @@ namespace chromeos {
> // TODO(glotov): remove this in favor of enabling Bubble class act
> // without |parent| specified. crosbug.com/4025
> static views::Widget* GetToplevelWidget() {
> -#if defined(USE_AURA)
> - // TODO(saintlou): Need to fix in PureViews.
> - return WebUILoginDisplay::GetLoginWindow();
> -#else
> - GtkWindow* window = NULL;
> + gfx::NativeWindow window = NULL;
>
> // We just use the default profile here -- this gets overridden as needed
> // in Chrome OS depending on whether the user is logged in or not.
> @@ -70,19 +66,23 @@ static views::Widget* GetToplevelWidget() {
> ProfileManager::GetDefaultProfile(),
> true); // match_incognito
> if (browser) {
> - window = GTK_WINDOW(browser->window()->GetNativeHandle());
> - } else {
> + window = browser->window()->GetNativeHandle();
> + }
> +#if defined(USE_AURA)
> + // TODO(saintlou): Unsure what to do for the Aura background.
> +#else
> + else {
> // Otherwise, see if there's a background window that we can use.
> BackgroundView* background = LoginUtils::Get()->GetBackgroundView();
> if (background)
> window = GTK_WINDOW(background->GetNativeWindow());
> }
> +#endif
>
> if (window)
> return views::Widget::GetWidgetForNativeWindow(window);
> else
> return WebUILoginDisplay::GetLoginWindow();
> -#endif
> }
>
> SettingLevelBubble::SettingLevelBubble(SkBitmap* increase_icon,
http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.cc:31: ->UpdateSettingLevelInternal(percent, enabled); nit: it seems strange to be calling a method named "Internal" from outside of the class http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:31: // Bubble widget, owned by VolumeBubble. is this comment incorrect? http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:32: static views::Widget* widget_; nit: add a blank line after this http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:32: static views::Widget* widget_; could this be non-static? http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.cc:64: // Is this the same as using parent->GetRootView()? yeah, i think that the root view is what we want here. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:29: static views::Widget* ConstructSettingLevelBubble( naming nit: s/Construct/Create/ (which i think is what's usually used in chrome) http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:58: // Start the hide_timer_ for this bubble. nit: |hide_timer_| http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:115: bool enabled_; nit: leave a blank line between this and the next member http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:131: gfx::Point anchor_point_; nit: leave a blank line after this
http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.cc:31: ->UpdateSettingLevelInternal(percent, enabled); On 2011/10/19 17:28:09, Daniel Erat wrote: > nit: it seems strange to be calling a method named "Internal" from outside of > the class Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:31: // Bubble widget, owned by VolumeBubble. On 2011/10/19 17:28:09, Daniel Erat wrote: > is this comment incorrect? updated. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:32: static views::Widget* widget_; On 2011/10/19 17:28:09, Daniel Erat wrote: > nit: add a blank line after this Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/brig... chrome/browser/chromeos/brightness_bubble.h:32: static views::Widget* widget_; On 2011/10/19 17:28:09, Daniel Erat wrote: > could this be non-static? Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.cc:64: // Is this the same as using parent->GetRootView()? On 2011/10/19 17:28:09, Daniel Erat wrote: > yeah, i think that the root view is what we want here. saintlou uploaded a fix to this code. let me know if there is anything else for this method we need for aura. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:29: static views::Widget* ConstructSettingLevelBubble( On 2011/10/19 17:28:09, Daniel Erat wrote: > naming nit: s/Construct/Create/ (which i think is what's usually used in chrome) Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:58: // Start the hide_timer_ for this bubble. On 2011/10/19 17:28:09, Daniel Erat wrote: > nit: |hide_timer_| Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:115: bool enabled_; On 2011/10/19 17:28:09, Daniel Erat wrote: > nit: leave a blank line between this and the next member Done. http://codereview.chromium.org/8319008/diff/4002/chrome/browser/chromeos/sett... chrome/browser/chromeos/setting_level_bubble.h:131: gfx::Point anchor_point_; On 2011/10/19 17:28:09, Daniel Erat wrote: > nit: leave a blank line after this Done.
http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:19: delete widget_; nit: deleting a null pointer is fine, so you can remove the if statement. also suggest setting widget_ to NULL afterwards http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:38: widget_->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); nit: comment style i've seen more frequently for parameters in chrome is StartFade(false /* fade_in */); http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:38: widget_->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); does this mean that we start fading the bubble out immediately? if so, i don't think that that's desired -- we should show it at full opacity for a bit and then fade it out quickly http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:23: // Returns a the singleton brightness bubble instance. nit: delete "a" http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:30: // Update the bubble |level| and |enabled| state. See tiny nit: s/Update/Updates/; add trailing period to second sentence http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:34: static_cast<SettingLevelBubble*>(widget_->widget_delegate()) i think that there's a decent amount of duplicated code between BrightnessBubble and VolumeBubble now. would it be feasible to push it into SettingLevelBubble?
Some feedback, there's a lot of opportunity for cleanup here. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:10: #include "views/bubble/bubble_view.h" merge and remove bubble_view.h http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:49: void BrightnessBubble::HideBubble() { Is this called anywhere? http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:15: class Widget; forward decl shouldn't be needed. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:19: #include "views/bubble/bubble_view.h" Merge and remove bubble_view.h soon. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:103: widget->client_view()->AsBubbleView()->set_close_on_esc(false); Merge for BubbleDelegateView::set_close_on_esc http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:121: WindowClosing(); This shouldn't be necessary. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:136: CalculateAnchorPoint(); I wonder if you can calculate the anchor point earlier and pass it into the BubbleDelegateView ctor, but maybe that's too tricky if the SettingLevelBubbleView needs to be initialized first. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:145: bool enabled) { This fits on one line. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:17: class Widget; Forward decl probably not necessary with bubble_delegate.h include. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:64: virtual views::BubbleBorder::ArrowLocation GetArrowLocation() const OVERRIDE; GetAnchorPoint and GetArrowLocation overrides shouldn't be neceassary after merging. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:43: void VolumeBubble::HideBubble() { Is this used at all? http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:15: class Widget; forward decl shouldn't be needed. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:23: // Returns a the singleton volume bubble instance. ditto here on dan's comment elsewhere; remove "a" http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:30: // hides the bubble. capitalize 'hides'
http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:10: #include "views/bubble/bubble_view.h" On 2011/10/20 19:36:13, msw wrote: > merge and remove bubble_view.h Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:19: delete widget_; On 2011/10/20 15:43:39, Daniel Erat wrote: > nit: deleting a null pointer is fine, so you can remove the if statement. also > suggest setting widget_ to NULL afterwards Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:38: widget_->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); On 2011/10/20 15:43:39, Daniel Erat wrote: > nit: comment style i've seen more frequently for parameters in chrome is > > StartFade(false /* fade_in */); Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:38: widget_->client_view()->AsBubbleView()->StartFade(/*fade_in=*/false); On 2011/10/20 15:43:39, Daniel Erat wrote: > does this mean that we start fading the bubble out immediately? if so, i don't > think that that's desired -- we should show it at full opacity for a bit and > then fade it out quickly yup, changed. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:49: void BrightnessBubble::HideBubble() { On 2011/10/20 19:36:13, msw wrote: > Is this called anywhere? yeah, in SystemKeyEventListener::ShowVolumeBubble, we end with a hide on brightness bubble there. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:15: class Widget; On 2011/10/20 19:36:13, msw wrote: > forward decl shouldn't be needed. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:23: // Returns a the singleton brightness bubble instance. On 2011/10/20 15:43:39, Daniel Erat wrote: > nit: delete "a" Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:30: // Update the bubble |level| and |enabled| state. See On 2011/10/20 15:43:39, Daniel Erat wrote: > tiny nit: s/Update/Updates/; add trailing period to second sentence Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:19: #include "views/bubble/bubble_view.h" On 2011/10/20 19:36:13, msw wrote: > Merge and remove bubble_view.h soon. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:103: widget->client_view()->AsBubbleView()->set_close_on_esc(false); On 2011/10/20 19:36:13, msw wrote: > Merge for BubbleDelegateView::set_close_on_esc Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:121: WindowClosing(); On 2011/10/20 19:36:13, msw wrote: > This shouldn't be necessary. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:136: CalculateAnchorPoint(); On 2011/10/20 19:36:13, msw wrote: > I wonder if you can calculate the anchor point earlier and pass it into the > BubbleDelegateView ctor, but maybe that's too tricky if the > SettingLevelBubbleView needs to be initialized first. yeah, I need the view first. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:145: bool enabled) { On 2011/10/20 19:36:13, msw wrote: > This fits on one line. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:17: class Widget; On 2011/10/20 19:36:13, msw wrote: > Forward decl probably not necessary with bubble_delegate.h include. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:64: virtual views::BubbleBorder::ArrowLocation GetArrowLocation() const OVERRIDE; On 2011/10/20 19:36:13, msw wrote: > GetAnchorPoint and GetArrowLocation overrides shouldn't be neceassary after > merging. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:34: static_cast<SettingLevelBubble*>(widget_->widget_delegate()) On 2011/10/20 15:43:39, Daniel Erat wrote: > i think that there's a decent amount of duplicated code between BrightnessBubble > and VolumeBubble now. would it be feasible to push it into SettingLevelBubble moved. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:43: void VolumeBubble::HideBubble() { On 2011/10/20 19:36:13, msw wrote: > Is this used at all? ya. in brightness_observer.cc BrightnessChanged. I think it's because at any given time we should only show a brightness bubble or a volume bubble. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:15: class Widget; On 2011/10/20 19:36:13, msw wrote: > forward decl shouldn't be needed. Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:23: // Returns a the singleton volume bubble instance. On 2011/10/20 19:36:13, msw wrote: > ditto here on dan's comment elsewhere; remove "a" Done. http://codereview.chromium.org/8319008/diff/10002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:30: // hides the bubble. On 2011/10/20 19:36:13, msw wrote: > capitalize 'hides' Done.
Looks fine to me after this last round of comments are addressed (assuming that you've played with it a bit in a regular non-Aura Chrome OS build and confirmed that it's still working correctly). http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:22: widget_= SettingLevelBubble::ShowBubble( nit: space between 'widget_' and '=' http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:99: // Construct and initialize settings. this comment seems unnecessary http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:122: // Show the window and start the timer. When The timer nit: s/The/the/ http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:159: nit: remove extra blank line http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:207: StartFade(false/* fade_in */); nit: add one space between 'false' and comment http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:26: static views::Widget* CreateSettingLevelBubble( nit: CreateBubble()? http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:37: static views::Widget* ShowBubble(views::Widget* widget, the semantics of this method seem strange. i'd recommend just taking a non-null widget along with the percent and enabled arguments. it seems fine to require the caller to cal either the Create() method or ShowBubble() as needed. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:67: // BubbleDelegate overrides: i think that other chrome code that i've seen usually puts overridden methods above non-overridden methods (not sure if they should come before or after the static method, though) http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:71: // WidgetDeletgate overrides: WidgetDeletgate -> WidgetDelegate http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:22: widget_= SettingLevelBubble::ShowBubble( nit: space between 'widget_' and '='
http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:14: BrightnessBubble::BrightnessBubble() : widget_(NULL) {} Move the ctor and dtor below the public functions, to match their declaration order. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:26: // Update. the bubble |level| and |enabled| state. See remove period after update http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:28: void UpdateWithoutShowingBubble(int level, bool enabled); SettingLevelBubble::UpdateWithoutShowingBubble takes double |percent| as its first argument, can you retype and rename this function to be consistent? http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:33: virtual ~SettingLevelBubble(); The destructor probably belongs below these static functions. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:14: VolumeBubble::VolumeBubble() : widget_(NULL) {} Move the private ctor and dtor below the public functions to match their declaration order. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:19: // Returns a the singleton volume bubble instance. remove 'a' http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:33: views::Widget* widget_; data members belong below functions.
http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:14: BrightnessBubble::BrightnessBubble() : widget_(NULL) {} On 2011/10/21 02:42:06, msw wrote: > Move the ctor and dtor below the public functions, to match their declaration > order. Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:22: widget_= SettingLevelBubble::ShowBubble( On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: space between 'widget_' and '=' Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:26: // Update. the bubble |level| and |enabled| state. See On 2011/10/21 02:42:06, msw wrote: > remove period after update Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:28: void UpdateWithoutShowingBubble(int level, bool enabled); On 2011/10/21 02:42:06, msw wrote: > SettingLevelBubble::UpdateWithoutShowingBubble takes double |percent| as its > first argument, can you retype and rename this function to be consistent? Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:99: // Construct and initialize settings. On 2011/10/21 00:25:21, Daniel Erat wrote: > this comment seems unnecessary Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:122: // Show the window and start the timer. When The timer On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: s/The/the/ Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:159: On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: remove extra blank line Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:207: StartFade(false/* fade_in */); On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: add one space between 'false' and comment Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:26: static views::Widget* CreateSettingLevelBubble( On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: CreateBubble()? Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:33: virtual ~SettingLevelBubble(); On 2011/10/21 02:42:06, msw wrote: > The destructor probably belongs below these static functions. Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:37: static views::Widget* ShowBubble(views::Widget* widget, On 2011/10/21 00:25:21, Daniel Erat wrote: > the semantics of this method seem strange. i'd recommend just taking a non-null > widget along with the percent and enabled arguments. it seems fine to require > the caller to cal either the Create() method or ShowBubble() as needed. Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:67: // BubbleDelegate overrides: On 2011/10/21 00:25:21, Daniel Erat wrote: > i think that other chrome code that i've seen usually puts overridden methods > above non-overridden methods (not sure if they should come before or after the > static method, though) Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:71: // WidgetDeletgate overrides: On 2011/10/21 00:25:21, Daniel Erat wrote: > WidgetDeletgate -> WidgetDelegate Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:14: VolumeBubble::VolumeBubble() : widget_(NULL) {} On 2011/10/21 02:42:06, msw wrote: > Move the private ctor and dtor below the public functions to match their > declaration order. Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.cc:22: widget_= SettingLevelBubble::ShowBubble( On 2011/10/21 00:25:21, Daniel Erat wrote: > nit: space between 'widget_' and '=' Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:19: // Returns a the singleton volume bubble instance. On 2011/10/21 02:42:06, msw wrote: > remove 'a' Done. http://codereview.chromium.org/8319008/diff/15002/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble.h:33: views::Widget* widget_; On 2011/10/21 02:42:06, msw wrote: > data members belong below functions. Done.
http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:26: if (!widget_ || widget_closed_) { are you leaking widget_ in the closed case? if it's automatically deleted when it gets closed, you should set widget_ to NULL then http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:39: if (!widget_closed_) { how can widget_closed_ ever be true here?
http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:66: delete widget_; Should |widget_| be closed first? I actually don't know... http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:48: DISALLOW_COPY_AND_ASSIGN(BrightnessBubble); Insert a blank line above this macro. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:139: monitor_area_(monitor_area) { enabled_ should probably be initialized here. afaict it's set correctly in CreateBubble via UpdateWithoutShowingBubble before it's used in Init, but explicitly initializing it is a good idea. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:40: virtual void Init() OVERRIDE; BubbleDelegateView::Init is declared protected, this override should retain that visibility and ordering. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:110: bool enabled_; Comment? http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:127: // |monitor_area_| associated with the top level widget. I think plain text "Monitor area" makes more sense than |monitor_area_| in this comment. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:130: // size of the child view. capitalize 'size'
also ran valgrind on the new tests. thanx, alice http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:26: if (!widget_ || widget_closed_) { On 2011/10/21 18:24:34, Daniel Erat wrote: > are you leaking widget_ in the closed case? if it's automatically deleted when > it gets closed, you should set widget_ to NULL then the latter, this bubble shouldnt need to delete. removed the clause and added a test case to check in valgrind. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:39: if (!widget_closed_) { On 2011/10/21 18:24:34, Daniel Erat wrote: > how can widget_closed_ ever be true here? removed. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:66: delete widget_; On 2011/10/22 00:44:48, msw wrote: > Should |widget_| be closed first? I actually don't know... actually, just need to set widget_ to null. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:48: DISALLOW_COPY_AND_ASSIGN(BrightnessBubble); On 2011/10/22 00:44:48, msw wrote: > Insert a blank line above this macro. Done. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:139: monitor_area_(monitor_area) { On 2011/10/22 00:44:48, msw wrote: > enabled_ should probably be initialized here. > afaict it's set correctly in CreateBubble via UpdateWithoutShowingBubble before > it's used in Init, but explicitly initializing it is a good idea. Done. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:40: virtual void Init() OVERRIDE; On 2011/10/22 00:44:48, msw wrote: > BubbleDelegateView::Init is declared protected, this override should retain that > visibility and ordering. Done. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:110: bool enabled_; On 2011/10/22 00:44:48, msw wrote: > Comment? Done. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:127: // |monitor_area_| associated with the top level widget. On 2011/10/22 00:44:48, msw wrote: > I think plain text "Monitor area" makes more sense than |monitor_area_| in this > comment. Done. http://codereview.chromium.org/8319008/diff/28007/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:130: // size of the child view. On 2011/10/22 00:44:48, msw wrote: > capitalize 'size' Done.
http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:22: widget_closed_ = true; i don't understand why you need a widget_closed_ member. wouldn't it be equivalent and simpler to just set widget_ to NULL here? http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:30: // Update the bubble |percent| and |enabled| state. See nit: s/Update/Updates/ http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:43: // Check that widget has been created or is not closed. nit: Checks http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:44: // Create widget if needed. nit: Creates
http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.cc (right): http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.cc:22: widget_closed_ = true; On 2011/10/24 15:52:10, Daniel Erat wrote: > i don't understand why you need a widget_closed_ member. wouldn't it be > equivalent and simpler to just set widget_ to NULL here? actually, yes! http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble.h (right): http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:30: // Update the bubble |percent| and |enabled| state. See On 2011/10/24 15:52:10, Daniel Erat wrote: > nit: s/Update/Updates/ Done. http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:43: // Check that widget has been created or is not closed. On 2011/10/24 15:52:10, Daniel Erat wrote: > nit: Checks Done. http://codereview.chromium.org/8319008/diff/35001/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble.h:44: // Create widget if needed. On 2011/10/24 15:52:10, Daniel Erat wrote: > nit: Creates Done.
http://codereview.chromium.org/8319008/diff/33011/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/33011/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:130: gfx::Point SettingLevelBubble::GetAnchorPoint() const { msw: I think we can consider making GetAnchorPoint non-const: the case I run into here is that the AnchorPoint need to call GetPreferredSize() on the view, which is non const. I ended up adding view_size_ instead.
http://codereview.chromium.org/8319008/diff/33011/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/33011/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:130: gfx::Point SettingLevelBubble::GetAnchorPoint() const { On 2011/10/24 17:56:56, alicet1 wrote: > msw: I think we can consider making GetAnchorPoint non-const: the case I run > into here is that the AnchorPoint need to call GetPreferredSize() on the view, > which is non const. I ended up adding view_size_ instead. That's reasonable, you can go ahead and change that.
http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:34: static void ShowBubble(views::Widget* widget, 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()).
On 2011/10/24 19:53:10, Daniel Erat wrote: > http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... > File chrome/browser/chromeos/setting_level_bubble.h (right): > > http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... > chrome/browser/chromeos/setting_level_bubble.h:34: static void > ShowBubble(views::Widget* widget, > 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()). (I'll be in MTV tomorrow, if this is easier to discuss in person.)
msw: ok, I'll put the GetAnchorPoint() change in another change. dan: see comments below. thanx, alice http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/32002/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:34: static void ShowBubble(views::Widget* widget, On 2011/10/24 19:53:10, Daniel Erat wrote: > 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()). 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.
reworked -- brightness and volume bubble will be untouched but inherit this change. also added tests for all three bubbles. the change to bubble_delegate.cc/h will be removed from this CL once http://codereview.chromium.org/8387020/ lands. thanx, alice
I haven't looked at the tests yet, but I'm much happier with this approach -- thanks! Re the bubble_delegate changes, if you're using git-cl, I think that you can supply a branch name (e.g. "git cl upload branch_containing_just_delegate_change") to upload a diff against that branch instead of against the upstream branch. This can be useful when working on a change that's dependent on code that isn't checked in yet. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:141: view_ = new SettingLevelBubbleView(); should probably call the SettingLevelBubbleView's Init() method here -- see below http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:149: view_= NULL; nit: "view_ =" http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:216: if (view_) { nit: no curly braces for single-line if statements in chromium http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:217: view_->Init(current_icon, percent, enabled); i think that it's the case in all of the chrome code that i've seen that Init() methods are intended to only be called once -- it's just a way to move more heavyweight work (or things that can fail) outside of the constructor. i'd recommend sticking to that pattern here, and instead calling the view's individual SetIcon/Level/Enabled methods. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:30: // Hides the Bubble, close the view. nit: ", closing the view"
actually, the change to bubble_delegate will stay in this CL. thanx, alice http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.cc (right): http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:141: view_ = new SettingLevelBubbleView(); On 2011/10/26 05:08:01, Daniel Erat wrote: > should probably call the SettingLevelBubbleView's Init() method here -- see > below yes, didn't do that for a reason -- the view's init needs current_icon, percent, and enabled args. The delegate, after the rework, doesn't hold any of these members, so I opt for calling Init() at where ShowBubble() calls it. let me see if you think that's still ok. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:149: view_= NULL; On 2011/10/26 05:08:01, Daniel Erat wrote: > nit: "view_ =" Done. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:216: if (view_) { On 2011/10/26 05:08:01, Daniel Erat wrote: > nit: no curly braces for single-line if statements in chromium Done. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.cc:217: view_->Init(current_icon, percent, enabled); On 2011/10/26 05:08:01, Daniel Erat wrote: > i think that it's the case in all of the chrome code that i've seen that Init() > methods are intended to only be called once -- it's just a way to move more > heavyweight work (or things that can fail) outside of the constructor. i'd > recommend sticking to that pattern here, and instead calling the view's > individual SetIcon/Level/Enabled methods. removed. http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/41001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:30: // Hides the Bubble, close the view. On 2011/10/26 05:08:01, Daniel Erat wrote: > nit: ", closing the view" Done.
LGTM. Thanks for the tests! http://codereview.chromium.org/8319008/diff/39013/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/39013/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:61: // Creates the bubble content view. also mention that the caller needs to call Init()
ben for views/bubble/... approval? thanx, alice
http://codereview.chromium.org/8319008/diff/39013/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble.h (right): http://codereview.chromium.org/8319008/diff/39013/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble.h:61: // Creates the bubble content view. On 2011/10/26 16:51:43, Daniel Erat wrote: > also mention that the caller needs to call Init() Done.
lgtm
LGTM with nits. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble_browsertest.cc:9: #include "third_party/skia/include/core/SkBitmap.h" What is the SkBitmap include used for? http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble_browsertest.cc:19: EXPECT_FALSE(bubble->view_ != NULL); Use EXPECT_EQ and EXPECT_NE for several checks in this test. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_browsertest.cc:28: EXPECT_FALSE(bubble.view_ != NULL); Use EXPECT_EQ and EXPECT_NE here. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_browsertest.cc:59: EXPECT_FALSE(bubble.view_ != NULL); ditto here and line 65. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:34: AddChildView(progress_bar_); Why are you moving this out of init? http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.h (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.h:49: FRIEND_TEST_ALL_PREFIXES(SettingLevelBubbleTest, CreateAndUpdate); I'm not sure if there's an official style reccomendation, but I usually see friends listed above members. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:9: #include "third_party/skia/include/core/SkBitmap.h" Is SkBitmap needed here? Please keep includes to what is required. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:22: EXPECT_TRUE(bubble1->view_ != NULL); Use EXPECT_EQ and EXPECT_NE for several checks in this test. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:29: EXPECT_FALSE(bubble1->view_ == saved_view); EXPECT_NE
thanks. updated styles, and ran valgrind on new tests. thanx, alice http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... File chrome/browser/chromeos/brightness_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble_browsertest.cc:9: #include "third_party/skia/include/core/SkBitmap.h" On 2011/10/26 18:51:42, msw wrote: > What is the SkBitmap include used for? removed. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/bri... chrome/browser/chromeos/brightness_bubble_browsertest.cc:19: EXPECT_FALSE(bubble->view_ != NULL); On 2011/10/26 18:51:42, msw wrote: > Use EXPECT_EQ and EXPECT_NE for several checks in this test. Done. see response in setting_level_bubble_browsertest.cc http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_browsertest.cc:28: EXPECT_FALSE(bubble.view_ != NULL); On 2011/10/26 18:51:42, msw wrote: > Use EXPECT_EQ and EXPECT_NE here. EXPECT_NE needs a casting for the null pointer, or a typed null pointer to compare to. I think expect_true(ptr != NULL) is still pretty readable, leaving it in. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_browsertest.cc:59: EXPECT_FALSE(bubble.view_ != NULL); On 2011/10/26 18:51:42, msw wrote: > ditto here and line 65. Done. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:34: AddChildView(progress_bar_); On 2011/10/26 18:51:42, msw wrote: > Why are you moving this out of init? the view is created as part of bubble delegate init, but the Init() call in this class requires args that the delegate doesn't hold, so after the bubble widget is created, the caller (SettingLevelBubble) who does hold all the relevant bits can initialize the view. SettingLevelBubble is one of those bubbles that need to keep updating its views after it is created, to refresh on the setting display, so it is holding on to the view. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.h (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.h:49: FRIEND_TEST_ALL_PREFIXES(SettingLevelBubbleTest, CreateAndUpdate); On 2011/10/26 18:51:42, msw wrote: > I'm not sure if there's an official style reccomendation, but I usually see > friends listed above members. Done. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... File chrome/browser/chromeos/volume_bubble_browsertest.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:9: #include "third_party/skia/include/core/SkBitmap.h" On 2011/10/26 18:51:42, msw wrote: > Is SkBitmap needed here? > Please keep includes to what is required. Done. http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:22: EXPECT_TRUE(bubble1->view_ != NULL); On 2011/10/26 18:51:42, msw wrote: > Use EXPECT_EQ and EXPECT_NE for several checks in this test. done. see response in setting_level_bubble_browsertest.cc http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/vol... chrome/browser/chromeos/volume_bubble_browsertest.cc:29: EXPECT_FALSE(bubble1->view_ == saved_view); On 2011/10/26 18:51:42, msw wrote: > EXPECT_NE Done.
http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:34: AddChildView(progress_bar_); On 2011/10/27 00:34:51, alicet1 wrote: > On 2011/10/26 18:51:42, msw wrote: > > Why are you moving this out of init? > > the view is created as part of bubble delegate init, but the Init() call in this > class requires args that the delegate doesn't hold, so after the bubble widget > is created, the caller (SettingLevelBubble) who does hold all the relevant bits > can initialize the view. SettingLevelBubble is one of those bubbles that need to > keep updating its views after it is created, to refresh on the setting display, > so it is holding on to the view. Init() should still just be getting called once, right? If so, can this be moved back there (and maybe a DCHECK(!progress_bar_) be added at the top of Init() to make sure that it doesn't get called more than once)?
http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/43014/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:34: AddChildView(progress_bar_); On 2011/10/27 00:40:21, Daniel Erat wrote: > On 2011/10/27 00:34:51, alicet1 wrote: > > On 2011/10/26 18:51:42, msw wrote: > > > Why are you moving this out of init? > > > > the view is created as part of bubble delegate init, but the Init() call in > this > > class requires args that the delegate doesn't hold, so after the bubble widget > > is created, the caller (SettingLevelBubble) who does hold all the relevant > bits > > can initialize the view. SettingLevelBubble is one of those bubbles that need > to > > keep updating its views after it is created, to refresh on the setting > display, > > so it is holding on to the view. > > Init() should still just be getting called once, right? If so, can this be > moved back there (and maybe a DCHECK(!progress_bar_) be added at the top of > Init() to make sure that it doesn't get called more than once)? init is still called only once (per lifetime of the SettingLevelBubbleView), added a dcheck on icon_, and left the progress bar creation at creation.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/8319008/50001
http://codereview.chromium.org/8319008/diff/50001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/50001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:33: progress_bar_ = new views::ProgressBar(); FWIW, I'm still confused by this and think it's wrong.
Try job failure for 8319008-50001 (retry) on linux_rel for step "ui_tests". It's a second try, previously, step "ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
From offline convo: Okay, that makes sense. Please share explanations on the CR when possible. In that case, I'd suggest using the progress_bar_ initializer on line 31 instead of a separate assignment in the ctor body. Also, you can delay AddChildView(progress_bar_) until Init. Mike On Thu, Oct 27, 2011 at 12:03 PM, Alice Tull <alicet@google.com> wrote: ah, I actually did tell dan why this is the case -- ---------- Forwarded message ---------- From: Alice Tull <alicet@google.com> Date: Thu, Oct 27, 2011 at 8:34 AM Subject: Re: aura: brightness and volume bubble. (issue 8319008) To: Daniel Erat <derat@google.com> oh, that last bit there needs some explanation -- At bubble widget creation, when widget first created the content views etc, Layout() is called to layout the children views. Layout() method in the SettingLevelBubbleView() class wants to call progress_bar_ to find out its bounds. That, without protection, will cause a dump. This wasn't a problem in the original code as the original code is view added to a widget after the widget is created. The solution is either to move up the progress bar creation, or condition every method in the class with if (progress_bar_) { }. I think creating the progress_bar up front is fine, and Init() is indeed only called once to set the initial state of this view. sorry if this seems warped. thanx, alice
http://codereview.chromium.org/8319008/diff/50001/chrome/browser/chromeos/set... File chrome/browser/chromeos/setting_level_bubble_view.cc (right): http://codereview.chromium.org/8319008/diff/50001/chrome/browser/chromeos/set... chrome/browser/chromeos/setting_level_bubble_view.cc:33: progress_bar_ = new views::ProgressBar(); On 2011/10/27 18:30:52, msw wrote: > FWIW, I'm still confused by this and think it's wrong. updated.
Thanks; LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alicet@chromium.org/8319008/50002
Change committed as 107821 |
