Compile-time and run-time flags for top chrome material design.

Compile-time and run-time flags for top chrome material design.

Compile time flag controls the inclusion of both md and regular assets.
Run-time flag (only available when the compile-time-flag is enabled)
determines which assets we use.

BUG=458662
Committed: https://crrev.com/fca876aa8f6905a75976ef0e15338b896d1a64e6
Cr-Commit-Position: refs/heads/master@{#331187}

[girard, 2015-04-28 14:32:34]

girard@chromium.org changed reviewers:
+ tdanderson@chromium.org

[girard, 2015-04-28 14:32:34]

FYI

[tdanderson, 2015-05-11 16:55:24]

tdanderson@chromium.org changed reviewers:
+ bruthig@chromium.org, jonross@chromium.org

[tdanderson, 2015-05-11 16:55:25]

+Jon and +Ben

[bruthig, 2015-05-11 17:12:21]

https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:894: ENABLE_DISABLE_VALUE_TYPE(
How come you aren't using a tri-state flag with a default value?

[girard, 2015-05-11 20:15:45]

https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:894: ENABLE_DISABLE_VALUE_TYPE(
On 2015/05/11 17:12:20, bruthig wrote:
> How come you aren't using a tri-state flag with a default value?

This _is_ a tri-state flag. It appears in the UI with values of "Default",
"Enabled" and "Disabled".

The macro name is a little mis-leading;)

[girard, 2015-05-14 20:27:38]

Patchset #2 (id:20001) has been deleted

[jonross, 2015-05-20 17:14:04]

On 2015/05/11 20:15:45, girard wrote:
>
https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
> File chrome/browser/about_flags.cc (right):
> 
>
https://codereview.chromium.org/1108333002/diff/40001/chrome/browser/about_fl...
> chrome/browser/about_flags.cc:894: ENABLE_DISABLE_VALUE_TYPE(
> On 2015/05/11 17:12:20, bruthig wrote:
> > How come you aren't using a tri-state flag with a default value?
> 
> This _is_ a tri-state flag. It appears in the UI with values of "Default",
> "Enabled" and "Disabled".
> 
> The macro name is a little mis-leading;)

LGTM

[girard, 2015-05-20 18:27:19]

girard@chromium.org changed reviewers:
+ cpu@chromium.org

[girard, 2015-05-20 18:27:20]

PTAL

[bruthig, 2015-05-20 19:25:05]

lgtm

[cpu_(ooo_6.6-7.5), 2015-05-20 23:56:47]

lgtm

[girard, 2015-05-21 13:31:30]

The CQ bit was checked by girard@chromium.org

[commit-bot: I haz the power, 2015-05-21 13:31:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108333002/60001

[commit-bot: I haz the power, 2015-05-21 17:27:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-21 17:27:25]

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_...)

[girard, 2015-05-22 15:17:28]

girard@chromium.org changed reviewers:
+ asvitkine@chromium.org

[girard, 2015-05-22 15:17:29]

asvitkine@chromium.org: Please review changes in 

histograms.xml

(I neglected to add this in the original patchset. Oof!)

[Alexei Svitkine (slow), 2015-05-22 16:56:44]

https://codereview.chromium.org/1108333002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1108333002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:41750: +<histogram name="TopChromeMD">
I don't see where you're logging this histogram. Could you clarify?

Also, it would be better not to introduce a new top-level prefix for just one
histogram - better to find an existing place where you can nest it under.

Finally, it looks like this is a histogram with two possible values, so instead
of having two separate histograms (as you've defined via histogram_suffixes),
it's better to use an enumerated histogram.

[girard, 2015-05-22 17:20:53]

On 2015/05/22 16:56:44, Alexei Svitkine wrote:
>
https://codereview.chromium.org/1108333002/diff/80001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1108333002/diff/80001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:41750: +<histogram name="TopChromeMD">
> I don't see where you're logging this histogram. Could you clarify?
> 
> Also, it would be better not to introduce a new top-level prefix for just one
> histogram - better to find an existing place where you can nest it under.
> 
> Finally, it looks like this is a histogram with two possible values, so
instead
> of having two separate histograms (as you've defined via histogram_suffixes),
> it's better to use an enumerated histogram.


Thanks for your review Alexei, and you're right - I was adding this to the wrong
place. I misunderstood the feedback from the unit test.

I've revised the patch to add the two settings into the correct place
(LoginCustomFlags).

[Alexei Svitkine (slow), 2015-05-22 18:20:44]

lgtm

[girard, 2015-05-22 18:24:44]

The CQ bit was checked by girard@chromium.org

[girard, 2015-05-22 18:24:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
jonross@chromium.org, cpu@chromium.org, bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/1108333002/#ps100001
(title: "Corrected histograms.xml")

[commit-bot: I haz the power, 2015-05-22 18:25:12]

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

[commit-bot: I haz the power, 2015-05-22 18:30:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-22 18:30:43]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[girard, 2015-05-22 19:58:04]

The CQ bit was checked by girard@chromium.org

[girard, 2015-05-22 19:58:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
jonross@chromium.org, asvitkine@chromium.org, cpu@chromium.org,
bruthig@chromium.org
Link to the patchset: https://codereview.chromium.org/1108333002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-05-22 19:58:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108333002/120001

[commit-bot: I haz the power, 2015-05-22 22:01:12]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-05-22 22:02:07]

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

[Dan Beam, 2015-06-24 21:45:06]

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

[Dan Beam, 2015-06-24 21:45:07]

https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn
File build/config/BUILD.gn (right):

https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn#...
build/config/BUILD.gn:231: defines += [ "ENABLE_TOPCHRME_MD=1" ]
note: misspelled

https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:300: #ifdef ENABLE_TOPCHROME_MD
prefer #if defined() to #ifdef

https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
chrome/common/chrome_switches.h:301: extern const char kTopChromeMD[];
^ this is never used and I have no idea why it's necessary (or what it's
supposed to do) when you've got an "enabled" and "disabled" flag...

[tdanderson, 2015-06-24 21:49:35]

On 2015/06/24 21:45:07, Dan Beam wrote:
> https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn
> File build/config/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn#...
> build/config/BUILD.gn:231: defines += [ "ENABLE_TOPCHRME_MD=1" ]
> note: misspelled
> 
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> File chrome/common/chrome_switches.h (right):
> 
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> chrome/common/chrome_switches.h:300: #ifdef ENABLE_TOPCHROME_MD
> prefer #if defined() to #ifdef
> 
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> chrome/common/chrome_switches.h:301: extern const char kTopChromeMD[];
> ^ this is never used and I have no idea why it's necessary (or what it's
> supposed to do) when you've got an "enabled" and "disabled" flag...

dbeam@, this CL has already landed, but further modifications to the
flags are in progress here: https://codereview.chromium.org/1188713007/

[Dan Beam, 2015-06-24 21:51:19]

On 2015/06/24 21:49:35, tdanderson wrote:
> On 2015/06/24 21:45:07, Dan Beam wrote:
> > https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn
> > File build/config/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/1108333002/diff/120001/build/config/BUILD.gn#...
> > build/config/BUILD.gn:231: defines += [ "ENABLE_TOPCHRME_MD=1" ]
> > note: misspelled
> > 
> >
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> > File chrome/common/chrome_switches.h (right):
> > 
> >
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> > chrome/common/chrome_switches.h:300: #ifdef ENABLE_TOPCHROME_MD
> > prefer #if defined() to #ifdef
> > 
> >
>
https://codereview.chromium.org/1108333002/diff/120001/chrome/common/chrome_s...
> > chrome/common/chrome_switches.h:301: extern const char kTopChromeMD[];
> > ^ this is never used and I have no idea why it's necessary (or what it's
> > supposed to do) when you've got an "enabled" and "disabled" flag...
> 
> dbeam@, this CL has already landed, but further modifications to the
> flags are in progress here: https://codereview.chromium.org/1188713007/

I'm looking at that as well