Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1932433002:
Update activity indicator from a light bar to a point (Closed)
Descriptionupdate activity indicator from a light bar to a point
TEST=manual
1. Sets the shelf position to left, right and bottom
2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5
3. Verifies if the activity indicator disappears when browser is closed.
4. Uses Gimp to measure the spacing
BUG=595007
Committed: https://crrev.com/f2ec84d242e07a67019642b64560a374c5df36c3
Cr-Commit-Position: refs/heads/master@{#393897}
Patch Set 1 #
Total comments: 18
Patch Set 2 : add MaterialDesignController and address comments #
Total comments: 21
Patch Set 3 : address comments #Patch Set 4 : add antialias flag to draw circle #
Total comments: 8
Patch Set 5 : address @varkha comments #
Total comments: 4
Patch Set 6 : add shelf material control and address comments #
Total comments: 3
Patch Set 7 : address comments #
Total comments: 2
Patch Set 8 : resolve compilation errors #
Messages
Total messages: 47 (16 generated)
Description was changed from ========== update activity indicator from a light bar to a point BUG=595007 ========== to ========== update activity indicator from a light bar to a point TEST (visual): 1. change shelf position to left, right and bottom 2. use --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. check if the activity indicator disappear when browser is closed. BUG=595007 ==========
yiyix@chromium.org changed reviewers: + bruthig@chromium.org, varkha@chromium.org
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1932433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1932433002/1
Description was changed from ========== update activity indicator from a light bar to a point TEST (visual): 1. change shelf position to left, right and bottom 2. use --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. check if the activity indicator disappear when browser is closed. BUG=595007 ========== to ========== update activity indicator from a light bar to a point TEST=manual 1. change shelf position to left, right and bottom 2. use --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. check if the activity indicator disappear when browser is closed. BUG=595007 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
It appears the ShelfButton::BarView has an animation wired in to it to indicate the ShelfButton::State::STATE_ATTENTION state. I suspect this will affect your new indicator circle as well. Is this desired? (This animation can be seen if you have a hangouts window open in the background and it receives a message.) Since the "alert" issue is in the hands of the designers (i.e. https://crbug.com/597372) it may be easier to create a base class ShelfButton::BaseBarView and subclass it with the existing ShelfButton::BarView and a new ShelfButton::BarViewMD class. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:494: // gfx::ImageSkia image = *rb->GetImageNamed(bar_id).ToImageSkia(); FYI You will need to 'hide' this feature behind the experimental flag that varkha@ is adding here: https://codereview.chromium.org/1921133002/ https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:519: SkPaint paint; Make sure to #include everything you use. i.e. SkPaint. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:520: paint.setColor(SK_ColorWHITE); nit: Assign SK_ColorWHITE to a constant above and use the constant here. Ditto for the 'magic' radius value of 2 below. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:521: int patVertical = kBarPadVertical + kBarRadius; Should this be 'padVertical'? https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:522: skcanvas->drawCircle(kShelfIconWidth / 2, kShelfIconHeight - patVertical, 2, Is it necessary to paint to the SkCanvas or would gfx::Canvas::DrawCircle() work? https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:93: // Create an light bar image nit: spelling 'a' instead of 'an' nit: Use full punctuation, i.e. a period at the end.
https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:493: // ResourceBundle* rb = &ResourceBundle::GetSharedInstance(); nit: You probably want to remove this before landing (no commented code or LOG statements in the commits). https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:93: // Create an light bar image nit: s/Create/Creates (https://google.github.io/styleguide/cppguide.html#Function_Comments - "descriptive ("Opens the file") rather than imperative ("Open the file")"). https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:94: void GetActivityIndicatorImage(gfx::Canvas& canvas); Maybe GetIndicatorImage() since we are not planning on doing anything different based on active state.
On 2016/04/27 18:56:01, bruthig wrote: > It appears the ShelfButton::BarView has an animation wired in to it to indicate > the ShelfButton::State::STATE_ATTENTION state. I suspect this will affect your > new indicator circle as well. Is this desired? This animation will cause my indicator circle to flash, but the circle size will not change. The exact animation for "throbbing" is not yet defined, i.e., BUG=597372. I thinking having a flashing indicator circle is an acceptable intermediate effect. > > (This animation can be seen if you have a hangouts window open in the background > and it receives a message.) > > Since the "alert" issue is in the hands of the designers (i.e. > https://crbug.com/597372) it may be easier to create a base class > ShelfButton::BaseBarView and subclass it with the existing ShelfButton::BarView > and a new ShelfButton::BarViewMD class. > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc > File ash/shelf/shelf_button.cc (right): > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... > ash/shelf/shelf_button.cc:494: // gfx::ImageSkia image = > *rb->GetImageNamed(bar_id).ToImageSkia(); > FYI You will need to 'hide' this feature behind the experimental flag that > varkha@ is adding here: https://codereview.chromium.org/1921133002/ > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... > ash/shelf/shelf_button.cc:519: SkPaint paint; > Make sure to #include everything you use. i.e. SkPaint. > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... > ash/shelf/shelf_button.cc:520: paint.setColor(SK_ColorWHITE); > nit: Assign SK_ColorWHITE to a constant above and use the constant here. > Ditto for the 'magic' radius value of 2 below. > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... > ash/shelf/shelf_button.cc:521: int patVertical = kBarPadVertical + kBarRadius; > Should this be 'padVertical'? > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... > ash/shelf/shelf_button.cc:522: skcanvas->drawCircle(kShelfIconWidth / 2, > kShelfIconHeight - patVertical, 2, > Is it necessary to paint to the SkCanvas or would gfx::Canvas::DrawCircle() > work? > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h > File ash/shelf/shelf_button.h (right): > > https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... > ash/shelf/shelf_button.h:93: // Create an light bar image > nit: spelling 'a' instead of 'an' > nit: Use full punctuation, i.e. a period at the end.
https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:493: // ResourceBundle* rb = &ResourceBundle::GetSharedInstance(); On 2016/04/28 16:49:03, varkha wrote: > nit: You probably want to remove this before landing (no commented code or LOG > statements in the commits). I keep these lines, so it is easier to add the ash-md flag. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:494: // gfx::ImageSkia image = *rb->GetImageNamed(bar_id).ToImageSkia(); On 2016/04/27 18:56:00, bruthig wrote: > FYI You will need to 'hide' this feature behind the experimental flag that > varkha@ is adding here: https://codereview.chromium.org/1921133002/ Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:519: SkPaint paint; On 2016/04/27 18:56:00, bruthig wrote: > Make sure to #include everything you use. i.e. SkPaint. Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:520: paint.setColor(SK_ColorWHITE); On 2016/04/27 18:56:00, bruthig wrote: > nit: Assign SK_ColorWHITE to a constant above and use the constant here. > Ditto for the 'magic' radius value of 2 below. Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:521: int patVertical = kBarPadVertical + kBarRadius; On 2016/04/27 18:56:00, bruthig wrote: > Should this be 'padVertical'? Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.cc#n... ash/shelf/shelf_button.cc:522: skcanvas->drawCircle(kShelfIconWidth / 2, kShelfIconHeight - patVertical, 2, On 2016/04/27 18:56:00, bruthig wrote: > Is it necessary to paint to the SkCanvas or would gfx::Canvas::DrawCircle() > work? Good catch. I draw the circle in Canvas now. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:93: // Create an light bar image On 2016/04/27 18:56:00, bruthig wrote: > nit: spelling 'a' instead of 'an' > nit: Use full punctuation, i.e. a period at the end. Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:93: // Create an light bar image On 2016/04/28 16:49:04, varkha wrote: > nit: s/Create/Creates > (https://google.github.io/styleguide/cppguide.html#Function_Comments - > "descriptive ("Opens the file") rather than imperative ("Open the file")"). Done. https://codereview.chromium.org/1932433002/diff/1/ash/shelf/shelf_button.h#ne... ash/shelf/shelf_button.h:94: void GetActivityIndicatorImage(gfx::Canvas& canvas); On 2016/04/28 16:49:04, varkha wrote: > Maybe GetIndicatorImage() since we are not planning on doing anything different > based on active state. Done.
yiyix@chromium.org changed reviewers: + tdanderson@chromium.org
Add @tdanderson
lgtm
LGTM with some nits below https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: if (MaterialDesignController::IsMaterial()) { varkha@, side comment: should we consider giving the Ash MDC a different class name? After reading line 497 I found that I had to scroll to the #includes to check that we were using the Ash version and not the top chrome one. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:502: nit: remove blank line https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:525: void ShelfButton::GetIndicatorImage(gfx::Canvas& canvas) { nit: the Google C++ style guide states that reference parameters should always be const (https://google.github.io/styleguide/cppguide.html#Reference_Arguments) https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:528: int pad_vertical = kIconInsideVertical + kIndicatorRadius; extreme nit, possibly personal preference: declare |pad_vertical| to be const since it is a local variable which is never mutated. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. nit: Remove "BUG=595007" from the CL title. nit: Include a brief CL description explaining what your CL is doing, and be sure to mention that this change is being landed behind an experimental flag for CrOS MD. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:94: void GetIndicatorImage(gfx::Canvas& canvas); nit: I would prefer this be called something like "PaintIndicatorImage" instead. Starting the function name with "Get" is misleading since it isn't returning anything.
https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:42: const int kShelfIconHeight = 48; Is this defined / dependent on a constant that is defined elsewhere? https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:44: const int kIconInsideVertical = 3; nit: rename kIndicatorOffsetFromBottom. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: if (MaterialDesignController::IsMaterial()) { On 2016/05/02 17:58:35, tdanderson wrote: > varkha@, side comment: should we consider giving the Ash MDC a different class > name? After reading line 497 I found that I had to scroll to the #includes to > check that we were using the Ash version and not the top chrome one. Would using explicit namespace here help? I think I have seen examples like that. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:528: int pad_vertical = kIconInsideVertical + kIndicatorRadius; nit: I would just remove the pad_vertical and inline in the invocation of DrawCircle below. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:530: gfx::Point(kShelfIconWidth / 2, kShelfIconHeight - pad_vertical), Would it be safer to use canvas properties instead of the constants? This way the constants are only used in one place to size the canvas. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:94: void GetIndicatorImage(gfx::Canvas& canvas); On 2016/05/02 17:58:35, tdanderson wrote: > nit: I would prefer this be called something like "PaintIndicatorImage" instead. > Starting the function name with "Get" is misleading since it isn't returning > anything. Or maybe PaintIndicatorOnCanvas to self-document the parameter.
https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: if (MaterialDesignController::IsMaterial()) { On 2016/05/02 18:13:58, varkha wrote: > On 2016/05/02 17:58:35, tdanderson wrote: > > varkha@, side comment: should we consider giving the Ash MDC a different class > > name? After reading line 497 I found that I had to scroll to the #includes to > > check that we were using the Ash version and not the top chrome one. > > Would using explicit namespace here help? I think I have seen examples like > that. Yes, though since there is no way to force all future CLs to do that.
https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:42: const int kShelfIconHeight = 48; On 2016/05/02 18:13:58, varkha wrote: > Is this defined / dependent on a constant that is defined elsewhere? You are right, this is defined in shelf_constants. Width = 44 and height = 47. I will update the code to use the current constraints. When I implement BUG 595005, I will update it to use the new spec. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:44: const int kIconInsideVertical = 3; On 2016/05/02 18:13:59, varkha wrote: > nit: rename kIndicatorOffsetFromBottom. Done. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: if (MaterialDesignController::IsMaterial()) { On 2016/05/02 18:19:55, tdanderson wrote: > On 2016/05/02 18:13:58, varkha wrote: > > On 2016/05/02 17:58:35, tdanderson wrote: > > > varkha@, side comment: should we consider giving the Ash MDC a different > class > > > name? After reading line 497 I found that I had to scroll to the #includes > to > > > check that we were using the Ash version and not the top chrome one. > > > > Would using explicit namespace here help? I think I have seen examples like > > that. > > Yes, though since there is no way to force all future CLs to do that. I added ash for this cl. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:502: On 2016/05/02 17:58:35, tdanderson wrote: > nit: remove blank line Done. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:525: void ShelfButton::GetIndicatorImage(gfx::Canvas& canvas) { On 2016/05/02 17:58:35, tdanderson wrote: > nit: the Google C++ style guide states that reference parameters should always > be const (https://google.github.io/styleguide/cppguide.html#Reference_Arguments) Since I am drawing a circle on this canvas, it is same as the String *out in the guide. I updated gfx::Canvas& canvas to gfx::Canvas *canvas. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:528: int pad_vertical = kIconInsideVertical + kIndicatorRadius; On 2016/05/02 17:58:35, tdanderson wrote: > extreme nit, possibly personal preference: declare |pad_vertical| to be const > since it is a local variable which is never mutated. Done. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:530: gfx::Point(kShelfIconWidth / 2, kShelfIconHeight - pad_vertical), On 2016/05/02 18:13:59, varkha wrote: > Would it be safer to use canvas properties instead of the constants? This way > the constants are only used in one place to size the canvas. Since it is harder to get these properties from canvas and the size does not change from the time we create the Canvas to drawing the circle on it, I will pass the gfx::size to this function so it does not look like random constants from the code. https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/20001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:94: void GetIndicatorImage(gfx::Canvas& canvas); On 2016/05/02 17:58:35, tdanderson wrote: > nit: I would prefer this be called something like "PaintIndicatorImage" instead. > Starting the function name with "Get" is misleading since it isn't returning > anything. I agree. I changed it to PaintIndicatorOnCanvas, i found this name is more descriptive.
Description was changed from ========== update activity indicator from a light bar to a point TEST=manual 1. change shelf position to left, right and bottom 2. use --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. check if the activity indicator disappear when browser is closed. BUG=595007 ========== to ========== update activity indicator from a light bar to a point TEST=manual 1. Sets the shelf position to left, right and bottom 2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. Verifies if the activity indicator disappears when browser is closed. 4. Uses Gimp to measure the spacing BUG=595007 ==========
I think this is almost looking good, just a couple small suggestions. https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: gfx::Size size = gfx::Size(kShelfButtonSize, kShelfSize); nit: Simpler: gfx::Size size(kShelfButtonSize, kShelfSize); https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:498: // Chrome os is most common to run with 100% or 200% scale as default nit: s/Chrome os/Chrome OS https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:500: // least 4. I suggest moving the scale to be a constant in anonymous namespace along with a comment that could say something like: // Canvas scale to ensures that the activity indicator is not pixelated even at // the highest possible device scale factors. const int kIndicatorCanvasScale = 5; https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:93: // Creates an activity indicator image. The comment seems out of date now. How about "Paints an activity indicator on |canvas| whose |size| is specified in DIP."
https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:497: gfx::Size size = gfx::Size(kShelfButtonSize, kShelfSize); On 2016/05/05 14:40:56, varkha wrote: > nit: Simpler: > gfx::Size size(kShelfButtonSize, kShelfSize); Done. https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:498: // Chrome os is most common to run with 100% or 200% scale as default On 2016/05/05 14:40:56, varkha wrote: > nit: s/Chrome os/Chrome OS Done. https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:500: // least 4. On 2016/05/05 14:40:56, varkha wrote: > I suggest moving the scale to be a constant in anonymous namespace along with a > comment that could say something like: > // Canvas scale to ensures that the activity indicator is not pixelated even at > // the highest possible device scale factors. > const int kIndicatorCanvasScale = 5; I like your comment much more. It explains the purpose of this choice more clearly. https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/60001/ash/shelf/shelf_button.... ash/shelf/shelf_button.h:93: // Creates an activity indicator image. On 2016/05/05 14:40:56, varkha wrote: > The comment seems out of date now. How about > "Paints an activity indicator on |canvas| whose |size| is specified in DIP." Done. sorry, what is DIP stands for?
yiyix@chromium.org changed reviewers: + sky@chromium.org
LGTM with a nit. https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:47: // Canvas scale to ensures that the activity indicator is not pixelated even at nit: s/ensures/ensure
https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:527: void ShelfButton::PaintIndicatorOnCanvas(gfx::Canvas* canvas, Make this a non-member function (see Effective C++ item 23).
https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:47: // Canvas scale to ensures that the activity indicator is not pixelated even at On 2016/05/10 21:04:11, varkha wrote: > nit: s/ensures/ensure Done. https://codereview.chromium.org/1932433002/diff/80001/ash/shelf/shelf_button.... ash/shelf/shelf_button.cc:527: void ShelfButton::PaintIndicatorOnCanvas(gfx::Canvas* canvas, On 2016/05/12 15:27:03, sky wrote: > Make this a non-member function (see Effective C++ item 23). Done.
https://codereview.chromium.org/1932433002/diff/100001/ash/material_design/ma... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1932433002/diff/100001/ash/material_design/ma... ash/material_design/material_design_controller.h:44: // Returns true if shelf is using Material Design nit: period at the end of a sentence. Maybe "Returns true if Material Design features are enabled for Chrome OS shelf." https://codereview.chromium.org/1932433002/diff/100001/ash/shelf/shelf_button.h File ash/shelf/shelf_button.h (right): https://codereview.chromium.org/1932433002/diff/100001/ash/shelf/shelf_button... ash/shelf/shelf_button.h:115: void PaintIndicatorOnCanvas(gfx::Canvas* canvas, const gfx::Size& size); Could it be in anonymous namespace and not exposed?
https://codereview.chromium.org/1932433002/diff/100001/ash/material_design/ma... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1932433002/diff/100001/ash/material_design/ma... ash/material_design/material_design_controller.h:44: // Returns true if shelf is using Material Design On 2016/05/13 23:01:24, varkha wrote: > nit: period at the end of a sentence. > Maybe "Returns true if Material Design features are enabled for Chrome OS > shelf." I like it better. It explicitly says that this controller is designed for the Chrome OS shelf. Done.
Still lgtm.
LGTM
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1932433002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1932433002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1932433002/diff/120001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/120001/ash/shelf/shelf_button... ash/shelf/shelf_button.cc:45: const int kIndicatorColor = SK_ColorWHITE; const SkColor instead of const int should solve the compilation errors.
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1932433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1932433002/140001
https://codereview.chromium.org/1932433002/diff/120001/ash/shelf/shelf_button.cc File ash/shelf/shelf_button.cc (right): https://codereview.chromium.org/1932433002/diff/120001/ash/shelf/shelf_button... ash/shelf/shelf_button.cc:45: const int kIndicatorColor = SK_ColorWHITE; On 2016/05/16 16:37:11, varkha wrote: > const SkColor instead of const int should solve the compilation errors. You are right! Sorry, I made a careless choice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yiyix@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, bruthig@chromium.org, varkha@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/1932433002/#ps140001 (title: "resolve compilation errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1932433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1932433002/140001
Message was sent while issue was closed.
Description was changed from ========== update activity indicator from a light bar to a point TEST=manual 1. Sets the shelf position to left, right and bottom 2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. Verifies if the activity indicator disappears when browser is closed. 4. Uses Gimp to measure the spacing BUG=595007 ========== to ========== update activity indicator from a light bar to a point TEST=manual 1. Sets the shelf position to left, right and bottom 2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. Verifies if the activity indicator disappears when browser is closed. 4. Uses Gimp to measure the spacing BUG=595007 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== update activity indicator from a light bar to a point TEST=manual 1. Sets the shelf position to left, right and bottom 2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. Verifies if the activity indicator disappears when browser is closed. 4. Uses Gimp to measure the spacing BUG=595007 ========== to ========== update activity indicator from a light bar to a point TEST=manual 1. Sets the shelf position to left, right and bottom 2. Uses --force-device-scale-factor to check the appearance of activity indicator at 2,3,4,5 3. Verifies if the activity indicator disappears when browser is closed. 4. Uses Gimp to measure the spacing BUG=595007 Committed: https://crrev.com/f2ec84d242e07a67019642b64560a374c5df36c3 Cr-Commit-Position: refs/heads/master@{#393897} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f2ec84d242e07a67019642b64560a374c5df36c3 Cr-Commit-Position: refs/heads/master@{#393897} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
