Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14631022:
Add support for blue buttons. (Closed)
|
Created:
7 years, 7 months ago by benwells Modified:
7 years, 6 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, oshima+watch_chromium.org, reed1, Dan Beam, xiyuan Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd support for blue buttons.
This also uses the new blue button style for
the app launcher signin button.
Binaries were landed in: https://codereview.chromium.org/15848008/
BUG=159733
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203089
Patch Set 1 #
Total comments: 18
Patch Set 2 : Feedback #
Total comments: 8
Patch Set 3 : More feedback #
Total comments: 4
Patch Set 4 : Rebase #Patch Set 5 : Introduce blue_button.[h|cc] #Patch Set 6 : Comment updated #
Total comments: 16
Patch Set 7 : Feedback #Patch Set 8 : Rebase #
Messages
Total messages: 32 (0 generated)
https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... File ui/views/controls/button/label_button.cc (right): https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:31: const SkColor kBlueButtonDisabledColor = SK_ColorWHITE; nit: consider using a single constant for the matching colors. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:158: } else if (style == STYLE_BLUE_BUTTON) { Please implement the blue buttons' 1px #538CEA drop shadow at 90 degrees, specified in the mock. It'll be similar to STYLE_BUTTON's label shadow above: https://docs.google.com/a/chromium.org/folder/d/0B6x6iYCtKinEcUh6eU1aZ2tKRWM/... https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:159: if (label_->enabled() && !explicitly_set_colors_[state()]) Ignore the current enabled/disabled state, both should be white regardless of the button's current state. Also, disregard the explicitly set color bit, unless you have specific client code in mind that sets a button text color then changes the button style to blue. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:160: label_->SetEnabledColor(kBlueButtonEnabledColor); Call SetTextColor here to mark these as explicitly set, so they won't be overwritten by ResetColorsFromNativeTheme, then remove your change there. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:211: if (style() == STYLE_BLUE_BUTTON) This isn't needed if you just call SetTextColor above. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button.cc:243: if (style() == STYLE_BLUE_BUTTON && ! explicitly_set_colors_[state()]) You shouldn't need to modify StateChanged at all. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... File ui/views/controls/button/label_button_border.cc (right): https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button_border.cc:28: const int kBlueNormalImages[] = IMAGE_GRID(IDR_BLUE_BUTTON_NORMAL); nit: consider dropping 'Images' from these constants; and above too if you want. https://chromiumcodereview.appspot.com/14631022/diff/1/ui/views/controls/butt... ui/views/controls/button/label_button_border.cc:135: // The alpha is how much of the foreground state to use. To get this, Hmm, I don't think this is correct, because the images themselves contain partially transparent pixels. For example, imagine a (black, 50% opaque) border pixel in both the background and foreground states. In the way you've changed the code ( basically reverting my http://crrev.com/199342 ), as the animation progresses, the background state will always draw (black, 50% opaque) and then the foreground state will draw (black, alpha*50% opaque) for alpha (0->1). So towards the end of the animation, that pixel will be super dark, close to 100% opaque, then as the animation ends it will instantaneously be back to 50% opaque. This is exactly the jank I fixed in my change, and you can see it more clearly if you increase the animation period. What is the specific defect you are trying to address? That should be mentioned in this comment or at least your CL description.
https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:159: if (label_->enabled() && !explicitly_set_colors_[state()]) On 2013/05/13 18:27:20, msw wrote: > Ignore the current enabled/disabled state, both should be white regardless of > the button's current state. Also, disregard the explicitly set color bit, unless > you have specific client code in mind that sets a button text color then changes > the button style to blue. Yes I was thinking client code should be able to set the font color and set the style in any order, or even set the style to blue button then to something else. Is this kind of use not important? Making assumptions about how the interface is used will simplify the code as I can then use the SetTextColor function as you describe. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button_border.cc:28: const int kBlueNormalImages[] = IMAGE_GRID(IDR_BLUE_BUTTON_NORMAL); On 2013/05/13 18:27:20, msw wrote: > nit: consider dropping 'Images' from these constants; and above too if you want. I think I prefer it being consistent with the normal button constants. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button_border.cc:135: // The alpha is how much of the foreground state to use. To get this, On 2013/05/13 18:27:20, msw wrote: > Hmm, I don't think this is correct, because the images themselves contain > partially transparent pixels. For example, imagine a (black, 50% opaque) border > pixel in both the background and foreground states. In the way you've changed > the code ( basically reverting my http://crrev.com/199342 ), as the animation > progresses, the background state will always draw (black, 50% opaque) and then > the foreground state will draw (black, alpha*50% opaque) for alpha (0->1). So > towards the end of the animation, that pixel will be super dark, close to 100% > opaque, then as the animation ends it will instantaneously be back to 50% > opaque. This is exactly the jank I fixed in my change, and you can see it more > clearly if you increase the animation period. > > What is the specific defect you are trying to address? That should be mentioned > in this comment or at least your CL description. Ah, I think I now understand the intent of that change, but it seems to have made things worse ... maybe the blue buttons show this more than other styles. A good example of how the blending is now wrong is in the opaque parts of the button. For these pixels the background now comes through. E.g, if the background color is B, image one is I1 and image two is I2 and we're halfway through the animation: - after the first paint, the color is B/2 + I1/2 - after the second paint, the color is B/4 + I1/4 + I2/2. So, the final result is 1/4 background, 1/4 first image and 1/2 the second image, instead of 1/2 first image and 1/2 second image which is what we want for opaque pixels in both. For the blue buttons on an almost white background, this shows as a brightening in the middle of the animation. I'm not sure what the best way to fix this is. One way that should work would be to 1. save the original background of the canvas 2. paint the background theme bitmap (without alpha) 3. start an alpha layer with the foreground alpha 4. copy the original background of the canvas into the layer 5. paint the foreground theme bitmap 6. restore, blending the two images together. This should work as when the blending happens there will be no alpha in either image being blended, assuming the canvas is opaque to start with and has the background in it. Is that the case with aura and it's layered structure? Or maybe there is some standard way to do this? It seems like it should be simple.
+CC Mike Reed for a skia expert's take on the image compositing question in: ui/views/controls/button/label_button_border.cc https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:159: if (label_->enabled() && !explicitly_set_colors_[state()]) On 2013/05/14 11:30:44, benwells wrote: > On 2013/05/13 18:27:20, msw wrote: > > Ignore the current enabled/disabled state, both should be white regardless of > > the button's current state. Also, disregard the explicitly set color bit, > unless > > you have specific client code in mind that sets a button text color then > changes > > the button style to blue. > > Yes I was thinking client code should be able to set the font color and set the > style in any order, or even set the style to blue button then to something else. > > Is this kind of use not important? Making assumptions about how the interface is > used will simplify the code as I can then use the SetTextColor function as you > describe. Maybe we can get API flexibility and code simplicity: Move these Label::Set[Enabled|Disabled]Color() calls (not SetTextColor() as my other comment mentions) to ResetColorsFromNativeTheme, as part of your early return. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:160: label_->SetEnabledColor(kBlueButtonEnabledColor); On 2013/05/13 18:27:20, msw wrote: > Call SetTextColor here to mark these as explicitly set, so they won't be > overwritten by ResetColorsFromNativeTheme, then remove your change there. Ignore this! https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button_border.cc:28: const int kBlueNormalImages[] = IMAGE_GRID(IDR_BLUE_BUTTON_NORMAL); On 2013/05/14 11:30:44, benwells wrote: > On 2013/05/13 18:27:20, msw wrote: > > nit: consider dropping 'Images' from these constants; and above too if you > want. > > I think I prefer it being consistent with the normal button constants. Ok, feel free to leave/change both. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button_border.cc:135: // The alpha is how much of the foreground state to use. To get this, On 2013/05/14 11:30:44, benwells wrote: > On 2013/05/13 18:27:20, msw wrote: > > Hmm, I don't think this is correct, because the images themselves contain > > partially transparent pixels. For example, imagine a (black, 50% opaque) > border > > pixel in both the background and foreground states. In the way you've changed > > the code ( basically reverting my http://crrev.com/199342 ), as the animation > > progresses, the background state will always draw (black, 50% opaque) and then > > the foreground state will draw (black, alpha*50% opaque) for alpha (0->1). So > > towards the end of the animation, that pixel will be super dark, close to 100% > > opaque, then as the animation ends it will instantaneously be back to 50% > > opaque. This is exactly the jank I fixed in my change, and you can see it more > > clearly if you increase the animation period. > > > > What is the specific defect you are trying to address? That should be > mentioned > > in this comment or at least your CL description. > > Ah, I think I now understand the intent of that change, but it seems to have > made things worse ... maybe the blue buttons show this more than other styles. > > A good example of how the blending is now wrong is in the opaque parts of the > button. For these pixels the background now comes through. E.g, if the > background color is B, image one is I1 and image two is I2 and we're halfway > through the animation: > - after the first paint, the color is B/2 + I1/2 > - after the second paint, the color is B/4 + I1/4 + I2/2. > > So, the final result is 1/4 background, 1/4 first image and 1/2 the second > image, instead of 1/2 first image and 1/2 second image which is what we want for > opaque pixels in both. > > For the blue buttons on an almost white background, this shows as a brightening > in the middle of the animation. > > I'm not sure what the best way to fix this is. One way that should work would be > to > 1. save the original background of the canvas > 2. paint the background theme bitmap (without alpha) > 3. start an alpha layer with the foreground alpha > 4. copy the original background of the canvas into the layer > 5. paint the foreground theme bitmap > 6. restore, blending the two images together. > > This should work as when the blending happens there will be no alpha in either > image being blended, assuming the canvas is opaque to start with and has the > background in it. Is that the case with aura and it's layered structure? Or > maybe there is some standard way to do this? It seems like it should be simple. Ah, you're right, we must blend the images before compositing with the canvas. I'm not sure I follow the steps you outlined, here's what I would try: 1) SaveLayerAlpha(0xff) (a new layer to blend the images together first) 2) SaveLayerAlpha(0xff-alpha) + Paint background + Restore 3) SaveLayerAlpha(alpha) + Paint foreground + Restore 4) Restore (this should composite the already-blended button images) Perhaps someone that knows Skia better would have a better idea.
drive by question: Are you trying to compute some weighted average of two src-images, and then blit that? If so, you're right that skia doesn't have a direct way to do this today... at least not on all of our backends (e.g. PDF, GPU). We can do it directly today on the Raster backend. However, we are actively working to add this notion to our GPU backend, though it may be a ways out.
On 2013/05/14 17:41:47, reed1 wrote: > drive by question: > > Are you trying to compute some weighted average of two src-images, and then blit > that? > > If so, you're right that skia doesn't have a direct way to do this today... at > least not on all of our backends (e.g. PDF, GPU). We can do it directly today on > the Raster backend. However, we are actively working to add this notion to our > GPU backend, though it may be a ways out. Yes, I believe that's an accurate description of our intent. (basically fading between two images as an animation progresses)
On 2013/05/14 17:50:42, msw wrote: > On 2013/05/14 17:41:47, reed1 wrote: > > drive by question: > > > > Are you trying to compute some weighted average of two src-images, and then > blit > > that? > > > > If so, you're right that skia doesn't have a direct way to do this today... at > > least not on all of our backends (e.g. PDF, GPU). We can do it directly today > on > > the Raster backend. However, we are actively working to add this notion to our > > GPU backend, though it may be a ways out. > > Yes, I believe that's an accurate description of our intent. > (basically fading between two images as an animation progresses) Yep, that's what we need. msw: I tried that approach (and a few varients) but it results in opaque pixels in the images becoming transparent after step 3. So the background still bleeds through. The code that paints the background image then blends the foreground image on top (i.e. pre r199342) works if the foreground imageis both opaque. My suggestion was to make the foreground image opaque by blending it with the original canvas background (i.e. the background of the window) before blending it. But I haven't tried this and I'm not sure if the original canvas background has the window background in it before the paint (it might not if this is on a layer in aura I suppose).
On 2013/05/14 22:40:56, benwells wrote: > On 2013/05/14 17:50:42, msw wrote: > > On 2013/05/14 17:41:47, reed1 wrote: > > > drive by question: > > > > > > Are you trying to compute some weighted average of two src-images, and then > > blit > > > that? > > > > > > If so, you're right that skia doesn't have a direct way to do this today... > at > > > least not on all of our backends (e.g. PDF, GPU). We can do it directly > today > > on > > > the Raster backend. However, we are actively working to add this notion to > our > > > GPU backend, though it may be a ways out. > > > > Yes, I believe that's an accurate description of our intent. > > (basically fading between two images as an animation progresses) > > Yep, that's what we need. > > msw: I tried that approach (and a few varients) but it results in opaque pixels > in the images becoming transparent after step 3. So the background still bleeds > through. > > The code that paints the background image then blends the foreground image on > top (i.e. pre r199342) works if the foreground imageis both opaque. My > suggestion was to make the foreground image opaque by blending it with the > original canvas background (i.e. the background of the window) before blending > it. But I haven't tried this and I'm not sure if the original canvas background > has the window background in it before the paint (it might not if this is on a > layer in aura I suppose). Gah, after trying many combinations of front/back alphas and all the xfer modes, I made a minor breakthrough using SkBitmapOperations::CreateBlendedBitmap, see the debugging/test code at https://codereview.chromium.org/14921006/ This is not yet a solution, but if I can write up a custom SkXfermode using the same logic, that'd be awesome... otherwise, it might be a bigger battle to actually call CreateBlendedBitmap with two different state images for each animation step (since the use of an image painter kinda of encapsulates the actual image loading and painting code).
https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:31: const SkColor kBlueButtonDisabledColor = SK_ColorWHITE; On 2013/05/13 18:27:20, msw wrote: > nit: consider using a single constant for the matching colors. Done. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:159: if (label_->enabled() && !explicitly_set_colors_[state()]) On 2013/05/14 16:20:02, msw wrote: > On 2013/05/14 11:30:44, benwells wrote: > > On 2013/05/13 18:27:20, msw wrote: > > > Ignore the current enabled/disabled state, both should be white regardless > of > > > the button's current state. Also, disregard the explicitly set color bit, > > unless > > > you have specific client code in mind that sets a button text color then > > changes > > > the button style to blue. > > > > Yes I was thinking client code should be able to set the font color and set > the > > style in any order, or even set the style to blue button then to something > else. > > > > Is this kind of use not important? Making assumptions about how the interface > is > > used will simplify the code as I can then use the SetTextColor function as you > > describe. > > Maybe we can get API flexibility and code simplicity: Move these > Label::Set[Enabled|Disabled]Color() calls (not SetTextColor() as my other > comment mentions) to ResetColorsFromNativeTheme, as part of your early return. OK, I've moved the logic into ResetColorsFromNativeTheme and I hope made it fit more into the logic there. https://codereview.chromium.org/14631022/diff/1/ui/views/controls/button/labe... ui/views/controls/button/label_button.cc:243: if (style() == STYLE_BLUE_BUTTON && ! explicitly_set_colors_[state()]) On 2013/05/13 18:27:20, msw wrote: > You shouldn't need to modify StateChanged at all. Done.
https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button.cc:32: // Blue button style default font color. you mean shadow color... https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button.cc:162: label_->SetShadowOffset(1, 1); This should be (0, 1) like above to drop the shadow straight down. https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:133: if (animation && animation->is_animating() && nit: comment above that this animation is incorrect and looks bad for STYLE_BLUE_BUTTON. Make it a TODO(msw) if you want. https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:170: if (style() == Button::STYLE_BLUE_BUTTON) nit: consider moving this up with STYLE_BUTTON, doing: || style() == blue
https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button.cc:32: // Blue button style default font color. On 2013/05/16 17:07:36, msw wrote: > you mean shadow color... Done. https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button.cc:162: label_->SetShadowOffset(1, 1); On 2013/05/16 17:07:36, msw wrote: > This should be (0, 1) like above to drop the shadow straight down. Done. https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:133: if (animation && animation->is_animating() && On 2013/05/16 17:07:36, msw wrote: > nit: comment above that this animation is incorrect and looks bad for > STYLE_BLUE_BUTTON. Make it a TODO(msw) if you want. Done. https://codereview.chromium.org/14631022/diff/39001/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:170: if (style() == Button::STYLE_BLUE_BUTTON) On 2013/05/16 17:07:36, msw wrote: > nit: consider moving this up with STYLE_BUTTON, doing: || style() == blue Done.
Update the CL description (this no longer "also fixes a bug with the hover animations for buttons", but needs details like "adds xyz assets..."), also post a picture or two of the new button (from your dialog or from Views Examples), on the bug, and this LGTM.
+sky for ui/views review +oshima for ui/resources review On 2013/05/17 02:36:21, msw wrote: > Update the CL description (this no longer "also fixes a bug with the hover > animations for buttons", but needs details like "adds xyz assets..."), also post > a picture or two of the new button (from your dialog or from Views Examples), on > the bug, and this LGTM. The assets will be landed in a separate patch to avoid the binary CQ problems. I'll update the description more after that.
ui/resources lgtm
Why does this need to be done in views and not in chrome?
On 2013/05/17 15:44:56, sky wrote: > Why does this need to be done in views and not in chrome? Scott, can you clarify your question? Are you suggesting that the button style be implemented in chrome/* rather than ui/views/control/button/* and ui/resources/*?
Exactly. Styles declared in ui/views/control/button should be ones that we plan on using in lots of places. The blue button seemed like a one off and not something we are going to use extensively. Is that not the case? On Fri, May 17, 2013 at 10:13 AM, <msw@chromium.org> wrote: > On 2013/05/17 15:44:56, sky wrote: >> >> Why does this need to be done in views and not in chrome? > > > Scott, can you clarify your question? Are you suggesting that the button > style > be implemented in chrome/* rather than ui/views/control/button/* and > ui/resources/*? > > https://codereview.chromium.org/14631022/
I think this is part of the work we are doing in crbug.com/155363. Mike should correct me if I'm wrong. -- Thiago
I think the blue button might be used elsewhere, so u/v/c/b seems ok. But it would be better to split styling out to a blue_button.cc, like checkbox, etc. Ideally, label_button would just host common functionality/api and no styling. I'd eventually like to simplify styling so much that it'd be a simple InitParams. Here's how I'd go about it for now; does this seem reasonable? 1) Remove the STYLE_BLUE_BUTTON and subclass LabelButton in blue_button.cc. 2) Override GetThemeAnimation to return NULL (still avoid animation for now). 3) Override GetInsets (or make STYLE_BUTTON the LabelButton default value?). 4) Call SetTextColor() instead of modifying ResetColorsFromNativeTheme. 5) Call LabelButtonBorder::SetPainter from blue_button.cc with the blue images. 6) Add LabelButton::SetShadowColor[AndOffset?] (or ignore for now). 7) Add LabelButton::set_draw_disabled_half_transparent() (or ignore for now). See Thiago's slightly related CL which makes Checkbox a LabelButton: http://codereview.chromium.org/15061006 https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... File ui/views/controls/button/label_button.cc (left): https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... ui/views/controls/button/label_button.cc:230: const SkColor color = button_state_colors_[state()]; nit: restore this const https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:133: // TODO(msw): re-enable animation is for blue buttons. It's disabled now due nit: capitalization and s/animation is/animations/
On 2013/05/18 20:07:24, msw wrote: > I think the blue button might be used elsewhere, so u/v/c/b seems ok. > But it would be better to split styling out to a blue_button.cc, like checkbox, > etc. > Ideally, label_button would just host common functionality/api and no styling. > I'd eventually like to simplify styling so much that it'd be a simple > InitParams. > > Here's how I'd go about it for now; does this seem reasonable? > 1) Remove the STYLE_BLUE_BUTTON and subclass LabelButton in blue_button.cc. > 2) Override GetThemeAnimation to return NULL (still avoid animation for now). > 3) Override GetInsets (or make STYLE_BUTTON the LabelButton default value?). > 4) Call SetTextColor() instead of modifying ResetColorsFromNativeTheme. > 5) Call LabelButtonBorder::SetPainter from blue_button.cc with the blue images. > 6) Add LabelButton::SetShadowColor[AndOffset?] (or ignore for now). > 7) Add LabelButton::set_draw_disabled_half_transparent() (or ignore for now). > > See Thiago's slightly related CL which makes Checkbox a LabelButton: > http://codereview.chromium.org/15061006 > > https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... > File ui/views/controls/button/label_button.cc (left): > > https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... > ui/views/controls/button/label_button.cc:230: const SkColor color = > button_state_colors_[state()]; > nit: restore this const > > https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... > File ui/views/controls/button/label_button_border.cc (right): > > https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... > ui/views/controls/button/label_button_border.cc:133: // TODO(msw): re-enable > animation is for blue buttons. It's disabled now due > nit: capitalization and s/animation is/animations/ If it is structured like that it can be moved around easily, and could start in ui/app_list where it is used now (it's not used in chrome). I'm happy to make the structural changes above, should I do that as part of this patch or in a follow up?
On 2013/05/20 04:28:28, benwells wrote: > If it is structured like that it can be moved around easily, and could start in > ui/app_list where it is used now (it's not used in chrome). I'd prefer to see it in u/v/c/b/, but leave it up to you and Scott. > I'm happy to make the structural changes above, should I do that as part of this > patch or in a follow up? I think it's better done now, in the initial implementation, than with followups.
On 2013/05/20 06:42:43, msw wrote: > I'd prefer to see it in u/v/c/b/, but leave it up to you and Scott. +CC Xiyuan/Dan/Evan. This definitely belongs in /ui, not chrome/. Xiyuan is hoping to use the blue button in the app launcher asap. Sebastien said the payments dialog might use it. (+Dan/Evan) Eh?
If we're going to use it in multiple places than I'm fine with it in views. Can we have a factory method for creating it? On Sun, May 19, 2013 at 11:42 PM, <msw@chromium.org> wrote: > On 2013/05/20 04:28:28, benwells wrote: >> >> If it is structured like that it can be moved around easily, and could >> start > > in >> >> ui/app_list where it is used now (it's not used in chrome). > > > I'd prefer to see it in u/v/c/b/, but leave it up to you and Scott. > > >> I'm happy to make the structural changes above, should I do that as part >> of > > this >> >> patch or in a follow up? > > > I think it's better done now, in the initial implementation, than with > followups. > > https://codereview.chromium.org/14631022/
On 2013/05/20 21:31:29, msw wrote: > On 2013/05/20 06:42:43, msw wrote: > > I'd prefer to see it in u/v/c/b/, but leave it up to you and Scott. > +CC Xiyuan/Dan/Evan. This definitely belongs in /ui, not chrome/. > Xiyuan is hoping to use the blue button in the app launcher asap. > Sebastien said the payments dialog might use it. (+Dan/Evan) Eh? I think we do want it, yes, although the mocks are 404'ing for me right now so I can't double check.
Updated per msw's suggestions. I haven't added the factory constructor yet, should that be on LabelButton or Button? https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... File ui/views/controls/button/label_button.cc (left): https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... ui/views/controls/button/label_button.cc:230: const SkColor color = button_state_colors_[state()]; On 2013/05/18 20:07:24, msw wrote: > nit: restore this const Done. https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... File ui/views/controls/button/label_button_border.cc (right): https://codereview.chromium.org/14631022/diff/58002/ui/views/controls/button/... ui/views/controls/button/label_button_border.cc:133: // TODO(msw): re-enable animation is for blue buttons. It's disabled now due On 2013/05/18 20:07:24, msw wrote: > nit: capitalization and s/animation is/animations/ Done.
If you want to try the new mode for "lerping" between two images, try
SkLerpXfermode.
saveLayer
draw-first-image
draw-second-image + lerpxfermode
restore
LGTM with nits. I don't think you need a factory function for now. I'm hoping to simplify style init across all buttons soon. Thanks, Mike Reed! I'll tackle SkLerpXfermode separately for issue 239121. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... File ui/views/controls/button/blue_button.cc (right): https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.cc:43: button_border->SetPainter(false, Button::STATE_NORMAL, nit: i suppose you don't need to specify Button:: for the STATE_* enums. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.cc:45: button_border->SetPainter( nit: match the arg placement across all these calls, like this call here, or like my preference: button_border->SetPainter(bool, state Painter::Create... https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... File ui/views/controls/button/blue_button.h (right): https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:16: // A native themed class representing a blue button. native themed? I'm not sure what you mean.
https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... File ui/views/controls/button/blue_button.h (right): https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:8: #include <string> remove, unused. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:16: // A native themed class representing a blue button. On 2013/05/24 16:14:28, msw wrote: > native themed? I'm not sure what you mean. looks like copied-paste typo. Because it is asset based now. :) https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:21: explicit BlueButton(ButtonListener* listener, const string16& text); no explicit https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:21: explicit BlueButton(ButtonListener* listener, const string16& text); base::string16 https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:26: virtual const char* GetClassName() const OVERRIDE; does this needs to be protected?
sky: OWNERS review please. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... File ui/views/controls/button/blue_button.cc (right): https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.cc:43: button_border->SetPainter(false, Button::STATE_NORMAL, On 2013/05/24 16:14:28, msw wrote: > nit: i suppose you don't need to specify Button:: for the STATE_* enums. Done. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.cc:45: button_border->SetPainter( On 2013/05/24 16:14:28, msw wrote: > nit: match the arg placement across all these calls, like this call here, or > like my preference: > button_border->SetPainter(bool, state > Painter::Create... Done. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... File ui/views/controls/button/blue_button.h (right): https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:8: #include <string> On 2013/05/24 16:17:51, tfarina wrote: > remove, unused. Done. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:16: // A native themed class representing a blue button. On 2013/05/24 16:17:51, tfarina wrote: > On 2013/05/24 16:14:28, msw wrote: > > native themed? I'm not sure what you mean. > > looks like copied-paste typo. Because it is asset based now. :) Oops fixed. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:16: // A native themed class representing a blue button. On 2013/05/24 16:17:51, tfarina wrote: > On 2013/05/24 16:14:28, msw wrote: > > native themed? I'm not sure what you mean. > > looks like copied-paste typo. Because it is asset based now. :) Comment updated. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:21: explicit BlueButton(ButtonListener* listener, const string16& text); On 2013/05/24 16:17:51, tfarina wrote: > base::string16 Done. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:21: explicit BlueButton(ButtonListener* listener, const string16& text); On 2013/05/24 16:17:51, tfarina wrote: > no explicit Done. https://codereview.chromium.org/14631022/diff/136004/ui/views/controls/button... ui/views/controls/button/blue_button.h:26: virtual const char* GetClassName() const OVERRIDE; On 2013/05/24 16:17:51, tfarina wrote: > does this needs to be protected? Made private.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/benwells@chromium.org/14631022/165001
Message was sent while issue was closed.
Change committed as 203089 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
