Don't update unbound WebApks.

Don't update unbound WebApks.

Since we didn't install them, we can't update them.
BUG=714740
Review-Url: https://codereview.chromium.org/2847973002
Cr-Commit-Position: refs/heads/master@{#468313}
Committed: https://chromium.googlesource.com/chromium/src/+/03d4670df994c2b4cdc96c3f2ee8903f2d3c8ca2

[Yaron, 2017-04-28 15:00:04]

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

[commit-bot: I haz the power, 2017-04-28 15:00:50]

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, 2017-04-28 15:13:25]

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

[commit-bot: I haz the power, 2017-04-28 15:13:25]

Dry run: 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...)

[Yaron, 2017-04-28 16:52:05]

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

[pkotwicz, 2017-04-28 18:32:13]

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

[pkotwicz, 2017-04-28 18:32:14]

Just test nits

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:264:
manifestData.iconUrlToMurmur2HashMap);
Nit: Use an arbitrary package name which is local to this function.
WebApkUpdateManager#needsUpdate() does not care about the package name

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:284:
private static void updateIfNeeded(String packageName, WebApkUpdateManager
updateManager) {
Can this function extract the package name from WebApkUpdateManager's
WebappDataStorage?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:590:
fetchedManifestData.scopeUrl += "/foo";
|fetchedManifestData| is unused?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:595:
assertFalse(updateManager.updateRequested());
Nit: We can probably remove the assert on line 595

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:713:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
You can pass |storage| here?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:724:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
You can pass |storage| here?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:739:
WebappRegistry.getInstance().getWebappDataStorage(getWebApkId(WEBAPK_PACKAGE_NAME));
This can be replaced with a call to getStorage() ?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:743:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
Can you pass |storage| to the TestWebApkUpdateManager constructor?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/s...
File
chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java:35:
protected static final String WEBAPK_PACKAGE_NAME =
"org.chromium.webapk.test_package";
Why is this variable protected?

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/t...
File
chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/t...
chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java:20:
* @param packageName The package for the to Register
Nit: "the to Register" -> "to register"

[Yaron, 2017-04-28 20:04:38]

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

[Yaron, 2017-04-28 20:04:46]

thanks, Peter!

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:264:
manifestData.iconUrlToMurmur2HashMap);
On 2017/04/28 18:32:13, pkotwicz wrote:
> Nit: Use an arbitrary package name which is local to this function.
> WebApkUpdateManager#needsUpdate() does not care about the package name

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:284:
private static void updateIfNeeded(String packageName, WebApkUpdateManager
updateManager) {
On 2017/04/28 18:32:13, pkotwicz wrote:
> Can this function extract the package name from WebApkUpdateManager's
> WebappDataStorage?

adds logic to this test class which I'm hoping to avoid. I prefer simple
overriden functions

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:590:
fetchedManifestData.scopeUrl += "/foo";
On 2017/04/28 18:32:13, pkotwicz wrote:
> |fetchedManifestData| is unused?

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:595:
assertFalse(updateManager.updateRequested());
On 2017/04/28 18:32:13, pkotwicz wrote:
> Nit: We can probably remove the assert on line 595

Doesn't seem like a big deal and is more explicit IMHO. Since you marked NIT I
will keep

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:713:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
On 2017/04/28 18:32:13, pkotwicz wrote:
> You can pass |storage| here?

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:724:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
On 2017/04/28 18:32:14, pkotwicz wrote:
> You can pass |storage| here?

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:739:
WebappRegistry.getInstance().getWebappDataStorage(getWebApkId(WEBAPK_PACKAGE_NAME));
On 2017/04/28 18:32:14, pkotwicz wrote:
> This can be replaced with a call to getStorage() ?

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:743:
new TestWebApkUpdateManager(getStorage(WEBAPK_PACKAGE_NAME));
On 2017/04/28 18:32:14, pkotwicz wrote:
> Can you pass |storage| to the TestWebApkUpdateManager constructor?

Done.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/s...
File
chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/s...
chrome/android/webapk/shell_apk/junit/src/org/chromium/webapk/shell_apk/MainActivityTest.java:35:
protected static final String WEBAPK_PACKAGE_NAME =
"org.chromium.webapk.test_package";
On 2017/04/28 18:32:14, pkotwicz wrote:
> Why is this variable protected?

Cause of the @Config above and internally using subclassing.

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/t...
File
chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java
(right):

https://codereview.chromium.org/2847973002/diff/20001/chrome/android/webapk/t...
chrome/android/webapk/test/src/org/chromium/webapk/test/WebApkTestHelper.java:20:
* @param packageName The package for the to Register
On 2017/04/28 18:32:14, pkotwicz wrote:
> Nit: "the to Register" -> "to register"

Done.

[commit-bot: I haz the power, 2017-04-28 20:05:30]

Dry run: CQ is trying da patch.

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

[Xi Han, 2017-04-28 20:24:31]

lgtm with nits.

https://codereview.chromium.org/2847973002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2847973002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:259:
return WebApkInfo.create(getWebApkId("org.random.webapk"), "", false /*
forceNavigation */,
Please declare a local variable for the package name, since the same string are
used twice.

[commit-bot: I haz the power, 2017-04-28 21:36:36]

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

[commit-bot: I haz the power, 2017-04-28 21:36:37]

Dry run: This issue passed the CQ dry run.

[Yaron, 2017-05-01 13:38:26]

The CQ bit was checked by yfriedman@chromium.org

[Yaron, 2017-05-01 13:38:26]

The patchset sent to the CQ was uploaded after l-g-t-m from hanxi@chromium.org
Link to the patchset: https://codereview.chromium.org/2847973002/#ps60001
(title: "Don't upadte unbound WebApks.")

[Yaron, 2017-05-01 13:38:33]

thanks

https://codereview.chromium.org/2847973002/diff/40001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2847973002/diff/40001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:259:
return WebApkInfo.create(getWebApkId("org.random.webapk"), "", false /*
forceNavigation */,
On 2017/04/28 20:24:31, Xi Han wrote:
> Please declare a local variable for the package name, since the same string
are
> used twice.

Done.

[commit-bot: I haz the power, 2017-05-01 13:38:35]

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, 2017-05-01 14:19:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493645906149950,
"parent_rev": "6474e28997d82bd16590b24c0371671b525fdd5a", "commit_rev":
"03d4670df994c2b4cdc96c3f2ee8903f2d3c8ca2"}

[commit-bot: I haz the power, 2017-05-01 14:19:38]

Description was changed from

==========
Don't update unbound WebApks.

Since we didn't install them, we can't update them.
BUG=714740
==========

to

==========
Don't update unbound WebApks.

Since we didn't install them, we can't update them.
BUG=714740

Review-Url: https://codereview.chromium.org/2847973002
Cr-Commit-Position: refs/heads/master@{#468313}
Committed:
https://chromium.googlesource.com/chromium/src/+/03d4670df994c2b4cdc96c3f2ee8...
==========

[commit-bot: I haz the power, 2017-05-01 14:19:39]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/03d4670df994c2b4cdc96c3f2ee8...