Add field trial for enabling Stage3D in XP

Add field trial for enabling Stage3D in XP

Adds an experiment that we can set to enable Stage3D on XP (where not completely blacklisted). It will be disabled by default.


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

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

[jbauman, 2012-09-19 20:04:30]

Adding vangelis to review the patch, asivitkine@ for review of the FieldTrial
stuff, and brettw for various OWNERS reviews.

[brettw, 2012-09-19 20:10:36]

LGTM but I don't know a lot about the field trial stuff. Be sure one of the
reviewers does.

[Alexei Svitkine (slow), 2012-09-19 20:33:16]

https://codereview.chromium.org/10944037/diff/1/chrome/browser/chrome_gpu_uti...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/10944037/diff/1/chrome/browser/chrome_gpu_uti...
chrome/browser/chrome_gpu_util.cc:140: trial->Disable();
Disabling the trial has the effect of choosing the default group. I think that's
not what you want here since you've chosen your default to be "blacklisted". I'm
assuming you don't want to blacklist users on Windows 7.

I think you probably want to make "Enabled" your default group, then disabling
the trial on non-XP platforms will have the correct behavior. In this scenario,
you would add the "blacklisted" group with 100% when creating the trial (which
happens only under XP).

Then, Stage3DBlacklisted() call in gpu_data_manager_impl.cc could simply check
the state of the trial (in the way I suggest below) without needing to also do
platform checks there. It will work correctly because:

1. On XP with no trial config, it will use the client set up code which will add
"blacklisted" as 100%, checking for "blacklisted" always works.
2. On XP with trial config, checking for "blacklisted" will work per config
probabilities.
3. On non-XP without trial config, trial is not created, so group value will not
be "blacklisted", so all is good.
4. On non-XP with trial config, trial will be created by config and then
disabled by this code, taking the default group of "enabled".

("trial config" means what we get from the server.)

https://codereview.chromium.org/10944037/diff/1/content/browser/gpu/gpu_data_...
File content/browser/gpu/gpu_data_manager_impl.cc (right):

https://codereview.chromium.org/10944037/diff/1/content/browser/gpu/gpu_data_...
content/browser/gpu/gpu_data_manager_impl.cc:398: trial->group_name() ==
content::kStage3DFieldTrialBlacklistedName;
Nit: You can just use:

return base::FieldTrialList::FindFullName(content::kStage3DFieldTrialName) ==
      content::kStage3DFieldTrialBlacklistedName;

If the trial doesn't exist, it will return an empty string, which wouldn't equal
your group.

https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
File content/public/common/content_constants.h (right):

https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
content/public/common/content_constants.h:71:
kStage3DFieldTrialBlacklistedName[];
Nit: I think this fits on the previous line.

https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
content/public/common/content_constants.h:73: kStage3DFieldTrialEnabledName[];
Nit: I think this fits on the previous line.

[jbauman, 2012-09-19 21:06:34]

Ok, fixed.

On 2012/09/19 20:33:16, Alexei Svitkine wrote:
>
https://codereview.chromium.org/10944037/diff/1/chrome/browser/chrome_gpu_uti...
> File chrome/browser/chrome_gpu_util.cc (right):
> 
>
https://codereview.chromium.org/10944037/diff/1/chrome/browser/chrome_gpu_uti...
> chrome/browser/chrome_gpu_util.cc:140: trial->Disable();
> Disabling the trial has the effect of choosing the default group. I think
that's
> not what you want here since you've chosen your default to be "blacklisted".
I'm
> assuming you don't want to blacklist users on Windows 7.
> 
> I think you probably want to make "Enabled" your default group, then disabling
> the trial on non-XP platforms will have the correct behavior. In this
scenario,
> you would add the "blacklisted" group with 100% when creating the trial (which
> happens only under XP).
> 
> Then, Stage3DBlacklisted() call in gpu_data_manager_impl.cc could simply check
> the state of the trial (in the way I suggest below) without needing to also do
> platform checks there. It will work correctly because:
> 
> 1. On XP with no trial config, it will use the client set up code which will
add
> "blacklisted" as 100%, checking for "blacklisted" always works.
> 2. On XP with trial config, checking for "blacklisted" will work per config
> probabilities.
> 3. On non-XP without trial config, trial is not created, so group value will
not
> be "blacklisted", so all is good.
> 4. On non-XP with trial config, trial will be created by config and then
> disabled by this code, taking the default group of "enabled".
> 
> ("trial config" means what we get from the server.)
> 
>
https://codereview.chromium.org/10944037/diff/1/content/browser/gpu/gpu_data_...
> File content/browser/gpu/gpu_data_manager_impl.cc (right):
> 
>
https://codereview.chromium.org/10944037/diff/1/content/browser/gpu/gpu_data_...
> content/browser/gpu/gpu_data_manager_impl.cc:398: trial->group_name() ==
> content::kStage3DFieldTrialBlacklistedName;
> Nit: You can just use:
> 
> return base::FieldTrialList::FindFullName(content::kStage3DFieldTrialName) ==
>       content::kStage3DFieldTrialBlacklistedName;
> 
> If the trial doesn't exist, it will return an empty string, which wouldn't
equal
> your group.
> 
>
https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
> File content/public/common/content_constants.h (right):
> 
>
https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
> content/public/common/content_constants.h:71:
> kStage3DFieldTrialBlacklistedName[];
> Nit: I think this fits on the previous line.
> 
>
https://codereview.chromium.org/10944037/diff/1/content/public/common/content...
> content/public/common/content_constants.h:73: kStage3DFieldTrialEnabledName[];
> Nit: I think this fits on the previous line.

[Alexei Svitkine (slow), 2012-09-19 21:12:36]

LGTM

https://codereview.chromium.org/10944037/diff/5002/chrome/browser/chrome_gpu_...
File chrome/browser/chrome_gpu_util.cc (right):

https://codereview.chromium.org/10944037/diff/5002/chrome/browser/chrome_gpu_...
chrome/browser/chrome_gpu_util.cc:157: bool enabled = !(trial->group() ==
blacklisted_group);
Nit: bool enabled = (trial->group() != blacklisted_group);

[commit-bot: I haz the power, 2012-09-20 23:11:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/10944037/5003

[commit-bot: I haz the power, 2012-09-20 23:11:34]

Failed to apply patch for chrome/browser/chrome_gpu_util.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/chrome_gpu_util.cc
Hunk #1 FAILED at 122.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/chrome_gpu_util.cc.rej

[commit-bot: I haz the power, 2012-09-21 00:05:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/10944037/7002

[commit-bot: I haz the power, 2012-09-21 03:35:55]

Change committed as 157910

[commit-bot: I haz the power, 2012-09-21 18:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbauman@chromium.org/10944037/8004

[commit-bot: I haz the power, 2012-09-21 21:10:19]

Change committed as 158078