Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
335223003:
Add trial for DirectWrite, and reorder some initialization so it can take effect (Closed)
|
Created:
6 years, 6 months ago by scottmg Modified:
6 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, eae, ananta Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd trial for DirectWrite, and reorder some initialization so it can take effect
You can set --force-fieldtrials="DirectWrite/Disabled/" to turn DirectWrite off.
R=cpu@chromium.org, jamesr@chromium.org, asvitkine@chromium.org
BUG=385778
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278238
Patch Set 1 #
Total comments: 2
Patch Set 2 : invert sense #
Total comments: 9
Patch Set 3 : review #Patch Set 4 : nacl_win64 #
Total comments: 2
Patch Set 5 : comment #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/335223003/diff/1/chrome/browser/chrome_browse... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/335223003/diff/1/chrome/browser/chrome_browse... chrome/browser/chrome_browser_field_trials.cc:63: base::FieldTrialList::FindValue("DirectWrite"); this seems to be necessary so that the command line setting makes it to the renderer. https://codereview.chromium.org/335223003/diff/1/content/renderer/renderer_ma... File content/renderer/renderer_main.cc (right): https://codereview.chromium.org/335223003/diff/1/content/renderer/renderer_ma... content/renderer/renderer_main.cc:202: platform.PlatformInitialize(); Needs to be moved so that trials can take effect here when setting up sandbox.
asvitkie: chrome/browser/chrome_browser_field_trials.cc jamesr: content/renderer/renderer_main.cc cpu: content/common/sandbox_win.cc
https://codereview.chromium.org/335223003/diff/20001/content/renderer/rendere... File content/renderer/renderer_main.cc (right): https://codereview.chromium.org/335223003/diff/20001/content/renderer/rendere... content/renderer/renderer_main.cc:201: // PlatformInitialize uses FieldTrials, so this must happen later. how does PlatformInitialize depend on FieldTrials? Does FieldTrials manipulate the current command line or something ridiculous like that? I don't see anything in PlatformInitialize that asks the field trials system any questions
On 2014/06/17 21:04:20, jamesr wrote: > https://codereview.chromium.org/335223003/diff/20001/content/renderer/rendere... > File content/renderer/renderer_main.cc (right): > > https://codereview.chromium.org/335223003/diff/20001/content/renderer/rendere... > content/renderer/renderer_main.cc:201: // PlatformInitialize uses FieldTrials, > so this must happen later. > how does PlatformInitialize depend on FieldTrials? Does FieldTrials manipulate > the current command line or something ridiculous like that? I don't see > anything in PlatformInitialize that asks the field trials system any questions In the code I added in content/common/sandbox_win.cc called from https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... it needs to know if the trial is in effect. But before the change to renderer_main here, the FieldTrials are not yet initialized.
Ah, I see it. lgtm
https://codereview.chromium.org/335223003/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/335223003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_field_trials.cc:63: base::FieldTrialList::FindValue("DirectWrite"); This is no longer necessary to do. You can mark the study as "starts_active: true" in the server-side config to achieve the same effect. https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:576: if (command_line.HasSwitch(switches::kDisableDirectWrite)) Do you want to see data from users who've manually toggled their flags as separate groups on the dashboard? If so, you can follow the pattern suggested at go/finch-and-flags to get that effect. If you don't care, then feel free to ignore as this will be fine otherwise. But it might be a good signal to see if people are manually disabling the experimental group. (If you do follow that suggested pattern, you should probably put the DPI scale check above this, just below the version check.) https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:586: return group_name != "Disabled"; Instead of checking for != "Disabled", check for "Enabled" instead. That way, you can have a group called "Control" that exercises the default behavior.
https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:586: return group_name != "Disabled"; On 2014/06/17 21:30:29, Alexei Svitkine wrote: > Instead of checking for != "Disabled", check for "Enabled" instead. That way, > you can have a group called "Control" that exercises the default behavior. Sorry, I just read your experiment description and you want the default to be enabled, so the above makes sense now. Please ignore this comment.
https://codereview.chromium.org/335223003/diff/20001/chrome/browser/chrome_br... File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/335223003/diff/20001/chrome/browser/chrome_br... chrome/browser/chrome_browser_field_trials.cc:63: base::FieldTrialList::FindValue("DirectWrite"); On 2014/06/17 21:30:29, Alexei Svitkine wrote: > This is no longer necessary to do. You can mark the study as "starts_active: > true" in the server-side config to achieve the same effect. Done. https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:576: if (command_line.HasSwitch(switches::kDisableDirectWrite)) On 2014/06/17 21:30:29, Alexei Svitkine wrote: > Do you want to see data from users who've manually toggled their flags as > separate groups on the dashboard? If so, you can follow the pattern suggested at > go/finch-and-flags to get that effect. If you don't care, then feel free to > ignore as this will be fine otherwise. > > But it might be a good signal to see if people are manually disabling the > experimental group. > > (If you do follow that suggested pattern, you should probably put the DPI scale > check above this, just below the version check.) Thanks, I think we don't care too much as there's many reasons (aesthetic, preference, etc.) that people might force it off. https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:586: return group_name != "Disabled"; On 2014/06/17 21:30:29, Alexei Svitkine wrote: > Instead of checking for != "Disabled", check for "Enabled" instead. That way, > you can have a group called "Control" that exercises the default behavior. Done. https://codereview.chromium.org/335223003/diff/20001/content/common/sandbox_w... content/common/sandbox_win.cc:586: return group_name != "Disabled"; On 2014/06/17 22:02:15, Alexei Svitkine wrote: > On 2014/06/17 21:30:29, Alexei Svitkine wrote: > > Instead of checking for != "Disabled", check for "Enabled" instead. That way, > > you can have a group called "Control" that exercises the default behavior. > > Sorry, I just read your experiment description and you want the default to be > enabled, so the above makes sense now. Please ignore this comment. Yeah, ok thanks. Seems like it'd work either way, depending on the server-side. I'm still working through that CL.
lgtm % nit https://codereview.chromium.org/335223003/diff/100001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/335223003/diff/100001/content/common/sandbox_... content/common/sandbox_win.cc:585: // Otherwise, Finch. Nit: Finch is a codename, change to "Otherwise, check the field trial." or just omit.
https://codereview.chromium.org/335223003/diff/100001/content/common/sandbox_... File content/common/sandbox_win.cc (right): https://codereview.chromium.org/335223003/diff/100001/content/common/sandbox_... content/common/sandbox_win.cc:585: // Otherwise, Finch. On 2014/06/18 17:38:27, Alexei Svitkine wrote: > Nit: Finch is a codename, change to "Otherwise, check the field trial." or just > omit. Done.
lgtm
The CQ bit was checked by scottmg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scottmg@chromium.org/335223003/110001
Message was sent while issue was closed.
Change committed as 278238 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
