Make sure --enable-features=Mash takes precedence over --enable-features=Mus |
|||
Issue descriptionI'm close to enabling the Window Service feature (I actually landed it last night, but had to revert). When I do land this it'll be the default on trunk. Because it's the default I suspect to enable Mash means you'll need to do: --enable-features=Mash --disable-features=Mus. Ick! I think we should change the code such that Mash takes precedence. For places only looking as Mus, they should change to Mus && !Mash.
,
Mar 13 2018
The recommend way to force variations without specifying command-line flags manually is to add an entry to about:flags that can be selected through the UI. Otherwise, you'd have to keep resetting local state/user data dir until you get dice-rolled into it...
,
Mar 13 2018
If you apply https://chromium-review.googlesource.com/c/chromium/src/+/932821 (or sink to tip-of-tree) Mus is now on by default. That means if you now do --enable-features=Mash you get both Mus and Mash defined. I'm not sure that we consistently check for Mus/Mash. To Alex's question. I'm more worried about developers on the team (including myself) being able to solely specify --enable-features=Mash and get Mash.
,
Mar 13 2018
So, that CL adding the field trial also enables Mus by default for everyone?
I applied that patch and ran "chrome --enable-features=Mash", and it seems to run Mash as intended:
1) base::FieldTrialList::IsTrialActive("WindowService-ChromeOS") is true
2) Shell::GetAshConfig() is Config::MASH
Is there something I'm missing that you'd like me to verify?
Would you like me to trace the codepaths setting WindowManagerService::ash_config_ and constructing ShellPortMus/Mash/Classic?
,
Mar 13 2018
The cl adding the field trial effectively enables Mus by default for tip-of-tree builds (not dev/beta/stable, but possibly Canary). I think what has to happen is that any where we are checking for Mash, we check for it before Mus. And any where we are checking for Mus only, we have a && !mash. We may all be good here, but I'm not positive.
,
Mar 13 2018
Okay, I'll try to audit our use of the feature switches and the config value.
,
Mar 14 2018
AFAICT, only ComputeAshConfig and IsMusEnabled check for base::Feature kMus directly, they look correct. IsMusEnabled returns true for (kMus || kMash) and has a lot more callers, but they seem to do the right thing for Mash too. Many places check for kMash directly, some are odd (eg. aura::Env::Mode::LOCAL || !kMash), but these look suspicious/wrong: ViewsMusTestSuite::Initialize enables kMash, not kMus, is that intended? https://cs.chromium.org/chromium/src/ui/views/mus/views_mus_test_suite.cc?rcl=fd02d61c7a2eb98543da651173025d84515d61ee&l=250 AuraTestBase::ConfigureBackend enables kMash as needed, but doesn't seem to enable kMus? https://cs.chromium.org/chromium/src/ui/aura/test/aura_test_base.cc?rcl=c2f61690f7a59a4c6ac7cc4b99dd92b44e810f23&l=152 WindowServerTestBase::SetUp enables kMash, should it always do that? https://cs.chromium.org/chromium/src/services/ui/ws/window_server_test_base.cc?rcl=a419f154068880cc0806c78def9e660bd1a98fc6&l=96 MusDemoTest::SetUp enables kMash, is that right? https://cs.chromium.org/chromium/src/services/ui/demo/mus_demo_unittests.cc?rcl=e0519ce7086c636a9955c78fc730f6ecf56215aa&l=35 Overall, I didn't find anything that used kMus in a manner that would exclude kMash or prevent mash-only code from executing. Obviously, I don't understand all the intricate workings of the callers, so it's possible I missed something.
,
Mar 14 2018
Unfortunately Mash as a feature name is mildly confusing. Mash controls two things: should the Window Service host viz, and are chrome and ash separate. Standalone test suites that connect to the window service generally want Mash mode, because they need the Window Service to host viz (no one else will). ViewsMusTestSuite, MusDemoTest and WindowServerTestBase all connect to the WindowService, so they need Mash. AuraTestBase has tests for local, mus and mash mode. So, it's doing the right thing. I think this means we can close this out.
,
Mar 14 2018
Sounds good! (IIUC AuraTestBase allows fixtures to change the config, it's not run separately for classic/mus/mash like ash_unittests) |
|||
►
Sign in to add a comment |
|||
Comment 1 by msw@chromium.org
, Mar 13 2018Status: Started (was: Assigned)