Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790012:
Volume bubble added for ChromeOS. (Closed)
|
Created:
10 years, 6 months ago by glotov Modified:
9 years, 7 months ago CC:
chromium-reviews, davemoore+watch_chromium.org, ben+cc_chromium.org Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionVolume bubble added for ChromeOS.
BUG=crosbug.com/525
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=49970
Patch Set 1 #
Total comments: 34
Patch Set 2 : '' #
Total comments: 53
Patch Set 3 : '' #
Total comments: 4
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 2
Messages
Total messages: 20 (0 generated)
Please fix lint errors - add nl to the end of each added file. http://codereview.chromium.org/2790012/diff/1/2 File app/resources/app_resources.grd (right): http://codereview.chromium.org/2790012/diff/1/2#newcode154 app/resources/app_resources.grd:154: <!-- Volume bubble picture --> Place under <if expr="pp_ifdef('chromeos')"> http://codereview.chromium.org/2790012/diff/1/5 File chrome/browser/chromeos/system_key_event_listener.h (left): http://codereview.chromium.org/2790012/diff/1/5#oldcode45 chrome/browser/chromeos/system_key_event_listener.h:45: File should contain nl character at the end. http://codereview.chromium.org/2790012/diff/1/6 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/1/6#newcode15 chrome/browser/chromeos/volume_bubble.cc:15: const int kTimeout = 2; // 2 sec kBubbleShowTimeoutSec http://codereview.chromium.org/2790012/diff/1/6#newcode15 chrome/browser/chromeos/volume_bubble.cc:15: const int kTimeout = 2; // 2 sec extra empty line before http://codereview.chromium.org/2790012/diff/1/6#newcode16 chrome/browser/chromeos/volume_bubble.cc:16: const int kAnimationDurationMs = 200; extra empty line after http://codereview.chromium.org/2790012/diff/1/6#newcode16 chrome/browser/chromeos/volume_bubble.cc:16: const int kAnimationDurationMs = 200; Depending on how it looks on the device may be it makes sense to increase duration, 400ms for instance. http://codereview.chromium.org/2790012/diff/1/6#newcode22 chrome/browser/chromeos/volume_bubble.cc:22: // TODO(glotov): remove this in favor of enabling InfoBubble class act Please create an issue to track this and place it's # here. http://codereview.chromium.org/2790012/diff/1/6#newcode46 chrome/browser/chromeos/volume_bubble.cc:46: LOG(INFO) << "Active window changed to " << window; Please remove if you don't need this for debug purposes. It will pollute logs. http://codereview.chromium.org/2790012/diff/1/6#newcode65 chrome/browser/chromeos/volume_bubble.cc:65: VolumeBubble::VolumeBubble() : previous_percent_(-1), Please start initializer list on next line. http://codereview.chromium.org/2790012/diff/1/6#newcode76 chrome/browser/chromeos/volume_bubble.cc:76: if (previous_percent_ == -1) previous_percent_ = percent; DCHECK(percent >= 0 && percent <= 100)? http://codereview.chromium.org/2790012/diff/1/6#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: bubble_ = InfoBubble::Show(toplevel_widget, gfx::Rect(300, 700, 0, 0), Please at least extract 300, 700 as a constants. Optional: provide setter methods for them so that VB default placement could be configured. http://codereview.chromium.org/2790012/diff/1/7 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/1/7#newcode1 chrome/browser/chromeos/volume_bubble.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. Does it show a "bubble tail" originating above the specific key? Add TODO if not. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown Please place comments at the line before each member. http://codereview.chromium.org/2790012/diff/1/8 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/1/8#newcode44 chrome/browser/chromeos/volume_bubble_view.cc:44: progress_bar_->SetProgress(progress); Update(progress);
http://codereview.chromium.org/2790012/diff/1/2 File app/resources/app_resources.grd (right): http://codereview.chromium.org/2790012/diff/1/2#newcode154 app/resources/app_resources.grd:154: <!-- Volume bubble picture --> On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Place under <if expr="pp_ifdef('chromeos')"> Done. http://codereview.chromium.org/2790012/diff/1/5 File chrome/browser/chromeos/system_key_event_listener.h (left): http://codereview.chromium.org/2790012/diff/1/5#oldcode45 chrome/browser/chromeos/system_key_event_listener.h:45: Verified, EOL exists. On 2010/06/11 14:22:27, Nikita Kostylev wrote: > File should contain nl character at the end. http://codereview.chromium.org/2790012/diff/1/6 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/1/6#newcode15 chrome/browser/chromeos/volume_bubble.cc:15: const int kTimeout = 2; // 2 sec I try to minimize extra whitespaces: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... On 2010/06/11 14:22:27, Nikita Kostylev wrote: > extra empty line before http://codereview.chromium.org/2790012/diff/1/6#newcode15 chrome/browser/chromeos/volume_bubble.cc:15: const int kTimeout = 2; // 2 sec On 2010/06/11 14:22:27, Nikita Kostylev wrote: > kBubbleShowTimeoutSec Done. http://codereview.chromium.org/2790012/diff/1/6#newcode15 chrome/browser/chromeos/volume_bubble.cc:15: const int kTimeout = 2; // 2 sec On 2010/06/11 14:22:27, Nikita Kostylev wrote: > kBubbleShowTimeoutSec Done. http://codereview.chromium.org/2790012/diff/1/6#newcode16 chrome/browser/chromeos/volume_bubble.cc:16: const int kAnimationDurationMs = 200; I try to minimize extra whitespaces: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Vertic... On 2010/06/11 14:22:27, Nikita Kostylev wrote: > extra empty line after http://codereview.chromium.org/2790012/diff/1/6#newcode22 chrome/browser/chromeos/volume_bubble.cc:22: // TODO(glotov): remove this in favor of enabling InfoBubble class act On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Please create an issue to track this and place it's # here. Done. http://codereview.chromium.org/2790012/diff/1/6#newcode46 chrome/browser/chromeos/volume_bubble.cc:46: LOG(INFO) << "Active window changed to " << window; On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Please remove if you don't need this for debug purposes. > It will pollute logs. Done. http://codereview.chromium.org/2790012/diff/1/6#newcode65 chrome/browser/chromeos/volume_bubble.cc:65: VolumeBubble::VolumeBubble() : previous_percent_(-1), On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Please start initializer list on next line. Done. http://codereview.chromium.org/2790012/diff/1/6#newcode76 chrome/browser/chromeos/volume_bubble.cc:76: if (previous_percent_ == -1) previous_percent_ = percent; On 2010/06/11 14:22:27, Nikita Kostylev wrote: > DCHECK(percent >= 0 && percent <= 100)? Done. http://codereview.chromium.org/2790012/diff/1/6#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: bubble_ = InfoBubble::Show(toplevel_widget, gfx::Rect(300, 700, 0, 0), I would not do it now. When OEM constants are used, there will most likely be function calls here. On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Please at least extract 300, 700 as a constants. > Optional: provide setter methods for them so that VB default placement could be > configured. http://codereview.chromium.org/2790012/diff/1/7 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/1/7#newcode1 chrome/browser/chromeos/volume_bubble.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. Its better be added in the bug description, than the code comments, no? On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Does it show a "bubble tail" originating above the specific key? Add TODO if > not. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown I think that such formatting of comments wastes space and harder to get the idea. On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Please place comments at the line before each member. http://codereview.chromium.org/2790012/diff/1/8 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/1/8#newcode44 chrome/browser/chromeos/volume_bubble_view.cc:44: progress_bar_->SetProgress(progress); On 2010/06/11 14:22:27, Nikita Kostylev wrote: > Update(progress); Done.
Scott, Zel, Here is a volume bubble implementation. Could you have a look? -- Thank you, Denis
http://codereview.chromium.org/2790012/diff/1/7 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/1/7#newcode1 chrome/browser/chromeos/volume_bubble.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/06/11 15:36:48, glotov wrote: > Its better be added in the bug description, than the code comments, no? > > On 2010/06/11 14:22:27, Nikita Kostylev wrote: > > Does it show a "bubble tail" originating above the specific key? Add TODO if > > not. > > Right, please file a cleanup issue. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown Please make each comment a capitalized statement. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S... More informal comments are OK in implementation but not in header. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown On 2010/06/11 15:36:48, glotov wrote: > I think that such formatting of comments wastes space and harder to get the > idea. Please change. Your comment style in headers differs from the style in the project. That should not be the case for new files. http://codereview.chromium.org/2790012/diff/4002/11001 File app/resources/app_resources.grd (right): http://codereview.chromium.org/2790012/diff/4002/11001#newcode154 app/resources/app_resources.grd:154: <!-- Volume bubble picture --> Comment line should be before IDR_VOLUMEBUBBLE_ICON include http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: // TODO(glotov): Place volume bubble over the keys initiated the Issue # to track that change? http://codereview.chromium.org/2790012/diff/4002/11005#newcode90 chrome/browser/chromeos/volume_bubble.cc:90: bubble_ = InfoBubble::Show(toplevel_widget, gfx::Rect(300, 700, 0, 0), Even if code is temporary leaving magic numbers in code is not a good idea. Please introduce constants. http://codereview.chromium.org/2790012/diff/4002/11005#newcode115 chrome/browser/chromeos/volume_bubble.cc:115: LOG(INFO) << "AnimationEnded"; Do you still need this log? http://codereview.chromium.org/2790012/diff/4002/11005#newcode120 chrome/browser/chromeos/volume_bubble.cc:120: LOG(INFO) << "AnimationProgressed: " << animation->GetCurrentValue(); Do you still need this log? http://codereview.chromium.org/2790012/diff/4002/11007 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/4002/11007#newcode42 chrome/browser/chromeos/volume_bubble_view.cc:42: DCHECK(progress >= 0 && progress <= 100); you don't need second DCHECK here. http://codereview.chromium.org/2790012/diff/4002/11007#newcode45 chrome/browser/chromeos/volume_bubble_view.cc:45: progress_bar_->SetProgress(progress); Update(progress);
http://codereview.chromium.org/2790012/diff/4002/11001 File app/resources/app_resources.grd (right): http://codereview.chromium.org/2790012/diff/4002/11001#newcode155 app/resources/app_resources.grd:155: <if expr="pp_ifdef('chromeos')"> This should in chrome/app/theme/theme_resources.grd and the image should be in chrome/app/theme. http://codereview.chromium.org/2790012/diff/4002/11003 File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 chrome/browser/chromeos/system_key_event_listener.cc:26: volume_bubble_(VolumeBubble::instance()) { You should ask for the volumn_bubble_ when you need it. There's no reason to create a VolumeBubble as soon as Chrome starts. http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode8 chrome/browser/chromeos/volume_bubble.cc:8: #include "base/singleton.h" Don't think you need this include as it's in the header already. http://codereview.chromium.org/2790012/diff/4002/11005#newcode24 chrome/browser/chromeos/volume_bubble.cc:24: class ToplevelWidgetHelper : public ActiveWindowWatcherX::Observer { Can't you just ask for the active window when you need it rather than adding an observer? http://codereview.chromium.org/2790012/diff/4002/11005#newcode27 chrome/browser/chromeos/volume_bubble.cc:27: return Singleton<ToplevelWidgetHelper>::get(); Why does ToplevelWidgetHelper need to be a singleton? It's only used by VolumeBubble. http://codereview.chromium.org/2790012/diff/4002/11005#newcode37 chrome/browser/chromeos/volume_bubble.cc:37: if (root) widget = root->GetWidget(); Chrome code nearly never uses single line ifs like this. Please avoid it. http://codereview.chromium.org/2790012/diff/4002/11005#newcode61 chrome/browser/chromeos/volume_bubble.cc:61: DISALLOW_COPY_AND_ASSIGN(ToplevelWidgetHelper); Newline between 60 and 61 http://codereview.chromium.org/2790012/diff/4002/11005#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: // TODO(glotov): Place volume bubble over the keys initiated the Shouldn't the bubble always appear the same spot? In fact shouldn't the volume bubble be rounded and not visually attached to anything? http://codereview.chromium.org/2790012/diff/4002/11005#newcode112 chrome/browser/chromeos/volume_bubble.cc:112: } Reset the animation_ too? http://codereview.chromium.org/2790012/diff/4002/11005#newcode124 chrome/browser/chromeos/volume_bubble.cc:124: (current_percent_ - previous_percent_) * animation->GetCurrentValue()); Tween::ValueBetween(animation->GetCurrentValue(), previous_percent_, current_percent_); http://codereview.chromium.org/2790012/diff/4002/11005#newcode125 chrome/browser/chromeos/volume_bubble.cc:125: } spacing off. http://codereview.chromium.org/2790012/diff/4002/11006 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/4002/11006#newcode9 chrome/browser/chromeos/volume_bubble.h:9: #ifndef CHROME_BROWSER_CHROMEOS_VOLUME_BUBBLE_H_ Guards before includes. http://codereview.chromium.org/2790012/diff/4002/11007 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/4002/11007#newcode47 chrome/browser/chromeos/volume_bubble_view.cc:47: SetBounds(0, 0, kWidth, kHeight); Your bounds are dictated by the info bubble, you shouldn't attempt to change them as you may run into problems. http://codereview.chromium.org/2790012/diff/4002/11007#newcode71 chrome/browser/chromeos/volume_bubble_view.cc:71: return gfx::Size(width(), height()); This should return kWidth/kHeight. http://codereview.chromium.org/2790012/diff/4002/11008 File chrome/browser/chromeos/volume_bubble_view.h (right): http://codereview.chromium.org/2790012/diff/4002/11008#newcode22 chrome/browser/chromeos/volume_bubble_view.h:22: void Init(int progress); Description? http://codereview.chromium.org/2790012/diff/4002/11008#newcode32 chrome/browser/chromeos/volume_bubble_view.h:32: SkBitmap* icon_; Description? http://codereview.chromium.org/2790012/diff/4002/11008#newcode37 chrome/browser/chromeos/volume_bubble_view.h:37: } // namespace chromeos
http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: // TODO(glotov): Place volume bubble over the keys initiated the On 2010/06/11 16:33:17, sky wrote: > Shouldn't the bubble always appear the same spot? In fact shouldn't the volume > bubble be rounded and not visually attached to anything? Nicholas clarified this for me. It's supposed to appear pointing at the key. In that case you'll want to use screen bounds here.
http://codereview.chromium.org/2790012/diff/1/7 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/1/7#newcode1 chrome/browser/chromeos/volume_bubble.h:1: // Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2010/06/11 16:30:08, Nikita Kostylev wrote: > On 2010/06/11 15:36:48, glotov wrote: > > Its better be added in the bug description, than the code comments, no? > > > > On 2010/06/11 14:22:27, Nikita Kostylev wrote: > > > Does it show a "bubble tail" originating above the specific key? Add TODO if > > > not. > > > > > > Right, please file a cleanup issue. Done. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Please make each comment a capitalized statement. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation%2C... > > More informal comments are OK in implementation but not in header. Done. http://codereview.chromium.org/2790012/diff/1/7#newcode42 chrome/browser/chromeos/volume_bubble.h:42: int previous_percent_; // previously shown percentage, -1 if not yet shown On 2010/06/11 16:30:08, Nikita Kostylev wrote: > On 2010/06/11 15:36:48, glotov wrote: > > I think that such formatting of comments wastes space and harder to get the > > idea. > > Please change. Your comment style in headers differs from the style in the > project. That should not be the case for new files. Done. http://codereview.chromium.org/2790012/diff/4002/11001 File app/resources/app_resources.grd (right): http://codereview.chromium.org/2790012/diff/4002/11001#newcode154 app/resources/app_resources.grd:154: <!-- Volume bubble picture --> On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Comment line should be before IDR_VOLUMEBUBBLE_ICON include Done. http://codereview.chromium.org/2790012/diff/4002/11001#newcode155 app/resources/app_resources.grd:155: <if expr="pp_ifdef('chromeos')"> On 2010/06/11 16:33:17, sky wrote: > This should in chrome/app/theme/theme_resources.grd and the image should be in > chrome/app/theme. Done. http://codereview.chromium.org/2790012/diff/4002/11003 File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 chrome/browser/chromeos/system_key_event_listener.cc:26: volume_bubble_(VolumeBubble::instance()) { You found my dirty hack that I hoped will be fixed before being found :) Should I add comments (where)? I need this to begin the current widget tracking. I need to provide it to InfoBubble as a |parent| now. On 2010/06/11 16:33:17, sky wrote: > You should ask for the volumn_bubble_ when you need it. There's no reason to > create a VolumeBubble as soon as Chrome starts. http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode8 chrome/browser/chromeos/volume_bubble.cc:8: #include "base/singleton.h" On 2010/06/11 16:33:17, sky wrote: > Don't think you need this include as it's in the header already. Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode24 chrome/browser/chromeos/volume_bubble.cc:24: class ToplevelWidgetHelper : public ActiveWindowWatcherX::Observer { This is the only way I know. Here I asked about it:http://groups.google.com/a/chromium.org/group/chromium-os-dev/browse_thread/thread/885b6c37b7567614/04fee36651a446cf?lnk=gst&q=Simple+way+to+get+current+toplevel+window# On 2010/06/11 16:33:17, sky wrote: > Can't you just ask for the active window when you need it rather than adding an > observer? http://codereview.chromium.org/2790012/diff/4002/11005#newcode27 chrome/browser/chromeos/volume_bubble.cc:27: return Singleton<ToplevelWidgetHelper>::get(); The idea was this class be standalone watcher. Currently yes, VolumeBubble only uses it. This class is a temporal hack before InfoBubble can accept NULL as a |parent|. Should I modify it anyway? On 2010/06/11 16:33:17, sky wrote: > Why does ToplevelWidgetHelper need to be a singleton? It's only used by > VolumeBubble. http://codereview.chromium.org/2790012/diff/4002/11005#newcode37 chrome/browser/chromeos/volume_bubble.cc:37: if (root) widget = root->GetWidget(); On 2010/06/11 16:33:17, sky wrote: > Chrome code nearly never uses single line ifs like this. Please avoid it. Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode61 chrome/browser/chromeos/volume_bubble.cc:61: DISALLOW_COPY_AND_ASSIGN(ToplevelWidgetHelper); On 2010/06/11 16:33:17, sky wrote: > Newline between 60 and 61 Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode88 chrome/browser/chromeos/volume_bubble.cc:88: // TODO(glotov): Place volume bubble over the keys initiated the On 2010/06/11 16:36:41, sky wrote: > On 2010/06/11 16:33:17, sky wrote: > > Shouldn't the bubble always appear the same spot? In fact shouldn't the volume > > bubble be rounded and not visually attached to anything? > > Nicholas clarified this for me. It's supposed to appear pointing at the key. In > that case you'll want to use screen bounds here. Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode90 chrome/browser/chromeos/volume_bubble.cc:90: bubble_ = InfoBubble::Show(toplevel_widget, gfx::Rect(300, 700, 0, 0), On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Even if code is temporary leaving magic numbers in code is not a good idea. > > Please introduce constants. Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode112 chrome/browser/chromeos/volume_bubble.cc:112: } On 2010/06/11 16:33:17, sky wrote: > Reset the animation_ too? Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode115 chrome/browser/chromeos/volume_bubble.cc:115: LOG(INFO) << "AnimationEnded"; On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Do you still need this log? Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode120 chrome/browser/chromeos/volume_bubble.cc:120: LOG(INFO) << "AnimationProgressed: " << animation->GetCurrentValue(); On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Do you still need this log? Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode124 chrome/browser/chromeos/volume_bubble.cc:124: (current_percent_ - previous_percent_) * animation->GetCurrentValue()); On 2010/06/11 16:33:17, sky wrote: > Tween::ValueBetween(animation->GetCurrentValue(), previous_percent_, > current_percent_); Done. http://codereview.chromium.org/2790012/diff/4002/11005#newcode125 chrome/browser/chromeos/volume_bubble.cc:125: } On 2010/06/11 16:33:17, sky wrote: > spacing off. Done. http://codereview.chromium.org/2790012/diff/4002/11006 File chrome/browser/chromeos/volume_bubble.h (right): http://codereview.chromium.org/2790012/diff/4002/11006#newcode9 chrome/browser/chromeos/volume_bubble.h:9: #ifndef CHROME_BROWSER_CHROMEOS_VOLUME_BUBBLE_H_ On 2010/06/11 16:33:17, sky wrote: > Guards before includes. Done. http://codereview.chromium.org/2790012/diff/4002/11007 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/4002/11007#newcode42 chrome/browser/chromeos/volume_bubble_view.cc:42: DCHECK(progress >= 0 && progress <= 100); I'll leave it for better scene of security. On 2010/06/11 16:30:08, Nikita Kostylev wrote: > you don't need second DCHECK here. http://codereview.chromium.org/2790012/diff/4002/11007#newcode45 chrome/browser/chromeos/volume_bubble_view.cc:45: progress_bar_->SetProgress(progress); I added PaintNow() call to Update() in attempt to avoid flicking. On 2010/06/11 16:30:08, Nikita Kostylev wrote: > Update(progress); http://codereview.chromium.org/2790012/diff/4002/11007#newcode47 chrome/browser/chromeos/volume_bubble_view.cc:47: SetBounds(0, 0, kWidth, kHeight); You are right. Fixed. On 2010/06/11 16:33:17, sky wrote: > Your bounds are dictated by the info bubble, you shouldn't attempt to change > them as you may run into problems. http://codereview.chromium.org/2790012/diff/4002/11007#newcode71 chrome/browser/chromeos/volume_bubble_view.cc:71: return gfx::Size(width(), height()); On 2010/06/11 16:33:17, sky wrote: > This should return kWidth/kHeight. Done. http://codereview.chromium.org/2790012/diff/4002/11008 File chrome/browser/chromeos/volume_bubble_view.h (right): http://codereview.chromium.org/2790012/diff/4002/11008#newcode22 chrome/browser/chromeos/volume_bubble_view.h:22: void Init(int progress); On 2010/06/11 16:33:17, sky wrote: > Description? Done. http://codereview.chromium.org/2790012/diff/4002/11008#newcode32 chrome/browser/chromeos/volume_bubble_view.h:32: SkBitmap* icon_; I change the name to more descriptive. Do you mean some non-trivial extra comment? On 2010/06/11 16:33:17, sky wrote: > Description? http://codereview.chromium.org/2790012/diff/4002/11008#newcode37 chrome/browser/chromeos/volume_bubble_view.h:37: } // namespace On 2010/06/11 16:33:17, sky wrote: > chromeos Done.
Scott, one small question. Currently volume bubble sometimes (rarely but still) is flicking in the progress bar area, when it is redrawn. Do you know a remedy for that? I heard words "double buffering" might help? Or how could we make progress bar redraw in-memory appearing the result on screen?
http://codereview.chromium.org/2790012/diff/4002/11003 File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 chrome/browser/chromeos/system_key_event_listener.cc:26: volume_bubble_(VolumeBubble::instance()) { On 2010/06/11 18:13:36, glotov wrote: > You found my dirty hack that I hoped will be fixed before being found :) Should > I add comments (where)? > I need this to begin the current widget tracking. I need to provide it to > InfoBubble as a |parent| now. > > On 2010/06/11 16:33:17, sky wrote: > > You should ask for the volumn_bubble_ when you need it. There's no reason to > > create a VolumeBubble as soon as Chrome starts. You only need the active window before showing the bubble. You can figure that out when you need it instead of tracking the active window. http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode27 chrome/browser/chromeos/volume_bubble.cc:27: return Singleton<ToplevelWidgetHelper>::get(); On 2010/06/11 18:13:36, glotov wrote: > The idea was this class be standalone watcher. > Currently yes, VolumeBubble only uses it. > This class is a temporal hack before InfoBubble can accept NULL as a |parent|. > Should I modify it anyway? Yes, please fix it. > On 2010/06/11 16:33:17, sky wrote: > > Why does ToplevelWidgetHelper need to be a singleton? It's only used by > > VolumeBubble. > >
It may the same as 43611. Try commenting out the call to DrawTransparentBackground in WidgetGtk::OnPaint. -Scott On Fri, Jun 11, 2010 at 11:26 AM, <glotov@chromium.org> wrote: > Scott, one small question. > > Currently volume bubble sometimes (rarely but still) is flicking in the > progress > bar area, when it is redrawn. > Do you know a remedy for that? I heard words "double buffering" might help? > Or > how could we make progress bar redraw in-memory appearing the result on > screen? > > > > http://codereview.chromium.org/2790012/show >
Oshima just landed this fix, so if you sync you should have it. -Scott On Fri, Jun 11, 2010 at 1:03 PM, Scott Violet <sky@chromium.org> wrote: > It may the same as 43611. Try commenting out the call to > DrawTransparentBackground in WidgetGtk::OnPaint. > > -Scott > > On Fri, Jun 11, 2010 at 11:26 AM, <glotov@chromium.org> wrote: >> Scott, one small question. >> >> Currently volume bubble sometimes (rarely but still) is flicking in the >> progress >> bar area, when it is redrawn. >> Do you know a remedy for that? I heard words "double buffering" might help? >> Or >> how could we make progress bar redraw in-memory appearing the result on >> screen? >> >> >> >> http://codereview.chromium.org/2790012/show >> >
LGTM but please check on trybots. http://codereview.chromium.org/2790012/diff/12002/13007 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/12002/13007#newcode108 chrome/browser/chromeos/volume_bubble.cc:108: if (bubble_) bubble_->Close(); Please split to 2 lines. http://codereview.chromium.org/2790012/diff/12002/13009 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/12002/13009#newcode51 chrome/browser/chromeos/volume_bubble_view.cc:51: PaintNow(); Please remove if Mitsuru CL fixes flickering without this call.
http://codereview.chromium.org/2790012/diff/4002/11003 File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 chrome/browser/chromeos/system_key_event_listener.cc:26: volume_bubble_(VolumeBubble::instance()) { On 2010/06/11 20:02:42, sky wrote: > On 2010/06/11 18:13:36, glotov wrote: > > You found my dirty hack that I hoped will be fixed before being found :) > Should > > I add comments (where)? > > I need this to begin the current widget tracking. I need to provide it to > > InfoBubble as a |parent| now. > > > > On 2010/06/11 16:33:17, sky wrote: > > > You should ask for the volumn_bubble_ when you need it. There's no reason to > > > create a VolumeBubble as soon as Chrome starts. > > You only need the active window before showing the bubble. You can figure that > out when you need it instead of tracking the active window. Done. http://codereview.chromium.org/2790012/diff/4002/11005 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/4002/11005#newcode27 chrome/browser/chromeos/volume_bubble.cc:27: return Singleton<ToplevelWidgetHelper>::get(); On 2010/06/11 20:02:42, sky wrote: > On 2010/06/11 18:13:36, glotov wrote: > > The idea was this class be standalone watcher. > > Currently yes, VolumeBubble only uses it. > > This class is a temporal hack before InfoBubble can accept NULL as a |parent|. > > Should I modify it anyway? > > Yes, please fix it. > > > On 2010/06/11 16:33:17, sky wrote: > > > Why does ToplevelWidgetHelper need to be a singleton? It's only used by > > > VolumeBubble. > > > > > > Done. http://codereview.chromium.org/2790012/diff/12002/13007#newcode108 chrome/browser/chromeos/volume_bubble.cc:108: DCHECK(info_bubble == bubble_); On 2010/06/15 11:27:30, Nikita Kostylev wrote: > Please split to 2 lines. Done. http://codereview.chromium.org/2790012/diff/12002/13009 File chrome/browser/chromeos/volume_bubble_view.cc (right): http://codereview.chromium.org/2790012/diff/12002/13009#newcode51 chrome/browser/chromeos/volume_bubble_view.cc:51: PaintNow(); On 2010/06/15 11:27:30, Nikita Kostylev wrote: > Please remove if Mitsuru CL fixes flickering without this call. Done.
Great, it works now, thank you. On 2010/06/11 20:53:53, sky wrote: > Oshima just landed this fix, so if you sync you should have it. > > -Scott > > On Fri, Jun 11, 2010 at 1:03 PM, Scott Violet <mailto:sky@chromium.org> wrote: > > It may the same as 43611. Try commenting out the call to > > DrawTransparentBackground in WidgetGtk::OnPaint. > > > > -Scott > > > > On Fri, Jun 11, 2010 at 11:26 AM, mailto: <glotov@chromium.org> wrote: > >> Scott, one small question. > >> > >> Currently volume bubble sometimes (rarely but still) is flicking in the > >> progress > >> bar area, when it is redrawn. > >> Do you know a remedy for that? I heard words "double buffering" might help? > >> Or > >> how could we make progress bar redraw in-memory appearing the result on > >> screen? > >> > >> > >> > >> http://codereview.chromium.org/2790012/show > >> > > >
http://codereview.chromium.org/2790012/diff/4002/11003 File chrome/browser/chromeos/system_key_event_listener.cc (right): http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 chrome/browser/chromeos/system_key_event_listener.cc:26: volume_bubble_(VolumeBubble::instance()) { Sorry, hit 'Done' too fast. Scott, how could I figure out active window? If I knew how to do it, I would not need to write ToplevelWidgetHelper tracker. Otherwise, I need to activate ToplevelWidgetHelper code early in the run flow so when user hits volume ket, the active window is already known. On 2010/06/15 12:05:20, glotov wrote: > On 2010/06/11 20:02:42, sky wrote: > > On 2010/06/11 18:13:36, glotov wrote: > > > You found my dirty hack that I hoped will be fixed before being found :) > > Should > > > I add comments (where)? > > > I need this to begin the current widget tracking. I need to provide it to > > > InfoBubble as a |parent| now. > > > > > > On 2010/06/11 16:33:17, sky wrote: > > > > You should ask for the volumn_bubble_ when you need it. There's no reason > to > > > > create a VolumeBubble as soon as Chrome starts. > > > > You only need the active window before showing the bubble. You can figure that > > out when you need it instead of tracking the active window. > > Done.
On Tue, Jun 15, 2010 at 5:58 AM, <glotov@chromium.org> wrote: > > http://codereview.chromium.org/2790012/diff/4002/11003 > File chrome/browser/chromeos/system_key_event_listener.cc (right): > > http://codereview.chromium.org/2790012/diff/4002/11003#newcode26 > chrome/browser/chromeos/system_key_event_listener.cc:26: > volume_bubble_(VolumeBubble::instance()) { > Sorry, hit 'Done' too fast. > > Scott, how could I figure out active window? If I knew how to do it, I > would not need to write ToplevelWidgetHelper tracker. > Otherwise, I need to activate ToplevelWidgetHelper code early in the run > flow so when user hits volume ket, the active window is already known. See the code in ActiveWindowWatcherX::NotifyActiveWindowChanged() that gets the active window. You should be able to move that to a standalone function. -Scott > On 2010/06/15 12:05:20, glotov wrote: >> >> On 2010/06/11 20:02:42, sky wrote: >> > On 2010/06/11 18:13:36, glotov wrote: >> > > You found my dirty hack that I hoped will be fixed before being > > found :) >> >> > Should >> > > I add comments (where)? >> > > I need this to begin the current widget tracking. I need to > > provide it to >> >> > > InfoBubble as a |parent| now. >> > > >> > > On 2010/06/11 16:33:17, sky wrote: >> > > > You should ask for the volumn_bubble_ when you need it. There's > > no reason >> >> to >> > > > create a VolumeBubble as soon as Chrome starts. >> > >> > You only need the active window before showing the bubble. You can > > figure that >> >> > out when you need it instead of tracking the active window. > >> Done. > > http://codereview.chromium.org/2790012/show >
All done. Scott, please have a look. Sorry for missing the idea.
LGTM http://codereview.chromium.org/2790012/diff/48001/49002 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/48001/49002#newcode53 chrome/browser/chromeos/volume_bubble.cc:53: if (percent < 0) percent = 0; Please, no single line ifs.
http://codereview.chromium.org/2790012/diff/48001/49002 File chrome/browser/chromeos/volume_bubble.cc (right): http://codereview.chromium.org/2790012/diff/48001/49002#newcode53 chrome/browser/chromeos/volume_bubble.cc:53: if (percent < 0) percent = 0; On 2010/06/16 15:14:08, sky wrote: > Please, no single line ifs. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
