Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2880033003:
Apply MD style to Linux avatar buttons. (Closed)
|
Created:
3 years, 7 months ago by Evan Stade Modified:
3 years, 3 months ago CC:
chromium-reviews, tfarina, dcheng, bruthig+ink_drop_chromium.org, oshima+watch_chromium.org, emx Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionApply MD style to Linux profile switcher/avatar button.
This is limited to Linux because it draws the avatar button to match
Linux caption buttons, but it can be enabled for custom themed browser
windows on Windows as well after we write a drawing routine that looks
appropriate next to those buttons.
1. Draw the button without the use of assets (scale better at larger scale factors).
2. Improve vertical centering of text.
3. Use ink drop highlight/ripple for interactions.
4. Keep active state going while profile switcher widget is open (also
applies to Win10).
BUG=591586
Review-Url: https://codereview.chromium.org/2880033003
Cr-Commit-Position: refs/heads/master@{#472671}
Committed: https://chromium.googlesource.com/chromium/src/+/def869130c7984358d2a7f93a37f135cb34777d5
Patch Set 1 #Patch Set 2 : tweaks #Patch Set 3 : tweaks #
Total comments: 29
Patch Set 4 : pkasting review #Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : updates #Patch Set 7 : tweaks to improve inkdrop #Patch Set 8 : rebase #
Total comments: 1
Messages
Total messages: 38 (24 generated)
Description was changed from ========== Apply MD style to Linux avatar buttons. BUG=591586 ========== to ========== Apply MD style to Linux profile switcher/avatar button. This is limited to Linux because it draws the avatar button to match Linux caption buttons, but it can be enabled for custom themed browser windows on Windows as well after we write a drawing routine that looks appropriate next to those buttons. 1. Draw the button without the use of assets (scale better at larger scale factors). 2. Improve vertical centering of text. 3. Use ink drop highlight/ripple for interactions. 4. Keep active state going while profile switcher widget is open. BUG=591586 ==========
Description was changed from ========== Apply MD style to Linux profile switcher/avatar button. This is limited to Linux because it draws the avatar button to match Linux caption buttons, but it can be enabled for custom themed browser windows on Windows as well after we write a drawing routine that looks appropriate next to those buttons. 1. Draw the button without the use of assets (scale better at larger scale factors). 2. Improve vertical centering of text. 3. Use ink drop highlight/ripple for interactions. 4. Keep active state going while profile switcher widget is open. BUG=591586 ========== to ========== Apply MD style to Linux profile switcher/avatar button. This is limited to Linux because it draws the avatar button to match Linux caption buttons, but it can be enabled for custom themed browser windows on Windows as well after we write a drawing routine that looks appropriate next to those buttons. 1. Draw the button without the use of assets (scale better at larger scale factors). 2. Improve vertical centering of text. 3. Use ink drop highlight/ripple for interactions. 4. Keep active state going while profile switcher widget is open (also applies to Win10). BUG=591586 ==========
estade@chromium.org changed reviewers: + pkasting@chromium.org
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
LGTM with one substantive issue. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:76: stroke_flags.setColor(SkColorSetA(SK_ColorBLACK, 0x2B)); Nit: Where did this constant come from? Is it just observed as what GTK does? Same with 0x3F below. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:83: // There's a second, light stroke that matches the fill bounds. I'm still concerned with computing these nested (stroke, stroke, ink drop flood fill) curves by simply drawing inset round rects instead of subtracting the actual stroke paths so the contours match exactly. I think I wrote more extensively about this on your previous version of this. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:90: return kBorderStrokeInsets + gfx::Insets(0, 6); Nit: Again, where does this 6 come from? Can it be computed somehow? Does it relate to anything else in the UI code? https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:115: constexpr float AvatarButtonThemedBorder::kStrokeWidth; Nit: Does the compiler actually complain if you don't define these (i.e. they're considered ODR-used)? https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:226: size.set_height(20); Nit: Simpler: gfx::Size size(LabelButton::GetPreferredSize().width(), 20); https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:230: // tabstrip). Its height should match the caption button height. Nit: This second sentence should probably turn into a TODO above where |size| is computed, like: // TODO(emx): Compute height to match caption button height for other // platforms than Windows. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:232: // as per the spec in http://crbug.com/635699. Nit: This thirds sentence, if it needs saying at all, probably should be up where these two constants are defined. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:236: size.set_height(MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs()); Nit: This call shouldn't be inside IsCondensible(), I don't think, since the fact that the avatar button height should match that of the caption buttons doesn't depend on that (I think?). I'd probably pull this #if out to where the "20" is used above, like: // TODO... #if OS_WIN const int height = MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs(); #else constexpr int height = 20; #endif gfx::Size size(LabelButton::GetPreferredSize().width(), height); https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:246: return LabelButton::CreateInkDropMask(); Nit: Could have single return with ?:, up to you https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:361: #if defined(OS_WIN) Nit: Can we preserve some kind of TODO here about using this button on non-Windows platforms too, as the place you moved this from had? https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.h (right): https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.h:59: // views::WidgetObserver Nit: Trailing colon
https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:76: stroke_flags.setColor(SkColorSetA(SK_ColorBLACK, 0x2B)); On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: Where did this constant come from? Is it just observed as what GTK does? > Same with 0x3F below. They match the assets we use for the caption buttons on Linux (which are not in themselves part of GTK but hard coded). https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:83: // There's a second, light stroke that matches the fill bounds. On 2017/05/15 21:51:27, Peter Kasting wrote: > I'm still concerned with computing these nested (stroke, stroke, ink drop flood > fill) curves by simply drawing inset round rects instead of subtracting the > actual stroke paths so the contours match exactly. I think I wrote more > extensively about this on your previous version of this. Yea. Before you said you were worried about appearance at larger scale factors; I tested at multiple DSFs up to 4x and it looked good. I don't know if there's a way to do as you suggest for the ink drop. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:90: return kBorderStrokeInsets + gfx::Insets(0, 6); On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: Again, where does this 6 come from? Can it be computed somehow? Does it > relate to anything else in the UI code? I don't believe so. It was hardcoded before and it continues to be hardcoded. It controls the amount of spacing between button text (or image) and border. Do you have a suggestion for how to compute that? I could see it being some fraction of the font size or something, but that fraction itself is a constant. I also forgot to ask: have you checked the appearance of the default avatar icon on Windows? I think it may be clipped at the top and/or bottom due to too much border inset. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:115: constexpr float AvatarButtonThemedBorder::kStrokeWidth; On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: Does the compiler actually complain if you don't define these (i.e. they're > considered ODR-used)? The code compiles but won't link. There is some discussion at [1] [1] http://stackoverflow.com/questions/8016780/undefined-reference-to-static-cons... https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:226: size.set_height(20); On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: Simpler: > > gfx::Size size(LabelButton::GetPreferredSize().width(), 20); Done. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:230: // tabstrip). Its height should match the caption button height. On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: This second sentence should probably turn into a TODO above where |size| is > computed, like: > > // TODO(emx): Compute height to match caption button height for other > // platforms than Windows. todo added https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:232: // as per the spec in http://crbug.com/635699. On 2017/05/15 21:51:28, Peter Kasting wrote: > Nit: This thirds sentence, if it needs saying at all, probably should be up > where these two constants are defined. removed https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:236: size.set_height(MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs()); On 2017/05/15 21:51:28, Peter Kasting wrote: > Nit: This call shouldn't be inside IsCondensible(), I don't think, since the > fact that the avatar button height should match that of the caption buttons > doesn't depend on that (I think?). > > I'd probably pull this #if out to where the "20" is used above, like: > > // TODO... > #if OS_WIN > const int height = MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs(); > #else > constexpr int height = 20; > #endif > gfx::Size size(LabelButton::GetPreferredSize().width(), height); I agree with this sentiment but changing that might create a behavioral difference on themed Windows or versions <10, which might not be desirable. I'd be worried to change that without having a way to test it. Seems like a task for whoever takes up the TODO for programmatic drawing of this button for themed Windows browsers. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:246: return LabelButton::CreateInkDropMask(); On 2017/05/15 21:51:28, Peter Kasting wrote: > Nit: Could have single return with ?:, up to you Acknowledged. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:361: #if defined(OS_WIN) On 2017/05/15 21:51:27, Peter Kasting wrote: > Nit: Can we preserve some kind of TODO here about using this button on > non-Windows platforms too, as the place you moved this from had? I considered that TODO to refer to the ink drop effects, not the condensibility. I tried to get rid of references to MD in this patch as it's kind of ambiguous here.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
estade@chromium.org changed reviewers: + bruthig@chromium.org, oshima@chromium.org
+bruthig for ui/views/animation +oshima for chrome/app/theme/theme_resources.grd
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:76: stroke_flags.setColor(SkColorSetA(SK_ColorBLACK, 0x2B)); On 2017/05/15 22:25:16, Evan Stade wrote: > On 2017/05/15 21:51:27, Peter Kasting wrote: > > Nit: Where did this constant come from? Is it just observed as what GTK does? > > > Same with 0x3F below. > > They match the assets we use for the caption buttons on Linux (which are not in > themselves part of GTK but hard coded). Maybe we should put a comment in saying "matches xyz, change this if you change those"? Maybe not worth it if you're hoping to do the work of expanding this beyond just Linux soon. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:83: // There's a second, light stroke that matches the fill bounds. On 2017/05/15 22:25:15, Evan Stade wrote: > On 2017/05/15 21:51:27, Peter Kasting wrote: > > I'm still concerned with computing these nested (stroke, stroke, ink drop > flood > > fill) curves by simply drawing inset round rects instead of subtracting the > > actual stroke paths so the contours match exactly. I think I wrote more > > extensively about this on your previous version of this. > > Yea. Before you said you were worried about appearance at larger scale factors; > I tested at multiple DSFs up to 4x and it looked good. Interesting. I recall noticing similar artifacts back in the day on the omnibox 1 px border but maybe I was testing it locally on a really crazy scale factor. It's theoretically more correct to do it the other way and doesn't seem too hard, but it'd be so hard to notice that it's not worth me pushing for it unless you're OCD like me and it would make you feel happier in principle :) > I don't know if there's a > way to do as you suggest for the ink drop. There probably is, but probably the easier route would be to assume a round rect for the ink drop and carve it out of the interior of the stroke around it. Basically, start with the ink drop bounds as a filled round rect, expand the round rect bounds out by 1 on each side twice, then turn those two expanded rects into rings by subtracting the smaller rects. The result would be rings whose outer perimeters match true round rects, but whose inner perimeters are different. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:90: return kBorderStrokeInsets + gfx::Insets(0, 6); On 2017/05/15 22:25:16, Evan Stade wrote: > On 2017/05/15 21:51:27, Peter Kasting wrote: > > Nit: Again, where does this 6 come from? Can it be computed somehow? Does it > > relate to anything else in the UI code? > > I don't believe so. It was hardcoded before and it continues to be hardcoded. It > controls the amount of spacing between button text (or image) and border. Do you > have a suggestion for how to compute that? I could see it being some fraction of > the font size or something, but that fraction itself is a constant. I'm thinking the spacing should match what we compute for labels-in-buttons elsewhere in the UI, however that's done. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:115: constexpr float AvatarButtonThemedBorder::kStrokeWidth; On 2017/05/15 22:25:15, Evan Stade wrote: > On 2017/05/15 21:51:27, Peter Kasting wrote: > > Nit: Does the compiler actually complain if you don't define these (i.e. > they're > > considered ODR-used)? > > The code compiles but won't link. Yeah, by "compiler" I really meant "linker". Leave them, they're correct. https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:361: #if defined(OS_WIN) On 2017/05/15 22:25:16, Evan Stade wrote: > On 2017/05/15 21:51:27, Peter Kasting wrote: > > Nit: Can we preserve some kind of TODO here about using this button on > > non-Windows platforms too, as the place you moved this from had? > > I considered that TODO to refer to the ink drop effects, not the condensibility. > I tried to get rid of references to MD in this patch as it's kind of ambiguous > here. OK. For reference, we want the condensibility on all platforms.
The CQ bit was checked by estade@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
c/a/t lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm with nit https://codereview.chromium.org/2880033003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:267: ProfileChooserView::GetCurrentBubbleWidget()->AddObserver(this); nit: It is ok to call AnimaterInkDrop() even when the mode is OFF, so the only real benefit I see here is to not add an observer. Consider not special casing this or consider adding and using a 'bool InkDropHostView::IsInkDropEnabled() const" method instead if the ink drop should be animated for the InkDropMode::ON_NO_GESTURE_HANDLER value.
Ben, I made a couple tweaks to make the inkdrop behave better (mainly so that if you click on the button while the bubble is open, thus closing the bubble, the ink drop won't activate again). Could you ptal? https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:76: stroke_flags.setColor(SkColorSetA(SK_ColorBLACK, 0x2B)); On 2017/05/15 23:41:02, Peter Kasting wrote: > On 2017/05/15 22:25:16, Evan Stade wrote: > > On 2017/05/15 21:51:27, Peter Kasting wrote: > > > Nit: Where did this constant come from? Is it just observed as what GTK > does? > > > > > Same with 0x3F below. > > > > They match the assets we use for the caption buttons on Linux (which are not > in > > themselves part of GTK but hard coded). > > Maybe we should put a comment in saying "matches xyz, change this if you change > those"? Maybe not worth it if you're hoping to do the work of expanding this > beyond just Linux soon. comment added https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:90: return kBorderStrokeInsets + gfx::Insets(0, 6); On 2017/05/15 23:41:02, Peter Kasting wrote: > On 2017/05/15 22:25:16, Evan Stade wrote: > > On 2017/05/15 21:51:27, Peter Kasting wrote: > > > Nit: Again, where does this 6 come from? Can it be computed somehow? Does > it > > > relate to anything else in the UI code? > > > > I don't believe so. It was hardcoded before and it continues to be hardcoded. > It > > controls the amount of spacing between button text (or image) and border. Do > you > > have a suggestion for how to compute that? I could see it being some fraction > of > > the font size or something, but that fraction itself is a constant. > > I'm thinking the spacing should match what we compute for labels-in-buttons > elsewhere in the UI, however that's done. I guess we're sorta matching this[1]. Changed. [1] https://cs.chromium.org/chromium/src/ui/views/controls/button/label_button_bo... https://codereview.chromium.org/2880033003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:361: #if defined(OS_WIN) On 2017/05/15 23:41:02, Peter Kasting wrote: > On 2017/05/15 22:25:16, Evan Stade wrote: > > On 2017/05/15 21:51:27, Peter Kasting wrote: > > > Nit: Can we preserve some kind of TODO here about using this button on > > > non-Windows platforms too, as the place you moved this from had? > > > > I considered that TODO to refer to the ink drop effects, not the > condensibility. > > I tried to get rid of references to MD in this patch as it's kind of ambiguous > > here. > > OK. For reference, we want the condensibility on all platforms. added comment https://codereview.chromium.org/2880033003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2880033003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/avatar_button.cc:267: ProfileChooserView::GetCurrentBubbleWidget()->AddObserver(this); On 2017/05/16 17:33:48, bruthig wrote: > nit: It is ok to call AnimaterInkDrop() even when the mode is OFF, so the only > real benefit I see here is to not add an observer. Consider not special casing > this or consider adding and using a 'bool InkDropHostView::IsInkDropEnabled() > const" method instead if the ink drop should be animated for the > InkDropMode::ON_NO_GESTURE_HANDLER value. Done.
slgtm
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, oshima@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2880033003/#ps120001 (title: "tweaks to improve inkdrop")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, oshima@chromium.org, bruthig@chromium.org Link to the patchset: https://codereview.chromium.org/2880033003/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495058766053090,
"parent_rev": "39e3f9e3bdd92088ebf9c48bfd9395ebe38d7801", "commit_rev":
"def869130c7984358d2a7f93a37f135cb34777d5"}
Message was sent while issue was closed.
Description was changed from ========== Apply MD style to Linux profile switcher/avatar button. This is limited to Linux because it draws the avatar button to match Linux caption buttons, but it can be enabled for custom themed browser windows on Windows as well after we write a drawing routine that looks appropriate next to those buttons. 1. Draw the button without the use of assets (scale better at larger scale factors). 2. Improve vertical centering of text. 3. Use ink drop highlight/ripple for interactions. 4. Keep active state going while profile switcher widget is open (also applies to Win10). BUG=591586 ========== to ========== Apply MD style to Linux profile switcher/avatar button. This is limited to Linux because it draws the avatar button to match Linux caption buttons, but it can be enabled for custom themed browser windows on Windows as well after we write a drawing routine that looks appropriate next to those buttons. 1. Draw the button without the use of assets (scale better at larger scale factors). 2. Improve vertical centering of text. 3. Use ink drop highlight/ripple for interactions. 4. Keep active state going while profile switcher widget is open (also applies to Win10). BUG=591586 Review-Url: https://codereview.chromium.org/2880033003 Cr-Commit-Position: refs/heads/master@{#472671} Committed: https://chromium.googlesource.com/chromium/src/+/def869130c7984358d2a7f93a37f... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/def869130c7984358d2a7f93a37f...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2880033003/diff/140001/chrome/app/theme/theme... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/2880033003/diff/140001/chrome/app/theme/theme... chrome/app/theme/theme_resources.grd:336: <if expr="is_win"> This doesn't match the C++ code. The IDR_AVATAR_THEMED usage in avatar_button.cc is not inside a #if defined(OS_WIN) conditional. This only works because output_all_resource_defines defaults to true and output_all_resource_defines="false" isn't set. See bug 627301. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
