New issue
Advanced search Search tips

Issue 821470 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Make sure --enable-features=Mash takes precedence over --enable-features=Mus

Project Member Reported by sky@chromium.org, Mar 13 2018

Issue description

I'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.

 

Comment 1 by msw@chromium.org, Mar 13 2018

Cc: asvitk...@chromium.org rkaplow@chromium.org
Status: Started (was: Assigned)
I tried some naive practical testing with combinations of commandline switches and enabling the about:flags #Mus.

All of these commandlines yield the "Mash" status area item, so Mash is indeed enabled:
chrome --enable-features=Mus,Mash
chrome --enable-features=Mash,Mus
chrome --enable-features=Mash (with about:flags #Mus enabled)
chrome --enable-features=Mus --enable-features=Mash

This commandline doesn't use Mash (likely because of switch ordering precedence):
chrome --enable-features=Mash --enable-features=Mus

I also applied the field trial CL locally: https://chromium-review.googlesource.com/c/chromium/src/+/932821
Then I ran the "chrome --enable-features=Mash" and it seems to run with Mash enabled.
Now I need to figure out how to force myself into a variation that enables the Mus field trial...
Can someone point me in the right direction?
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...

Comment 3 by sky@chromium.org, 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.

Comment 4 by msw@chromium.org, 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?

Comment 5 by sky@chromium.org, 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.

Comment 6 by msw@chromium.org, Mar 13 2018

Okay, I'll try to audit our use of the feature switches and the config value.

Comment 7 by msw@chromium.org, Mar 14 2018

Cc: -asvitk...@chromium.org -rkaplow@chromium.org
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.

Comment 8 by sky@chromium.org, 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.

Comment 9 by msw@chromium.org, Mar 14 2018

Status: WontFix (was: Started)
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