Show all experiments, even those that are unavailable on the current platform

Show all experiments, even those that are unavailable on the current platform

BUG=53560
TEST=Go to about:flags, check that all experiments are shown, with a list of the platforms where each one can run.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115054

[Tyler Breisacher (Chromium), 2011-12-14 07:15:01]

Adding Peter as the reviewer as the bug was assigned to him previously. Let me
know if I should add anyone else.

[Nico, 2011-12-16 20:25:07]

Overall approach looks nice!

Remove R= line from cl description, that's not widely used.

When you add someone as reviewer, you need to hit "publish+mail comments", else
they will never know

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags....
chrome/browser/about_flags.cc:45: std::map<int, std::string> nameMap;
Global non-pods need both static initialization and exit time destruction, both
things that we're trying to get rid of (and both things that will turn bots red
nowadays)

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags....
chrome/browser/about_flags.cc:52: nameMap[kOsCrOS] = "Chrome OS";
std::map<int, ...> with less than many keys is almost always better done as a
switch statement

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags....
chrome/browser/about_flags.cc:664: }
supported_platforms->Append(new StringValue("Mac"));
supported_platforms->Append(new StringValue("Windows"));
supported_platforms->Append(new StringValue("Linux"));
supported_platforms->Append(new StringValue("ChromeOS"));

does the same thing, is easier to understand, and less code

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
File chrome/browser/resources/flags.css (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
chrome/browser/resources/flags.css:113: however, should remain completely
legible. */
Move this comment up above your new rules, s/Disabled/Disabled and unsupported/,
s/completely//

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:49: <tr jsvalues="class:supported ? (enabled
? 'experiment-enabled' : 'experiment-disabled')
80 col violation

[Tyler Breisacher (Chromium), 2011-12-16 21:18:11]

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags....
chrome/browser/about_flags.cc:664: }
Wouldn't that append all four platforms to every single experiment? We want to
only show the platform(s) where this particular experiment is available. I'm not
sure how to do this more clearly.

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
File chrome/browser/resources/flags.css (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
chrome/browser/resources/flags.css:113: however, should remain completely
legible. */
On 2011/12/16 20:25:07, Nico wrote:
> Move this comment up above your new rules, s/Disabled/Disabled and
unsupported/,
> s/completely//

Done.

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:49: <tr jsvalues="class:supported ? (enabled
? 'experiment-enabled' : 'experiment-disabled')
On 2011/12/16 20:25:07, Nico wrote:
> 80 col violation

Done.

[Nico, 2011-12-16 21:38:01]

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/12008/chrome/browser/about_flags....
chrome/browser/about_flags.cc:664: }
On 2011/12/16 21:18:11, tbreisacher wrote:
> Wouldn't that append all four platforms to every single experiment? We want to
> only show the platform(s) where this particular experiment is available. I'm
not
> sure how to do this more clearly.

Ah, right. Either write the function you have above with a switch and do

for (int platforms = experiment.supported_platforms; platforms; platforms &=
platforms - 1)
  supported_platforms->Append(new StringValue(NameForPlatform(platform &
-platform)));

(which I suppose is a bit hard to read) or do

