Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2241293003:
[MD User Menu] Adjusted its background color (Closed)
Description[MD User Menu] Adjusted its background color
See first bug.
BUG=636073
BUG=615893
Committed: https://crrev.com/12350097b18cf5a7225a57138834ce9d62b741af
Cr-Commit-Position: refs/heads/master@{#414092}
Patch Set 1 #Patch Set 2 : native_theme change #
Total comments: 2
Patch Set 3 : Break in win #
Messages
Total messages: 38 (18 generated)
Description was changed from ========== [MD User Menu] Adjusted the background color of it See first bug. BUG=636073 BUG=615893 ========== to ========== [MD User Menu] Adjusted its background color See first bug. BUG=636073 BUG=615893 ==========
janeliulwq@google.com changed reviewers: + sky@chromium.org
janeliulwq@google.com changed required reviewers: + sky@chromium.org
Thanks!
LGTM
On 2016/08/15 23:51:25, sky wrote: > LGTM Actually, not LGTM. Instead of calling to aura directly can't the conditional be moved to common_theme? By that I mean common_theme can check if IsSecondaryUiMatrial and then call appropriately.
sky@chromium.org changed reviewers: + estade@chromium.org
it's unclear to me why you need to make any changes at all here. GetNativeTheme should return something that calls through to GetAuraColor and with top chrome md on (which it is, by default) that should return white.
janeliulwq@google.com changed reviewers: - sky@chromium.org
janeliulwq@google.com changed required reviewers: - sky@chromium.org
Alright, back to bug. I made the change so that native_theme_mac and native_theme_win returns GetAuraColor for DialogBackground when IsSecondaryUiMaterial. PTAL. Mind my lack of knowledge on this, but what's the plan for turning on SecondaryUiMaterial by default? As for Linux, for classic theme, the background color is correctly returned from GetAuraColor as #FFFFFF; it's just the GTK theme that's weird. What's the best way to handle this (or not)? Thanks!
https://codereview.chromium.org/2241293003/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2241293003/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:498: } I think you can just break in the md case, and let the other GetAuraColor calls handle it. That way you also get an inverted color as appropriate.
https://codereview.chromium.org/2241293003/diff/20001/ui/native_theme/native_... File ui/native_theme/native_theme_win.cc (right): https://codereview.chromium.org/2241293003/diff/20001/ui/native_theme/native_... ui/native_theme/native_theme_win.cc:498: } On 2016/08/23 19:31:05, Evan Stade wrote: > I think you can just break in the md case, and let the other GetAuraColor calls > handle it. That way you also get an inverted color as appropriate. Done.
lgtm
On 2016/08/23 20:10:15, Evan Stade wrote: > lgtm Just one more question: what's the plan for turning on SecondaryUiMaterial by default? Just so I can update the bug properly. Thanks!
The CQ bit was checked by janeliulwq@google.com 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: This issue passed the CQ dry run.
The CQ bit was checked by janeliulwq@google.com
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
A disapproval has been posted by following reviewers: sky@chromium.org. Please make sure to get positive review before another CQ attempt.
The CQ bit was checked by janeliulwq@google.com
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
A disapproval has been posted by following reviewers: sky@chromium.org. Please make sure to get positive review before another CQ attempt.
janeliulwq@google.com changed reviewers: + sky@chromium.org
Hi sky@, can't commit with your disapproval.
LGTM
The CQ bit was checked by janeliulwq@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [MD User Menu] Adjusted its background color See first bug. BUG=636073 BUG=615893 ========== to ========== [MD User Menu] Adjusted its background color See first bug. BUG=636073 BUG=615893 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [MD User Menu] Adjusted its background color See first bug. BUG=636073 BUG=615893 ========== to ========== [MD User Menu] Adjusted its background color See first bug. BUG=636073 BUG=615893 Committed: https://crrev.com/12350097b18cf5a7225a57138834ce9d62b741af Cr-Commit-Position: refs/heads/master@{#414092} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/12350097b18cf5a7225a57138834ce9d62b741af Cr-Commit-Position: refs/heads/master@{#414092}
Message was sent while issue was closed.
On 2016/08/23 20:12:10, Jane wrote: > On 2016/08/23 20:10:15, Evan Stade wrote: > > lgtm > > Just one more question: what's the plan for turning on SecondaryUiMaterial by > default? Just so I can update the bug properly. Thanks! we're targetting m55 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
