Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1943093002:
[Mac][Material Design] Make Material Design the default. (Closed)
Description[Mac][Material Design] Make Material Design the default.
This makes Material Design the default/only UI on the Mac.
R=avi@chromium.org
BUG=608438
Committed: https://crrev.com/83b63627687009b2919c4eb18f40c9beff61077c
Cr-Commit-Position: refs/heads/master@{#392049}
Patch Set 1 #Patch Set 2 : Make the default by changing the flag setup. #Patch Set 3 : Revert to hard coding of setting. #Patch Set 4 : Disable Material Design controller tests. #
Total comments: 4
Patch Set 5 : Change how make Material the default. #Patch Set 6 : Rebase against ToT. #Messages
Total messages: 20 (11 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. BUG=608438 ========== to ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. BUG=608438 ==========
shrike@chromium.org changed reviewers: + tdanderson@chromium.org
tdanderson@ - I assume this is the correct (or at least a valid) way to flip the switch on MD on the Mac? PTAL
Hi, please see my comments below: https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... File ui/base/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... ui/base/material_design/material_design_controller.cc:40: SetMode(Mode::MATERIAL_NORMAL); From my reading of the code, it looks like OSX does define the compile-time flag ENABLE_TOPCHROME_MD (see src/build/config/ui.gni). So instead of calling SetMode() here, you should add an #elif case for OSX at the end of DefaultMode() to return MATERIAL_NORMAL. https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... File ui/base/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... ui/base/material_design/material_design_controller_unittest.cc:115: EXPECT_TRUE(MaterialDesignController::IsModeMaterial()); Since OSX does include ENABLE_TOPCHROME_MD I don't think any changes to this file are necessary.
Description was changed from ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. BUG=608438 ========== to ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. R=avi@chromium.org BUG=608438 ==========
shrike@chromium.org changed reviewers: + avi@chromium.org
shrike@chromium.org changed reviewers: - avi@chromium.org
https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... File ui/base/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... ui/base/material_design/material_design_controller.cc:40: SetMode(Mode::MATERIAL_NORMAL); On 2016/05/05 14:56:55, tdanderson wrote: > From my reading of the code, it looks like OSX does define the compile-time flag > ENABLE_TOPCHROME_MD (see src/build/config/ui.gni). So instead of calling > SetMode() here, you should add an #elif case for OSX at the end of DefaultMode() > to return MATERIAL_NORMAL. Thank you. I felt like I had seen per-platform #defines somewhere but they weren't where I had expected to find them. https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... File ui/base/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1943093002/diff/80001/ui/base/material_design... ui/base/material_design/material_design_controller_unittest.cc:115: EXPECT_TRUE(MaterialDesignController::IsModeMaterial()); On 2016/05/05 14:56:55, tdanderson wrote: > Since OSX does include ENABLE_TOPCHROME_MD I don't think any changes to this > file are necessary. Thank you. With your other suggested change the MD tests no longer fail, so I don't need to do this.
shrike@chromium.org changed reviewers: + avi@chromium.org
avi@ - I'm not quite ready to land this change (need to fix a couple more tests, at least), but hoping to get this approved so that I can land it when I'm ready. PTAL
patch set 5 lgtm
LGTM for when it's ready.
The CQ bit was checked by shrike@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdanderson@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/1943093002/#ps120001 (title: "Rebase against ToT.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943093002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943093002/120001
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. R=avi@chromium.org BUG=608438 ========== to ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. R=avi@chromium.org BUG=608438 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. R=avi@chromium.org BUG=608438 ========== to ========== [Mac][Material Design] Make Material Design the default. This makes Material Design the default/only UI on the Mac. R=avi@chromium.org BUG=608438 Committed: https://crrev.com/83b63627687009b2919c4eb18f40c9beff61077c Cr-Commit-Position: refs/heads/master@{#392049} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/83b63627687009b2919c4eb18f40c9beff61077c Cr-Commit-Position: refs/heads/master@{#392049} |