void AddOsStrings(bitmask, list) {
  struct { int bit, const char str[] } kBitsToOs = {
    ...
  }
  for (int i = 0; i < ARRAYSIZE_UNSAFE(kBitsToOs); ++i)
    if (bitmask & kBitsToOs[i].bit)
      list->Append(new StringValue(kBitsToOs[i].str)
}

and call that from here

I like the second approach, since it also reduces the number of nested loops in
GetFlagsExperimentData

[Tyler Breisacher (Chromium), 2011-12-17 03:29:00]

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:48: const char str[10];
This "10" is super ugly. Without it, I get a compiler error: "initializer-string
for array of chars is too long". There has to be a less hacky way to fix that,
right?

[Nico, 2011-12-17 03:34:26]

Almost there, just some final nits :-)

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4603: <message name="IDS_FLAGS_NOT_SUPPORTED"
desc="Shown instead of the Enable/Disable link, to let the user know that an
experiment is not supported on the current platform.">
NOT_AVAILABLE? also in desc

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:45: void AddOsStrings(unsigned bitmask, ListValue*
list) {
Needs a very short comment above it describing what it does

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:48: const char str[10];
On 2011/12/17 03:29:01, tbreisacher wrote:
> This "10" is super ugly. Without it, I get a compiler error:
"initializer-string
> for array of chars is too long". There has to be a less hacky way to fix that,
> right?

const char* const str

Also, "name" is probably better than "str", I was in pseudo-code-typing mode

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:55: for (unsigned i = 0; i <
ARRAYSIZE_UNSAFE(kBitsToOs); ++i)
does "int" not work?

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/resources/fl...
File chrome/browser/resources/flags.css (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/resources/fl...
chrome/browser/resources/flags.css:106: .experiment-unsupported > td {
I wonder if this should be

.experiment-disabled > td,
.experiment-unsupported > td {
  ...
}

That way the bodies aren't duplicated.

[Tyler Breisacher (Chromium), 2011-12-17 04:20:34]

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4603: <message name="IDS_FLAGS_NOT_SUPPORTED"
desc="Shown instead of the Enable/Disable link, to let the user know that an
experiment is not supported on the current platform.">
On 2011/12/17 03:34:27, Nico wrote:
> NOT_AVAILABLE? also in desc

Done.

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:45: void AddOsStrings(unsigned bitmask, ListValue*
list) {
On 2011/12/17 03:34:27, Nico wrote:
> Needs a very short comment above it describing what it does

Done. Although writing the comment is making me think it might be clearer if
this function takes the Experiment, rather than the bitmask. What do you think?

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:48: const char str[10];
On 2011/12/17 03:34:27, Nico wrote:
> On 2011/12/17 03:29:01, tbreisacher wrote:
> > This "10" is super ugly. Without it, I get a compiler error:
> "initializer-string
> > for array of chars is too long". There has to be a less hacky way to fix
that,
> > right?
> 
> const char* const str
> 
> Also, "name" is probably better than "str", I was in pseudo-code-typing mode

Done.

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:55: for (unsigned i = 0; i <
ARRAYSIZE_UNSAFE(kBitsToOs); ++i)
On 2011/12/17 03:34:27, Nico wrote:
> does "int" not work?

chrome/browser/about_flags.cc:59: error: comparison between signed and unsigned
integer expressions

Although, the ARRAYSIZE_UNSAFE documentation says it returns a |size_t| not an
|unsigned| so it's probably better to declare it as size_t.

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/resources/fl...
File chrome/browser/resources/flags.css (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/resources/fl...
chrome/browser/resources/flags.css:106: .experiment-unsupported > td {
On 2011/12/17 03:34:27, Nico wrote:
> I wonder if this should be
> 
> .experiment-disabled > td,
> .experiment-unsupported > td {
>   ...
> }
> 
> That way the bodies aren't duplicated.

Sure, I don't see why not. If someone decides later to make them look different,
they can always re-split them. Done.

[Nico, 2011-12-17 05:14:20]

lgtm, but maybe change the name too

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:4603: <message name="IDS_FLAGS_NOT_SUPPORTED"
desc="Shown instead of the Enable/Disable link, to let the user know that an
experiment is not supported on the current platform.">
On 2011/12/17 04:20:34, tbreisacher wrote:
> On 2011/12/17 03:34:27, Nico wrote:
> > NOT_AVAILABLE? also in desc
> 
> Done.

Looks like you changed only the desc attribute, not the name

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/24002/chrome/browser/about_flags....
chrome/browser/about_flags.cc:45: void AddOsStrings(unsigned bitmask, ListValue*
list) {
On 2011/12/17 04:20:34, tbreisacher wrote:
> On 2011/12/17 03:34:27, Nico wrote:
> > Needs a very short comment above it describing what it does
> 
> Done. Although writing the comment is making me think it might be clearer if
> this function takes the Experiment, rather than the bitmask. What do you
think?

It's fine either way I think

http://codereview.chromium.org/8898039/diff/22007/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/22007/chrome/browser/about_flags....
chrome/browser/about_flags.cc:46: // the experiment is available on that
platform.
s/the/whether the/

[commit-bot: I haz the power, 2011-12-17 23:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8898039/24004

[commit-bot: I haz the power, 2011-12-17 23:39:44]

Presubmit check for 8898039-24004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/resources/flags.css,chrome/browser/resources/flags.js,chrome/browser/resources/flags.html

[Tyler Breisacher (Chromium), 2011-12-17 23:50:57]

OWNERS are arv, jhawkins, and estade. Can one of you take a look at this please?

[James Hawkins, 2011-12-17 23:58:26]

On 2011/12/17 23:50:57, tbreisacher wrote:
> OWNERS are arv, jhawkins, and estade. Can one of you take a look at this
please?

In the future please don't add more than one OWNER for review at a time.

[James Hawkins, 2011-12-18 00:01:59]

http://codereview.chromium.org/8898039/diff/24004/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/24004/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:75: i18n-content="flagsNotSupported">NOT
AVAILABLE ON THIS PLATFORM
I don't think you meant to leave in the inner text content here.

[Tyler Breisacher (Chromium), 2011-12-18 02:23:57]

http://codereview.chromium.org/8898039/diff/24004/chrome/browser/resources/fl...
> chrome/browser/resources/flags.html:75: i18n-content="flagsNotSupported">NOT
> AVAILABLE ON THIS PLATFORM
> I don't think you meant to leave in the inner text content here.

I was following the example of what is done elsewhere in the same file. I assume
that's so you can load the HTML directly without building the whole browser, to
quickly see the structure and styling of the page.

[James Hawkins, 2011-12-18 21:38:25]

http://codereview.chromium.org/8898039/diff/24004/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/24004/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:75: i18n-content="flagsNotSupported">NOT
AVAILABLE ON THIS PLATFORM
On 2011/12/18 00:01:59, James Hawkins wrote:
> I don't think you meant to leave in the inner text content here.

Please remove this one.

The content of the resources should match what we ship (and no more).  Building
doesn't take that long, and if it does take too long, we should fix that.

[Peter Beverloo, 2011-12-19 15:23:56]

Thank you for taking care of this!

[Tyler Breisacher (Chromium), 2011-12-19 18:00:08]

> Please remove this one.
> 
> The content of the resources should match what we ship (and no more). 
Building
> doesn't take that long, and if it does take too long, we should fix that.

That makes sense. Done.

[James Hawkins, 2011-12-19 18:09:44]

LGTM with nits.

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:47: void AddOsStrings(unsigned bitmask, ListValue*
list) {
nit: s/Os/OS/

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:57: <span
jscontent="supported_platforms.join(', ')">MAC, LINUX</span>
Remove inner content

[Nico, 2011-12-19 18:53:36]

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/about_flags....
chrome/browser/about_flags.cc:47: void AddOsStrings(unsigned bitmask, ListValue*
list) {
On 2011/12/19 18:09:44, James Hawkins wrote:
> nit: s/Os/OS/

Don't do this. We generally treat abbreviations as words, since not doing so
leads to abominations like HTTPURL (HttpUrl reads much nicer).

[Tyler Breisacher (Chromium), 2011-12-19 18:59:28]

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/resources/fl...
File chrome/browser/resources/flags.html (right):

http://codereview.chromium.org/8898039/diff/32001/chrome/browser/resources/fl...
chrome/browser/resources/flags.html:57: <span
jscontent="supported_platforms.join(', ')">MAC, LINUX</span>
On 2011/12/19 18:09:44, James Hawkins wrote:
> Remove inner content

Done.

[commit-bot: I haz the power, 2011-12-19 21:05:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8898039/31008

[commit-bot: I haz the power, 2011-12-19 22:18:10]

Try job failure for 8898039-31008 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Tyler Breisacher (Chromium), 2011-12-19 23:23:25]

http://codereview.chromium.org/8898039/diff/16003/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/8898039/diff/16003/chrome/browser/about_flags....
chrome/browser/about_flags.cc:659: bool supported =
!!(experiment.supported_platforms & current_platform);
This should fix the compile error on Windows: warning C4800: 'const unsigned
int' : forcing value to bool 'true' or 'false'.

http://codereview.chromium.org/8898039/diff/32005/chrome/browser/about_flags_...
File chrome/browser/about_flags_unittest.cc (right):

http://codereview.chromium.org/8898039/diff/32005/chrome/browser/about_flags_...
chrome/browser/about_flags_unittest.cc:226: EXPECT_EQ(arraysize(kExperiments),
switch_prefs->GetSize());
The kExperiments array used for testing has a few experiments that are available
on the current platform, and 1 that is not. So GetFlagsExperimentsData would
always return a list whose length is one less than kExperiments. Now,
GetFlagsExperimentsData always returns *all* experiments, so the sizes should
match exactly. The same thing applies at line 276.

[James Hawkins, 2011-12-19 23:25:29]

SLGTM

[commit-bot: I haz the power, 2011-12-19 23:32:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8898039/32005

[Nico, 2011-12-19 23:32:43]

slgtm2

[commit-bot: I haz the power, 2011-12-20 01:01:43]

Change committed as 115054