Add field trial for the WebAPK server URL

Add field trial for the WebAPK server URL

BUG=651561
Committed: https://crrev.com/b13981b5f196a6d6643fbc19ca266788c1acb3cd
Cr-Commit-Position: refs/heads/master@{#427894}

[pkotwicz, 2016-10-22 02:20:28]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-10-22 02:20:28]

Dominick, can you please take a look? I am reviving
https://codereview.chromium.org/2380053003/

Sam's been bugging me about this for a few weeks now and I am finally caving in.
The goal of the field trial is to enable testing the server internally prior to
the server being ready to be used externally.

[dominickn, 2016-10-24 00:32:37]

https://codereview.chromium.org/2441203002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2441203002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:66: return
GURL(command_line->GetSwitchValueASCII(switches::kWebApkServerUrl));
For safety, should you test whether this GURL is_valid() and return the empty
GURL if so? And then don't initiate the network request if the URL is no good?

https://codereview.chromium.org/2441203002/diff/1/chrome/common/chrome_featur...
File chrome/common/chrome_features.cc (right):

https://codereview.chromium.org/2441203002/diff/1/chrome/common/chrome_featur...
chrome/common/chrome_features.cc:171: const base::Feature kWebApk{"WebAPK",
base::FEATURE_DISABLED_BY_DEFAULT};
Nit: call this "WebApks" for consistency in this file.

https://codereview.chromium.org/2441203002/diff/1/chrome/common/chrome_featur...
File chrome/common/chrome_features.h (right):

https://codereview.chromium.org/2441203002/diff/1/chrome/common/chrome_featur...
chrome/common/chrome_features.h:100: #if defined(OS_ANDROID)
Nit: probably should call this kWebApks.

https://codereview.chromium.org/2441203002/diff/1/chrome/common/chrome_featur...
chrome/common/chrome_features.h:101: extern const base::Feature kWebApk;
If you're adding this, you should look at revamping the existing kill-switch and
enabling flag for WebAPKs. You can enable / disable WebAPKs with
--enable-features=WebAPKs or --disable-features=WebAPKs, and you can use that as
a kill switch in Finch as well. That makes the existing enabling flag and kill
switch a bit redundant.

if (base::FeatureList::IsEnabled(kWebApks)) will check whether the feature is on
or off, so that should suffice for enabling and killing it

On the command line, you can pass parameters like this:

 --enable-features="WebApks<FieldTrialName"
--force-fieldtrials=FieldTrialName/GroupName
--force-fieldtrial-params=FieldTrialName.GroupName:server_url/server_url_goes_here.

(You'll need to URL escape the URL in the parameter, but I think that's the only
gotcha).

Then you can remove the server URL command line switch entirely, and just query
the variations like you have, because that command line will populate the
variation correctly.

I had a look at ChromeWebApkHost, and I think that the native side should take
care of querying the feature / etc., whilst the Java side will persist whatever
the value is in prefs afterwards.

Thoughts? I know you've got a bunch on right now, but this will greatly tidy up
the command line flag / Finch situation for WebAPKs. It'll remove a Finch kill
switch and the current server URL command line flag in favour of just using the
Feature architecture, so you don't have this duplication. E.g. the semantics of
what happens when you have the Finch feature on, but the kill switch on and vice
versa are confusing. Unifying under a single Feature with parameters makes it
much tidier.

[pkotwicz, 2016-10-24 04:33:42]

Dominick, can you please take another look?

I have made the "Finch kill switch" use the "feature architecture" in
https://codereview.chromium.org/2441243003/

Thank you very much for your suggestions. Using the "feature architecture" for
the kill switch and getting rid of the command line flag makes things much
cleaner.

https://codereview.chromium.org/2441203002/diff/1/chrome/browser/android/weba...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2441203002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:66: return
GURL(command_line->GetSwitchValueASCII(switches::kWebApkServerUrl));
Good point.

UrlFetcher crashes if given an invalid URL

[dominickn, 2016-10-24 04:43:10]

lgtm, this is looking much cleaner. :)

[pkotwicz, 2016-10-26 00:27:37]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-10-26 00:27:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org
Link to the patchset: https://codereview.chromium.org/2441203002/#ps40001
(title: "Merge branch 'master' into webapk_finch")

[commit-bot: I haz the power, 2016-10-26 00:28:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 01:02:27]

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

[commit-bot: I haz the power, 2016-10-26 01:02:27]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dominickn, 2016-10-26 01:05:52]

It might be good for you to get into the habit of doing a CQ dry run (with git
cl try or ticking the box in code review) before sending code to me to review.
Then I can look at it knowing that it will pass the tests.

[pkotwicz, 2016-10-26 22:55:09]

The CQ bit was checked by pkotwicz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 22:55:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 23:45:41]

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

[commit-bot: I haz the power, 2016-10-26 23:45:42]

Dry run: This issue passed the CQ dry run.

[pkotwicz, 2016-10-27 00:20:50]

Dominick, can you please take another look?

I will try to be more diligent in running tests before posting CLs for code
review. I already do this but sometimes I forget a test suite. It is hard to run
the try jobs for most of the CLs that I have been posting this week because the
CLs are based on others which have not landed yet. (For instance, this is why
the "CQ dry run" failed for https://codereview.chromium.org/2435383002/)

[dominickn, 2016-10-27 00:36:26]

still lgtm

FYI: Rietveld handles "dependent CLs". If you have your branches set up
correctly, and upload in the order of dependency (oldest CL, then dependent CL,
then dependent CL), then Rietveld will pick up that it needs to patch in another
CL. 

$ git new-branch branch1
# do some stuff and commit
$ git cl upload
$ git cl try

$ git new-branch branch2
# want this to be dependent on branch1
$ git branch --set-upstream-to=branch1
$ git rebase
# now this has branch1 as its parent
# do some stuff and commit
$ git cl upload
$ git cl try # this just works, Rietveld will apply branch1 then branch2 on top

https://codereview.chromium.org/2441203002/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2441203002/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer_unittest.cc:7: #include <jni.h>
Nit: #include <map>

[pkotwicz, 2016-10-27 00:45:47]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-10-27 00:46:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 00:51:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-27 00:54:06]

Description was changed from

==========
Add field trial for the WebAPK server URL

BUG=651561
==========

to

==========
Add field trial for the WebAPK server URL

BUG=651561

Committed: https://crrev.com/b13981b5f196a6d6643fbc19ca266788c1acb3cd
Cr-Commit-Position: refs/heads/master@{#427894}
==========

[commit-bot: I haz the power, 2016-10-27 00:54:07]

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