Add method to invoke mobile only field trials. Add a field trial that controls rollout of data comp…

Add method to invoke mobile only field trials. Add a field trial that controls rollout of data compression proxy.


BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191714

[SteveT, 2013-03-29 17:40:55]

Looking good! A question inline.

I'm guessing you're just setting up the experiment in this CL, and the proxy
code doesn't quite query the experiment yet?

Did you set up an issue through the template at go/chrome-experiment? You should
make BUG= to that issue.

R+rohit: Hey Rohit, how do you feel about the mobile method here? I know that
iOS currently has its own home for field trials...

https://codereview.chromium.org/13255003/diff/9001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/13255003/diff/9001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_field_trials.cc:143: const
base::FieldTrial::Probability kDataCompressionProxyStable = 0;
So what's the plan? Stable is zero for now, and we can merge a simple patch to
change this if things look good?

[SteveT, 2013-03-29 17:40:58]

Looking good! A question inline.

I'm guessing you're just setting up the experiment in this CL, and the proxy
code doesn't quite query the experiment yet?

Did you set up an issue through the template at go/chrome-experiment? You should
make BUG= to that issue.

R+rohit: Hey Rohit, how do you feel about the mobile method here? I know that
iOS currently has its own home for field trials...

[vagababov, 2013-03-29 17:45:31]

1. Yes, correct. Querying will be done separaterly.
2. Yes, we'll patch in some % if Finch won't be ready by m28. If ready, then
just control from server.
3. iOS is expected to use same experiment as well and, in theory, there may be
more shared experiments in future, so I think it should make sense.
4. Matt is digging up the bug number. Once he finds it, I'll update the cl.

[rohitrao (ping after 24h), 2013-03-29 17:46:59]

I don't have a problem with the mobile method in theory, but in practice we
don't currently compile this file on iOS and looking at the include list we
probably won't be able to.

Two options would work:
1) #ifdef out the body of SetupDesktopFieldTrials() on mobile (and vice versa
for the mobile methods)

2) Move the desktop trial code into a separate helper file.  Then
ChromeBrowserFieldTrials will be a small class that simply calls out to either
the desktop file or the mobile file.

What do you guys think?  (Also, if you want this trial to work on iOS too, then
let me know because right now we don't compile this file at all.)

[vagababov, 2013-03-29 18:02:57]

I guess, I can put exclusion ifdef for iOS around SetupDesktopTrials as well.

[rohitrao (ping after 24h), 2013-03-29 18:09:30]

That may not be enough, because I'm not sure we compile enough on iOS to make
all the various other desktop methods link.

From an iOS perspective, it would be nicest to split the desktop methods out
into a separate file.  That way we could exclude that file without having to
litter #ifdefs everywhere.

If you want I can take a look at doing that (and getting this trial running on
iOS) next week.

[vagababov, 2013-03-29 18:15:08]

Truly, that would be most helpful for me :).
Especially that I won't be implementing the actual code using this trial in
iOS (marq@google.com from our team will, though).
So I can probably check this in and you can do refactoring whenever this is
suitable for you in the next week or so. Beng@ or I will work on Android
usage of this trial in the meantime.

[SteveT, 2013-04-01 19:26:51]

Just one question inline.

https://codereview.chromium.org/13255003/diff/14002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/13255003/diff/14002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.h:38: void SetupMobileFieldTrials();
Do we want to declare/define this at all on desktop platforms? The code doesn't
get used.

That is, should we be wrapping this and the definition and appropriate IFDEFs?

[vagababov, 2013-04-01 19:34:55]

https://codereview.chromium.org/13255003/diff/14002/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_field_trials.h (right):

https://codereview.chromium.org/13255003/diff/14002/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_field_trials.h:38: void SetupMobileFieldTrials();
Makes sense. Wrapped declarations and definitions in the ifdef guards as well.

[SteveT, 2013-04-01 19:44:31]

lgtm - Thanks for your patience!

Hopefully Rohit's future refactoring for iOS will remove some of those guards,
which we prefer to avoid.

[commit-bot: I haz the power, 2013-04-01 20:34:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vagababov@google.com/13255003/21001

[commit-bot: I haz the power, 2013-04-01 22:42:50]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-01 22:48:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vagababov@google.com/13255003/21001

[commit-bot: I haz the power, 2013-04-02 00:17:22]

Change committed as 191714