Enable MD by default on Windows for Canary/Dev and local builds.

Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none

Committed: https://crrev.com/56100334b16d67730fef9918be5b0c1131dacad9
Cr-Commit-Position: refs/heads/master@{#393992}

[Peter Kasting, 2016-05-06 05:46:08]

pkasting@chromium.org changed reviewers:
+ mpearson@chromium.org, varkha@chromium.org

[Peter Kasting, 2016-05-06 05:46:09]

mpearson: Please check if the way I did the field trial stuff in
material_design_controller.cc makes sense

varkha: everything else, but especially see if moving the blocks around that I
did in chrome_browser_main.cc makes sense; this feels extremely risky to me

[varkha, 2016-05-06 14:58:22]

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1008: if (first_run::IsChromeFirstRun()) {
This seems like the riskiest area that you are bypassing. I hope nothing there
needs to show UI / load assets or lazily initialize something. Were you able to
try first-run flow (I am not sure how well it is covered by tests)?

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1100: (channel ==
version_info::Channel::DEV));
Would it make sense to conditionally compile this as well as
ui::MaterialDesignController windows bits only when OS_WIN is defined?

[varkha, 2016-05-06 16:32:16]

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1006: // browser's profile_manager object
is created, but after ResourceBundle
Is this comment still accurate? ResourceBundle::InitSharedInstanceWithLocale is
now called below in line 1133.

[varkha, 2016-05-06 17:02:55]

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1010:
first_run::ProcessMasterPreferences(user_data_dir_,
I think this may call ShowPostInstallEULAIfNeeded which if I trust [1] is a
resource so may not be loaded yet. It then may call [2] which seems to be using
a string resource.
You probably don't need MDC initialized there but you may need to call
InitSharedInstanceWithLocale(DO_NOT_LOAD_COMMON_RESOURCES) and then unload
similar to how it is done in [3]. I think this is all Windows-only so I didn't
really have an easy way to check this flow.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/fir...

[3]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[varkha, 2016-05-06 17:05:52]

Description was changed from

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none
==========

to

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none
==========

[Mark P, 2016-05-06 20:40:57]

Do you have a command line to control the MD state?  If so, then do you want the
finch trial to override the command line flag?  Usually we make command line
flags take priority over finch so particular users can get the behavior they
want.

If you have a flag and you want the flag to override the trial, here's how the
code should look:
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/finch-and-flags.md?cl=...

That said, I feel reminisce if I don't mention the finch feature API.  That's
supposed to be the new best practice for adding experiments to Chrome.  However,
I've always found it awkward for multi-armed experiments as yours appears to be.
 But here's the brief doc on this in case you feel like it'd appropriate:
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head

--mark

[Peter Kasting, 2016-05-06 20:49:12]

On 2016/05/06 20:40:57, Mark P wrote:
> Do you have a command line to control the MD state?  If so, then do you want
the
> finch trial to override the command line flag?  Usually we make command line
> flags take priority over finch so particular users can get the behavior they
> want.
> 
> If you have a flag and you want the flag to override the trial, here's how the
> code should look:
>
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/finch-and-flags.md?cl=...

We have a flag and as written it will override the trial here.  See
MaterialDesignController::Initialize(), which only falls back to calling this
DefaultMode() method when the flag is not set.  (That means that when the user
sets the flag, the field trial code is never called, so users won't be reported
as being in the trial at all, which I assumed was desirable.)

The flag is not an enable/disable flag but a multi-state flag,
--top-chrome-md={various values}.  It's not clear to me whether that works with
the forcing_flag stuff you link to.

> That said, I feel reminisce if I don't mention the finch feature API.  That's
> supposed to be the new best practice for adding experiments to Chrome. 
However,
> I've always found it awkward for multi-armed experiments as yours appears to
be.
>  But here's the brief doc on this in case you feel like it'd appropriate:
> https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head

Again, this seems to assume features are on or off, rather than multi-state.  I
don't know how to reconcile this.

[Mark P, 2016-05-06 21:31:43]

Field trial usage looks good to me.

On 2016/05/06 20:49:12, Peter Kasting wrote:
> On 2016/05/06 20:40:57, Mark P wrote:
> > Do you have a command line to control the MD state?  If so, then do you want
> the
> > finch trial to override the command line flag?  Usually we make command line
> > flags take priority over finch so particular users can get the behavior they
> > want.
> > 
> > If you have a flag and you want the flag to override the trial, here's how
the
> > code should look:
> >
>
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/finch-and-flags.md?cl=...
> 
> We have a flag and as written it will override the trial here.  See
> MaterialDesignController::Initialize(), which only falls back to calling this
> DefaultMode() method when the flag is not set.  (That means that when the user
> sets the flag, the field trial code is never called, so users won't be
reported
> as being in the trial at all, which I assumed was desirable.)
> 
> The flag is not an enable/disable flag but a multi-state flag,
> --top-chrome-md={various values}.  It's not clear to me whether that works
with
> the forcing_flag stuff you link to.

You're right.  It doesn't appear to work.

> > That said, I feel reminisce if I don't mention the finch feature API. 
That's
> > supposed to be the new best practice for adding experiments to Chrome. 
> However,
> > I've always found it awkward for multi-armed experiments as yours appears to
> be.
> >  But here's the brief doc on this in case you feel like it'd appropriate:
> >
https://g3doc.corp.google.com/analysis/uma/g3doc/finch/feature-api.md?cl=head
> 
> Again, this seems to assume features are on or off, rather than multi-state. 
I
> don't know how to reconcile this.

Acknowledged.

[Peter Kasting, 2016-05-13 06:36:45]

pkasting@chromium.org changed reviewers:
+ sky@chromium.org

[Peter Kasting, 2016-05-13 06:36:46]

+sky for ui/ OWNERS review.

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1006: // browser's profile_manager object
is created, but after ResourceBundle
On 2016/05/06 16:32:16, varkha wrote:
> Is this comment still accurate? ResourceBundle::InitSharedInstanceWithLocale
is
> now called below in line 1133.

Good catch.

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1008: if (first_run::IsChromeFirstRun()) {
On 2016/05/06 14:58:22, varkha wrote:
> This seems like the riskiest area that you are bypassing. I hope nothing there
> needs to show UI / load assets or lazily initialize something. Were you able
to
> try first-run flow (I am not sure how well it is covered by tests)?

No :(.

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1010:
first_run::ProcessMasterPreferences(user_data_dir_,
On 2016/05/06 17:02:55, varkha wrote:
> I think this may call ShowPostInstallEULAIfNeeded which if I trust [1] is a
> resource so may not be loaded yet.

Yep.  There are also other indirect dependencies inside there which need
resources as well.

I refactored the code from bad_flags_prompt.cc into a utility in
resource_bundle.* and used it to solve this.  I was reluctant to do this since
this seems rather special-purpose and exposing it in resource_bundle.h feels
slightly unfortunate.  But I really needed a scoping object, and not just a
copy-and-paste of the existing code, in the new location, so I decided if I had
to write such a thing, I might as well use it both places, and ResourceBundle
itself seemed like the obvious place to put it.

https://codereview.chromium.org/1955773002/diff/40001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1100: (channel ==
version_info::Channel::DEV));
On 2016/05/06 14:58:22, varkha wrote:
> Would it make sense to conditionally compile this as well as
> ui::MaterialDesignController windows bits only when OS_WIN is defined?

Sure.  It would be harmless to do the other way, but why run code you don't need
to elsewhere.

I elected not to #ifdef all the related bits in material_design_controller.* in
the interest of fewer ifdefs and thus hopefully a little more readability.  This
basically just turns into an unused function/member on other platforms.  I could
be convinced to ifdef these as well if you feel strongly, but it was two more
sets of ifdefs in each of the .h and .cc file, compared with just one total
additional ifdef here.

[sky, 2016-05-13 15:49:57]

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1134:
ui::ResourceBundle::InitSharedInstanceWithLocale(
Is resourcebundle set up to have Init called multiple times?

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1135: locale, NULL,
ui::ResourceBundle::LOAD_COMMON_RESOURCES);
nullptr

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:45: static Mode
DefaultMode();
sorry, random question. Can this be private?

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:50: static void
SetDefaultToMaterial(bool default_to_material);
I find it mildly confusing that you expect this function to be called before
initialize. Did you consider naming this InitializeWithDefault?

https://codereview.chromium.org/1955773002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1955773002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:129: locale, NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
nullptr

[varkha, 2016-05-14 05:03:08]

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1100: (channel ==
version_info::Channel::DEV));
Could this channel check be done inside MaterialDesignController::DefaultMode? I
am a bit concerned that we do this here at a call site and so the other places
that call MDC::Initialize() (zygote or tests) will not be getting the same value
as the browser. Or is this intentional?

[Peter Kasting, 2016-05-16 19:53:16]

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1100: (channel ==
version_info::Channel::DEV));
On 2016/05/14 05:03:08, varkha wrote:
> Could this channel check be done inside MaterialDesignController::DefaultMode?

No.  That's why this unorthodox design exists.  MaterialDesignController lives
in ui/base/, which does not depend on chrome/.  The channel checker is a
chrome/-level concept and thus cannot be accessed from here.

> I
> am a bit concerned that we do this here at a call site and so the other places
> that call MDC::Initialize() (zygote or tests) will not be getting the same
value
> as the browser. Or is this intentional?

Most of these places are also not chrome/-level and thus can't vary by channel.

However, in those cases, I think the right thing to do is to default to material
mode and not non-material.  I reversed the sense of the code such that Windows
is now always material except when we call it from chrome_browser_main.cc.

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1134:
ui::ResourceBundle::InitSharedInstanceWithLocale(
On 2016/05/13 15:49:56, sky wrote:
> Is resourcebundle set up to have Init called multiple times?

It should be OK if and only if you unload the shared instance after any prior
init.

https://codereview.chromium.org/1955773002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main.cc:1135: locale, NULL,
ui::ResourceBundle::LOAD_COMMON_RESOURCES);
On 2016/05/13 15:49:56, sky wrote:
> nullptr

Done.

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:45: static Mode
DefaultMode();
On 2016/05/13 15:49:56, sky wrote:
> sorry, random question. Can this be private?

It's called elsewhere only by tests.  I think it's better to restrict access to
it to just testing, so I did so.

https://codereview.chromium.org/1955773002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:50: static void
SetDefaultToMaterial(bool default_to_material);
On 2016/05/13 15:49:56, sky wrote:
> I find it mildly confusing that you expect this function to be called before
> initialize. Did you consider naming this InitializeWithDefault?

You mean, by combining its functionality with Initialize()?  That's a good idea,
and will remove the need for the static variable.  Done.

https://codereview.chromium.org/1955773002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/1955773002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle.cc:129: locale, NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
On 2016/05/13 15:49:56, sky wrote:
> nullptr

Done.

[sky, 2016-05-16 19:56:11]

LGTM

https://codereview.chromium.org/1955773002/diff/80001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1955773002/diff/80001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:63:
MaterialDesignController() = delete;
DISALLOW_IMPLICIT_CONSTRUCTORS?

[varkha, 2016-05-16 22:42:06]

Thanks for explaining this, makes sense.
lgtm.

[Peter Kasting, 2016-05-16 23:09:15]

https://codereview.chromium.org/1955773002/diff/80001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/1955773002/diff/80001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:63:
MaterialDesignController() = delete;
On 2016/05/16 19:56:10, sky wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS?

Sure.

[Peter Kasting, 2016-05-16 23:09:21]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-05-16 23:09:21]

The patchset sent to the CQ was uploaded after l-g-t-m from varkha@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1955773002/#ps100001
(title: "DISALLOW_IMPLICIT_CONSTRUCTORS")

[commit-bot: I haz the power, 2016-05-16 23:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1955773002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1955773002/100001

[commit-bot: I haz the power, 2016-05-17 00:28:23]

Description was changed from

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none
==========

to

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none
==========

[commit-bot: I haz the power, 2016-05-17 00:28:26]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-17 00:29:54]

Description was changed from

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none
==========

to

==========
Enable MD by default on Windows for Canary/Dev and local builds.

Allow a field trial to override the default enable state.

BUG=609044
TEST=none

Committed: https://crrev.com/56100334b16d67730fef9918be5b0c1131dacad9
Cr-Commit-Position: refs/heads/master@{#393992}
==========

[commit-bot: I haz the power, 2016-05-17 00:29:55]

Patchset 6 (id:??) landed as
https://crrev.com/56100334b16d67730fef9918be5b0c1131dacad9
Cr-Commit-Position: refs/heads/master@{#393992}

[Nico, 2016-05-17 13:37:32]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-05-17 13:37:32]

I think this broke tests in official builds. Filed
https://bugs.chromium.org/p/chromium/issues/detail?id=612437