Specify the WebAPK server URL in Chromium source code

Specify the WebAPK server URL in Chromium source code

BUG=668001
Committed: https://crrev.com/ba7e85ba518389da9df8f434a85ef536f35d3b74
Cr-Commit-Position: refs/heads/master@{#434758}

[pkotwicz, 2016-11-23 04:11:37]

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

[pkotwicz, 2016-11-23 04:11:37]

Glann, can you please take a look?

I am not using the other URL because the other URL still fails for me.

[hartmanng, 2016-11-23 14:52:57]

The code looks good, but why are we getting rid of the Variation? Couldn't that
still be useful to us if we need to shut off the experiment or something?

The prod URL is still not working, but we're still hoping to get it going soon -
it sort of depends on another team at the moment, but we're hoping to get it
resolved within a week or so. If we hard-code the test URL now, we'll just need
to land another CL to switch it to the prod URL in a few days.

[pkotwicz, 2016-11-23 15:44:49]

I am not removing the variations kill switch. I am just not specifying the
server URL via variations anymore. The variations team allowed us to do this on
the condition of it being a temporary thing

[hartmanng, 2016-11-23 15:47:50]

On 2016/11/23 15:44:49, pkotwicz wrote:
> I am not removing the variations kill switch. I am just not specifying the
> server URL via variations anymore. The variations team allowed us to do this
on
> the condition of it being a temporary thing

Ah, I see. Ok, in that case LGTM. Like I said, we'll want to submit another CL
likely within a week to change this URL to the prod one. If you're ok with that,
then go ahead and land this - otherwise you can wait a bit longer and just kill
the variations once the other URL is fully up and running.

[pkotwicz, 2016-11-24 04:24:44]

pkotwicz@chromium.org changed reviewers:
+ hanxi@google.com

[pkotwicz, 2016-11-24 04:24:44]

Xi can you please take a look?

No e-mail from hartmanng@ about the prod server URL actually working

[Xi Han, 2016-11-24 14:27:49]

hanxi@chromium.org changed reviewers:
+ hanxi@chromium.org

[Xi Han, 2016-11-24 14:27:50]

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

https://codereview.chromium.org/2526713002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:21: #include
"chrome/browser/android/chrome_feature_list.h"
Do you still need this include?

https://codereview.chromium.org/2526713002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:27: #include
"components/variations/variations_associated_data.h"
Do you still need this include? I see you removed it in the unit tests, and some
more. Please clean up here if I missed anything:)

https://codereview.chromium.org/2526713002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:41:
"https://test-webapk.sandbox.googleapis.com/v1/webApks/?alt=proto&key=AIzaSyAoI6v-F31-3t9NunLYEiKcPIqgTJIUZBw";
Is the test server public? I am wondering whether is it secure to put this URL
public? Maybe wait until the prod server is public and add that URL directly?

[Xi Han, 2016-11-24 14:28:11]

Description was changed from

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001
==========

to

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001
==========

[Xi Han, 2016-11-24 14:28:11]

hanxi@chromium.org changed reviewers:
- hanxi@google.com

[hartmanng, 2016-11-24 14:42:01]

On 2016/11/24 04:24:44, pkotwicz wrote:
> Xi can you please take a look?
> 
> No e-mail from hartmanng@ about the prod server URL actually working

Sorry I didn't get back to you last night. I'd go ahead and just land this
version for now... :/

[pkotwicz, 2016-11-25 01:29:20]

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

https://codereview.chromium.org/2526713002/diff/1/chrome/browser/android/weba...
chrome/browser/android/webapk/webapk_installer.cc:41:
"https://test-webapk.sandbox.googleapis.com/v1/webApks/?alt=proto&key=AIzaSyAoI6v-F31-3t9NunLYEiKcPIqgTJIUZBw";
A user can currently figure out what the URL is using Charles proxy or Wireshark

[pkotwicz, 2016-11-25 01:29:48]

Xi, can you please take another look? I think that I addressed all of your
comments

[Xi Han, 2016-11-25 15:21:11]

lgtm

[pkotwicz, 2016-11-25 15:22:47]

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

[pkotwicz, 2016-11-25 15:22:48]

Dominick for OWNERS

[dominickn, 2016-11-25 22:27:41]

Just for my understanding, why didn't the metrics team want you to specify the
URL via Finch?

Also, is the WebAPK team comfortable with having to make a binary change (and
therefore up to a 12+ week cycle to stable) if the server URL has to change?

https://codereview.chromium.org/2526713002/diff/20001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2526713002/diff/20001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:63: return GURL(url_string);
Should you check whether the GURL is valid here, in case of a malformed command
line input?

[pkotwicz, 2016-11-28 02:15:25]

The metrics team did not like specifying the URL via Finch because of "We
generally only allow 100% rollouts once the feature is approved for stable. Why
are we going to 100% here?". As far as I know there isn't any precedent for
hiding a server URL behind Finch

Yes, we are ok with the 12+ week cycle for changes to the URL

[pkotwicz, 2016-11-28 02:29:41]

Dominick, can you please take another look?

[dominickn, 2016-11-28 02:38:13]

On 2016/11/28 02:29:41, pkotwicz wrote:
> Dominick, can you please take another look?

Please run the trybots and then I'll review.

[dominickn, 2016-11-28 04:00:03]

lgtm

https://codereview.chromium.org/2526713002/diff/40001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2526713002/diff/40001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:39:
"https://test-webapk.sandbox.googleapis.com/v1/webApks/?alt=proto&key=AIzaSyAoI6v-F31-3t9NunLYEiKcPIqgTJIUZBw";
I'm assuming that hartmanng's lgtm is a sign that exposing this key is fine.

[pkotwicz, 2016-11-28 14:42:33]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-11-28 14:42:34]

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

[commit-bot: I haz the power, 2016-11-28 14:42:47]

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-11-28 14:49:42]

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

[commit-bot: I haz the power, 2016-11-28 14:49:43]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pkotwicz, 2016-11-28 21:02:35]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-11-28 21:02:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, hanxi@chromium.org, hartmanng@chromium.org
Link to the patchset: https://codereview.chromium.org/2526713002/#ps60001
(title: "Merge branch 'master' into no_finch")

[commit-bot: I haz the power, 2016-11-28 21:04:15]

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-11-28 22:54:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480366955457570,
"parent_rev": "6bc208198e0d3f0b4c09475368cdf1c59a3e3050", "commit_rev":
"9b9e43f131836cba62ecc959c3b1575c72097fcd"}

[commit-bot: I haz the power, 2016-11-28 22:55:18]

Description was changed from

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001
==========

to

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001
==========

[commit-bot: I haz the power, 2016-11-28 22:55:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 22:58:12]

Description was changed from

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001
==========

to

==========
Specify the WebAPK server URL in Chromium source code

BUG=668001

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

[commit-bot: I haz the power, 2016-11-28 22:58:13]

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