Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1921133002:
Adds ash::MaterialDesignController (Closed)
DescriptionAdds ash::MaterialDesignController and a run-time flag
Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable
Material Design in Chrome OS system UI.
ash::MaterialDesignController::IsMaterial() can be used to query the
mode at run-time.
BUG=605644
Committed: https://crrev.com/b6a59711024115cb14728ca0dfdc2a1d507fa567
Cr-Commit-Position: refs/heads/master@{#390260}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Adds ash::MaterialDesignController (comments) #
Total comments: 2
Patch Set 3 : Adds ash::MaterialDesignController (doc) #
Total comments: 9
Patch Set 4 : Adds ash::MaterialDesignController (multi-value) #
Total comments: 4
Patch Set 5 : Adds ash::MaterialDesignController (AshTestBase initialization) #Patch Set 6 : Adds ash::MaterialDesignController (unit_tests) #
Total comments: 8
Patch Set 7 : Adds ash::MaterialDesignController (comments) #
Total comments: 6
Patch Set 8 : Adds ash::MaterialDesignController (order of args to CHECK_EQ) #Patch Set 9 : Adds ash::MaterialDesignController (CHECK_* -> DCHECK_*) #
Messages
Total messages: 68 (27 generated)
The CQ bit was checked by varkha@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/1921133002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/1
varkha@chromium.org changed reviewers: + oshima@chromium.org
oshima@, can you please take a first look? This is modeled on ui/base/material_design_controller but simpler and I didn't want to necessarily tie the lifetime of those sets of flags together (top chrome MD vs. ash). Thanks!
https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 same for other files. https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:34: static bool mode_; move private static to anonymous namespace in .cc https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller_unittest.cc:113: } // namespace Just define test bodies in ash namespace. Rationale is that you can avoid having two tests having the same name this way.
https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/04/25 23:01:43, oshima wrote: > 2016 > > same for other files. Done. https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:34: static bool mode_; On 2016/04/25 23:01:43, oshima wrote: > move private static to anonymous namespace in .cc > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts It is used by a friend MaterialDesignControllerTestAPI class. https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller_unittest.cc:113: } // namespace On 2016/04/25 23:01:43, oshima wrote: > Just define test bodies in ash namespace. Rationale is that you can avoid having > two tests having the same name this way. Done.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by varkha@chromium.org to run a CQ dry run
varkha@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine@ for OWNERS in tools/metrics/histograms/histograms.xml
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921133002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.cc:26: switches::kAshEnableMaterialDesign)) { By the way, I personally prefer not to have two flags because you can't tell which one has priority. (--ash-disable-material-design --ash-enable-material-design disable the mode, but you can't tell unless you read the code) Alternative way is to have a boolean flag, true by default --ash-enable-material-design (implies=true) --ash-enable-material-design=false You don't have to change. Just an idea you may want to consider next time. https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:34: static bool mode_; On 2016/04/25 23:16:05, varkha wrote: > On 2016/04/25 23:01:43, oshima wrote: > > move private static to anonymous namespace in .cc > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > It is used by a friend MaterialDesignControllerTestAPI class. In that case, how about defiing enum enum class Mode { NONE, // or UNINITIALIZED MATERIAL, CLASSIC } Then all you need is SetMode and DefualtMode
bruthig@chromium.org changed reviewers: + bruthig@chromium.org
Drive by....vroooom https://codereview.chromium.org/1921133002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1921133002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:870: ENABLE_DISABLE_VALUE_TYPE(ash::switches::kAshEnableMaterialDesign, Suggestion, I've found it easier to use a tri-state flag with a 'Default' over the binary ENABLE_DISABLE_VALUE_TYPE.
https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.cc:26: switches::kAshEnableMaterialDesign)) { On 2016/04/26 05:56:37, oshima wrote: > By the way, I personally prefer not to have two flags because you can't tell > which one has priority. > (--ash-disable-material-design --ash-enable-material-design disable the mode, > but you can't tell unless you read the code) > > Alternative way is to have a boolean flag, true by default > > --ash-enable-material-design (implies=true) > --ash-enable-material-design=false > > You don't have to change. Just an idea you may want to consider next time. oshima@, how strongly do you feel about it? I can see pros and cons for all approaches and I've tried to carefully consider them. - simple value (enabeld / disabled): lacks "default" and so needs code changes when we change what that default maps to impossible to change to default in about://flags - special value (like top-chrome-md) this works well and would be my second choice has custom values for what should be boolean is flexible (but more so than what we need here) - tri-state default / enabled / disabled (such as in this CL) works well for our case when wrapped in a controller class is safe to use regarding your priority concern maps to 2 separate flags (this is unfortunate but we support it this way for many other flags) >> but you can't tell unless you read the code Readability is a real concern. Do you think documenting this precedence in a header file would alleviate it? https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:34: static bool mode_; On 2016/04/26 05:56:37, oshima wrote: > On 2016/04/25 23:16:05, varkha wrote: > > On 2016/04/25 23:01:43, oshima wrote: > > > move private static to anonymous namespace in .cc > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > It is used by a friend MaterialDesignControllerTestAPI class. > > In that case, how about defiing enum > > enum class Mode { > NONE, // or UNINITIALIZED > MATERIAL, > CLASSIC > } > > Then all you need is SetMode and DefualtMode > I think this doesn't do what I want to accomplish: - explicit mandatory initialization (no legal default value) - the only external interface is IsMaterial() Basically I wanted the client code to only ever worry about true or false and hide the rest in md_controller. Of course it is still possible with a tri-state enum but looks more artificial to me and the header would still have that enum. https://codereview.chromium.org/1921133002/diff/40001/chrome/browser/about_fl... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1921133002/diff/40001/chrome/browser/about_fl... chrome/browser/about_flags.cc:870: ENABLE_DISABLE_VALUE_TYPE(ash::switches::kAshEnableMaterialDesign, On 2016/04/26 14:00:48, bruthig wrote: > Suggestion, I've found it easier to use a tri-state flag with a 'Default' over > the binary ENABLE_DISABLE_VALUE_TYPE. ENABLE_DISABLE_VALUE_TYPE is already tri-state (default / enabled / disabled). Your concern is exactly why I did it this way - to allow switching what our default is without changing too many files - that would be just a change in the md_controller.cc
https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc#new... ash/ash_switches.cc:63: const char kAshDisableMaterialDesign[] = "ash-disable-md"; Have you considered using base/feature_list.h instead? It provides the same ability to have an entry in about:flags as well as allows to later enable this via a server-side experiment without any additional code.
https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc File ash/ash_switches.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc#new... ash/ash_switches.cc:63: const char kAshDisableMaterialDesign[] = "ash-disable-md"; On 2016/04/26 15:12:39, Alexei Svitkine wrote: > Have you considered using base/feature_list.h instead? It provides the same > ability to have an entry in about:flags as well as allows to later enable this > via a server-side experiment without any additional code. Thanks, I will take a look, didn't know this was a thing. I don't see it ever used in Chrome OS core UI, probably just lack of awareness. oshima@, do you have a feel for this one way or the other? I've used a regular flag mostly because this is how we did "top-chrome-md" flag.
https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.cc:26: switches::kAshEnableMaterialDesign)) { On 2016/04/26 15:06:02, varkha wrote: > On 2016/04/26 05:56:37, oshima wrote: > > By the way, I personally prefer not to have two flags because you can't tell > > which one has priority. > > (--ash-disable-material-design --ash-enable-material-design disable the mode, > > but you can't tell unless you read the code) > > > > Alternative way is to have a boolean flag, true by default > > > > --ash-enable-material-design (implies=true) > > --ash-enable-material-design=false > > > > You don't have to change. Just an idea you may want to consider next time. > > oshima@, how strongly do you feel about it? I can see pros and cons for all > approaches and I've tried to carefully consider them. I don't have strong feeling about this. This is just FYI. > - simple value (enabeld / disabled): > lacks "default" and so needs code changes when we change what that default > maps to > impossible to change to default in about://flags You can. See my example of use-zoom-for-dsf. > - special value (like top-chrome-md) > this works well and would be my second choice > has custom values for what should be boolean > is flexible (but more so than what we need here) > - tri-state default / enabled / disabled (such as in this CL) > works well for our case > when wrapped in a controller class is safe to use regarding your priority > concern > maps to 2 separate flags (this is unfortunate but we support it this way for > many other flags) > > >> but you can't tell unless you read the code > Readability is a real concern. Do you think documenting this precedence in a > header file would alleviate it? I'm not worry much about it assuming that this is very short lived. https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/1/ash/material_design/materia... ash/material_design/material_design_controller.h:34: static bool mode_; On 2016/04/26 15:06:02, varkha wrote: > On 2016/04/26 05:56:37, oshima wrote: > > On 2016/04/25 23:16:05, varkha wrote: > > > On 2016/04/25 23:01:43, oshima wrote: > > > > move private static to anonymous namespace in .cc > > > > > > > > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts > > > > > > It is used by a friend MaterialDesignControllerTestAPI class. > > > > In that case, how about defiing enum > > > > enum class Mode { > > NONE, // or UNINITIALIZED > > MATERIAL, > > CLASSIC > > } > > > > Then all you need is SetMode and DefualtMode > > > > I think this doesn't do what I want to accomplish: > - explicit mandatory initialization (no legal default value) mode_(NONE) will do it. > - the only external interface is IsMaterial() This will stay the same. My key point is that a state machine like system is better than multiple booleans. It's easier to understand and less error prone. Here is an example: public: bool IsMaterial() { CHECK_NE(mode_, NONE); return mode_ == MATERIAL; } private: enum class Mode { } void Initizlize() { CHECK_EQ(mode_, NONE); mode_ = DefaultMode(); } void SetMode(Mode mode) { mode_ = mode; } to uninitialize it SetMode(NONE); This can be different depending on how much strict you want to do. For example, it may be better to set "default mode", then let Initialize initialize with it. That way, you can make test code close to production code. > Basically I wanted the client code to only ever worry about true or false and > hide the rest in md_controller. Of course it is still possible with a tri-state > enum but looks more artificial to me and the header would still have that enum. https://codereview.chromium.org/1921133002/diff/60001/ash/material_design/mat... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/material_design/mat... ash/material_design/material_design_controller_unittest.cc:51: switches::kAshEnableMaterialDesign); These additional flags will be carried over to other tests. You should explicitly clear them, or wait for https://codereview.chromium.org/1908633002/ Is there a reason to do this in ctor, not in SetUp()? https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... File ash/test/material_design_controller_test_api.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... ash/test/material_design_controller_test_api.cc:18: MaterialDesignController::mode_ = previous_mode_; Are you going to allow a test to change mode in the middle? It can allow two UI mode mixed, which may results in unexpected results.
On 2016/04/26 15:23:00, varkha wrote: > https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc > File ash/ash_switches.cc (right): > > https://codereview.chromium.org/1921133002/diff/60001/ash/ash_switches.cc#new... > ash/ash_switches.cc:63: const char kAshDisableMaterialDesign[] = > "ash-disable-md"; > On 2016/04/26 15:12:39, Alexei Svitkine wrote: > > Have you considered using base/feature_list.h instead? It provides the same > > ability to have an entry in about:flags as well as allows to later enable this > > via a server-side experiment without any additional code. > > Thanks, I will take a look, didn't know this was a thing. I don't see it ever > used in Chrome OS core UI, probably just lack of awareness. > oshima@, do you have a feel for this one way or the other? I've used a regular > flag mostly because this is how we did "top-chrome-md" flag. It looks relatively new thing. I'm fine with either way.
I will try to re-work it with a feature. Also I hear that I will need more than one enabled state - one for more stable set of features that is enabled by default sooner and one for longer term work that is disabled for most users. Stay tuned...
Description was changed from ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ========== to ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ==========
PTAL. I've added a multi-value switch. It seems like feature_list is (a) limited to enabled / disabled (and the last I've heard we will need to be able to launch incrementally) and (b) we are not really planning on experimenting with on/off switch - it is more of a tool to allow development of stage 2 while having stage 1 deployed by default, plus a kill switch.
The CQ bit was checked by varkha@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/1921133002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... ash/material_design/material_design_controller.h:36: // Maps to "ash-enable-md" and "ash-diable-md flags" pair of flags. Is this doc up to date? sp/ash-diable-md->ash-disable-md It would be worth noting that IsMaterial() will also return true if IsMaterialExperimental() returns true. https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... ash/material_design/material_design_controller.h:40: // nit: empty doc?
rubberstamp lgtm for histograms
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/1921133002/diff/60001/ash/material_design/mat... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/material_design/mat... ash/material_design/material_design_controller_unittest.cc:51: switches::kAshEnableMaterialDesign); On 2016/04/26 16:06:58, oshima wrote: > These additional flags will be carried over to other tests. You should > explicitly clear them, or wait for https://codereview.chromium.org/1908633002/ > > Is there a reason to do this in ctor, not in SetUp()? Done. https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... File ash/test/material_design_controller_test_api.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... ash/test/material_design_controller_test_api.cc:18: MaterialDesignController::mode_ = previous_mode_; On 2016/04/26 16:06:58, oshima wrote: > Are you going to allow a test to change mode in the middle? > It can allow two UI mode mixed, which may results in unexpected results. This can be used by tests that want to set and restore mode, e.g. when a test wants to test with multiple flag values. There are tests like that for ui::base::MaterialDesignController (the top-chrome variant) but not yet for this ash variant. https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... File ash/material_design/material_design_controller.h (right): https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... ash/material_design/material_design_controller.h:36: // Maps to "ash-enable-md" and "ash-diable-md flags" pair of flags. On 2016/04/27 19:57:36, bruthig wrote: > Is this doc up to date? > > sp/ash-diable-md->ash-disable-md > > > It would be worth noting that IsMaterial() will also return true if > IsMaterialExperimental() returns true. Done. https://codereview.chromium.org/1921133002/diff/80001/ash/material_design/mat... ash/material_design/material_design_controller.h:40: // On 2016/04/27 19:57:36, bruthig wrote: > nit: empty doc? Done.
The CQ bit was checked by varkha@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/1921133002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by varkha@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/1921133002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/140001
https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... File ash/test/material_design_controller_test_api.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... ash/test/material_design_controller_test_api.cc:18: MaterialDesignController::mode_ = previous_mode_; On 2016/04/27 21:00:09, varkha wrote: > On 2016/04/26 16:06:58, oshima wrote: > > Are you going to allow a test to change mode in the middle? > > It can allow two UI mode mixed, which may results in unexpected results. > > This can be used by tests that want to set and restore mode, e.g. when a test > wants to test with multiple flag values. There are tests like that for > ui::base::MaterialDesignController (the top-chrome variant) but not yet for this > ash variant. Since the flag will never change during runtime, I'd usually recommend to separate such test into multiple tests, but I don't have strong opinion, so you may keep it if you prefer. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller.cc:16: MaterialDesignController::Mode g_mode = mode_ per https://www.chromium.org/developers/coding-style/cpp-dos-and-donts https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:20: // TODO(varkha): replace with base/test/scoped_command_line.h when it becomes According to the comment in that CL, my understanding was obsolete and unit tests framework now resets command line flag upon exit, so you no longer need this. sorry for false alarm. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:43: ~MaterialDesignControllerTest() override; nit: or just = default for ctor/dtor. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:63: } not necessary?
The CQ bit was checked by varkha@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/1921133002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/160001
https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... File ash/test/material_design_controller_test_api.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... ash/test/material_design_controller_test_api.cc:18: MaterialDesignController::mode_ = previous_mode_; On 2016/04/27 21:51:04, oshima wrote: > On 2016/04/27 21:00:09, varkha wrote: > > On 2016/04/26 16:06:58, oshima wrote: > > > Are you going to allow a test to change mode in the middle? > > > It can allow two UI mode mixed, which may results in unexpected results. > > > > This can be used by tests that want to set and restore mode, e.g. when a test > > wants to test with multiple flag values. There are tests like that for > > ui::base::MaterialDesignController (the top-chrome variant) but not yet for > this > > ash variant. > > Since the flag will never change during runtime, I'd usually recommend to > separate such test into multiple tests, > but I don't have strong opinion, so you may keep it if you prefer. This is more for having pre-existing tests that we would want to run with or without MD flags to make sure both cases are not broken. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller.cc:16: MaterialDesignController::Mode g_mode = On 2016/04/27 21:51:04, oshima wrote: > mode_ > > per https://www.chromium.org/developers/coding-style/cpp-dos-and-donts Done. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... File ash/material_design/material_design_controller_unittest.cc (right): https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:20: // TODO(varkha): replace with base/test/scoped_command_line.h when it becomes On 2016/04/27 21:51:04, oshima wrote: > According to the comment in that CL, my understanding was obsolete and unit > tests framework now resets command line flag upon exit, so you no longer need > this. > sorry for false alarm. Right, thanks! https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:43: ~MaterialDesignControllerTest() override; On 2016/04/27 21:51:04, oshima wrote: > nit: or just > > = default > > for ctor/dtor. Done. https://codereview.chromium.org/1921133002/diff/140001/ash/material_design/ma... ash/material_design/material_design_controller_unittest.cc:63: } On 2016/04/27 21:51:04, oshima wrote: > not necessary? Done. Right, old scaffolding.
The CQ bit was unchecked by varkha@chromium.org
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by varkha@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/1921133002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/180001
https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... File ash/test/material_design_controller_test_api.cc (right): https://codereview.chromium.org/1921133002/diff/60001/ash/test/material_desig... ash/test/material_design_controller_test_api.cc:18: MaterialDesignController::mode_ = previous_mode_; On 2016/04/27 22:14:00, varkha wrote: > On 2016/04/27 21:51:04, oshima wrote: > > On 2016/04/27 21:00:09, varkha wrote: > > > On 2016/04/26 16:06:58, oshima wrote: > > > > Are you going to allow a test to change mode in the middle? > > > > It can allow two UI mode mixed, which may results in unexpected results. > > > > > > This can be used by tests that want to set and restore mode, e.g. when a > test > > > wants to test with multiple flag values. There are tests like that for > > > ui::base::MaterialDesignController (the top-chrome variant) but not yet for > > this > > > ash variant. > > > > Since the flag will never change during runtime, I'd usually recommend to > > separate such test into multiple tests, > > but I don't have strong opinion, so you may keep it if you prefer. > > This is more for having pre-existing tests that we would want to run with or > without MD flags to make sure both cases are not broken. e.g. here - https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/animation...
lgtm with nits https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:23: CHECK_EQ(Mode::UNINITIALIZED, mode_); nit: DCHECK_EQ (sorry if I typed this) https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:52: CHECK_NE(Mode::UNINITIALIZED, mode_); nit: ditto.
varkha@chromium.org changed reviewers: + sky@chromium.org
+sky for OWNERS in chrome/browser/chrome_browser_main.cc https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:23: CHECK_EQ(Mode::UNINITIALIZED, mode_); On 2016/04/27 22:26:22, oshima wrote: > nit: DCHECK_EQ (sorry if I typed this) I actually wanted this to assert in release builds so we never have this promise broken. This is how it is for the top-chrome-md flag as well. https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:52: CHECK_NE(Mode::UNINITIALIZED, mode_); On 2016/04/27 22:26:22, oshima wrote: > nit: ditto. Acknowledged.
The CQ bit was checked by varkha@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/1921133002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/200001
https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:23: CHECK_EQ(Mode::UNINITIALIZED, mode_); On 2016/04/27 22:30:05, varkha wrote: > On 2016/04/27 22:26:22, oshima wrote: > > nit: DCHECK_EQ (sorry if I typed this) > > I actually wanted this to assert in release builds so we never have this promise > broken. This is how it is for the top-chrome-md flag as well. CHECK is for security/privacy issue, or you need data from fiels. If this ever fails, then a user won't be able to start chrome, which is really bad. Bots are compiled with DCHECK_ALWAYS_ON, so you still should be able catch on these builds.
https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... File ash/material_design/material_design_controller.cc (right): https://codereview.chromium.org/1921133002/diff/180001/ash/material_design/ma... ash/material_design/material_design_controller.cc:23: CHECK_EQ(Mode::UNINITIALIZED, mode_); On 2016/04/27 22:58:44, oshima wrote: > On 2016/04/27 22:30:05, varkha wrote: > > On 2016/04/27 22:26:22, oshima wrote: > > > nit: DCHECK_EQ (sorry if I typed this) > > > > I actually wanted this to assert in release builds so we never have this > promise > > broken. This is how it is for the top-chrome-md flag as well. > > CHECK is for security/privacy issue, or you need data from fiels. If this ever > fails, then a user won't be able to start chrome, which is really bad. > > Bots are compiled with DCHECK_ALWAYS_ON, so you still should be able catch on > these builds. Done.
lgtm thanks!
The CQ bit was checked by varkha@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/1921133002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/220001
LGTM
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 varkha@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1921133002/#ps220001 (title: "Adds ash::MaterialDesignController (CHECK_* -> DCHECK_*)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1921133002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1921133002/220001
Message was sent while issue was closed.
Description was changed from ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ========== to ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ========== to ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-md" and "ash-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b6a59711024115cb14728ca0dfdc2a1d507fa567 Cr-Commit-Position: refs/heads/master@{#390260}
Message was sent while issue was closed.
Description was changed from ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-md" and "ash-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 ========== to ========== Adds ash::MaterialDesignController and a run-time flag Adds "ash-enable-md" and "ash-disable-md" flags to enable / disable Material Design in Chrome OS system UI. ash::MaterialDesignController::IsMaterial() can be used to query the mode at run-time. BUG=605644 Committed: https://crrev.com/b6a59711024115cb14728ca0dfdc2a1d507fa567 Cr-Commit-Position: refs/heads/master@{#390260} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
