Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)

Committed: https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f
Cr-Commit-Position: refs/heads/master@{#426868}

[pkotwicz, 2016-10-18 19:09:44]

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

[pkotwicz, 2016-10-18 19:09:44]

Dominick, can you please take a look?

[dominickn, 2016-10-19 05:54:13]

In the CL description, can you clarify that

  - the previous WebAPK update failed; AND
  - A WebAPK update is no longer needed

And that this CL also adds tests for those scenarios.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:38:
private static final String TAG = "WebApkUpdateManager";
TAG is usually listed first in the class - put it back?

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:167:
if (shellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION) {
Nit: inline the return

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:173:
if (sinceLastCheckDurationMs >= FULL_CHECK_UPDATE_INTERVAL) {
Nit: inline the return

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:187:
private static void trackUpdateSuccessInWebappDataStorage(String id, boolean
success) {
Make this method take a WebappDataStorage as an argument instead of id? It's
already a local variable in one of the callsites, and you can call
WebappRegistry.getInstance().getWebappDataStorage in the other one.

Also, maybe rename this method recordUpdate(WebappDataStorage storage, boolean
success). It's a bit weird calling it trackUpdateSuccess when you might be
recording a failure.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:200:
private static void onBuiltWebApk(String id, final boolean success) {
Nit: does |success| have to be final?

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:9:
import org.junit.Assert;
Perhaps import org.junit.Assert.assertTrue, assertEquals, and assertFalse here
to save you lots of Assert. in the test

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:125:
private void resetStorage(WebappDataStorage storage) {
Is this resetting the WebappDataStorage, or is it setting a succeeded update?
Resetting implies setting everything to default/zero values.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:201:
* - The previous WebAPK update succeeded.
Nit: AND

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:222:
* - The previous WebAPK update failed.
AND

[pkotwicz, 2016-10-19 18:02:41]

Description was changed from

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes two corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
==========

to

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes two corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
==========

[pkotwicz, 2016-10-19 20:43:05]

Description was changed from

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes two corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
==========

to

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
==========

[pkotwicz, 2016-10-19 20:43:42]

Dominick can you please take another look?

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:38:
private static final String TAG = "WebApkUpdateManager";
On 2016/10/19 05:54:13, dominickn wrote:
> TAG is usually listed first in the class - put it back?

Done.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:167:
if (shellApkVersion < WebApkVersion.CURRENT_SHELL_APK_VERSION) {
On 2016/10/19 05:54:13, dominickn wrote:
> Nit: inline the return

Done.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:173:
if (sinceLastCheckDurationMs >= FULL_CHECK_UPDATE_INTERVAL) {
On 2016/10/19 05:54:13, dominickn wrote:
> Nit: inline the return

Done.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:187:
private static void trackUpdateSuccessInWebappDataStorage(String id, boolean
success) {
Renamed this function to recordUpdateInWebappDataStorage(). I am still passing
in an id argument because I can't find the call site where WebappDataStorage is
a local variable

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:200:
private static void onBuiltWebApk(String id, final boolean success) {
On 2016/10/19 05:54:13, dominickn wrote:
> Nit: does |success| have to be final?

Done.

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
File
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java
(right):

https://codereview.chromium.org/2430773002/diff/1/chrome/android/junit/src/or...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:125:
private void resetStorage(WebappDataStorage storage) {
I removed this function.

Thank you for your comment. Your comment made me realize that when we launch a
WebAPK for the first time we consider the "previous WebAPK update" as having
failed.

[dominickn, 2016-10-19 21:38:44]

lgtm

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2430773002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:187:
private static void trackUpdateSuccessInWebappDataStorage(String id, boolean
success) {
On 2016/10/19 20:43:41, pkotwicz wrote:
> Renamed this function to recordUpdateInWebappDataStorage(). I am still passing
> in an id argument because I can't find the call site where WebappDataStorage
is
> a local variable

There was a method signature in the middle of what I thought was a continuous
method. Acknowledged.

https://codereview.chromium.org/2430773002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2430773002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:182:
WebappDataStorage storage, int shellApkVersion, boolean previousUpdateSucceeded)
{
This could just check mPreviousUpdatedSucceeded for consistency with
onUpgradeNeededCheckFinished.

[pkotwicz, 2016-10-20 21:47:42]

https://codereview.chromium.org/2430773002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java
(right):

https://codereview.chromium.org/2430773002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:182:
WebappDataStorage storage, int shellApkVersion, boolean previousUpdateSucceeded)
{
I am not using |mPreviousUpdatedSucceeded| because I want to make this method
static once currentTimeMillis() is also static

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

The CQ bit was checked by pkotwicz@chromium.org

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

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/2430773002/#ps60001
(title: "Merge branch 'master' into update_fail00")

[commit-bot: I haz the power, 2016-10-20 22:03: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-10-20 22:27:37]

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

[commit-bot: I haz the power, 2016-10-20 22:27:38]

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-10-21 00:27:25]

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

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

mlamouri@ for OWNERS rubberstamp for
	chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
(dfalcantara@ is away)

[mlamouri (slow - plz ping), 2016-10-21 10:19:22]

stampy lgtm

[pkotwicz, 2016-10-21 14:08:41]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-10-21 14:08:41]

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/2430773002/#ps80001
(title: "Merge branch 'master' into update_fail00")

[commit-bot: I haz the power, 2016-10-21 14:08:55]

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-21 14:29:41]

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

[commit-bot: I haz the power, 2016-10-21 14:29:42]

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-10-21 18:23:11]

Description was changed from

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
==========

to

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)
==========

[pkotwicz, 2016-10-21 18:23:11]

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

[pkotwicz, 2016-10-21 18:23:20]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-10-21 18:24:14]

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-21 19:54:36]

Description was changed from

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)
==========

to

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)
==========

[commit-bot: I haz the power, 2016-10-21 19:54:37]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-21 20:00:07]

Description was changed from

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)
==========

to

==========
Fix WebappDataStorage#updateDidLastWebApkUpdateRequestSucceed() corner cases

This CL fixes three corner cases:
1) This CL sets the "WebAPK update" as having succeeded if:
  - the previous WebAPK update failed
  AND
  - A WebAPK update is no longer needed
  Doing this is important because the is-webapk-update-needed check is
  done much more frequently if the "previous WebAPK update failed."
2) This CL ignores whether the "WebAPK update" has previously succeeded if
   there have been no update attempts.
   The goal is to wait at least WebApkUpdateManager#FULL_CHECK_UPDATE_INTERVAL
   since the WebAPK's first launch before checking for updates.
3) This CL sets the "WebAPK update" as having failed if Chrome is killed
  after the time that the WebAPK update is requested but before
  WebApkUpdateManager#onBuiltWebApk() is called.

BUG=639536
TEST=WebApkManagerTest.*
R=dominickn, mlamouri
TBR=tedchoc (For new dependencies to chrome_junit_tests in BUILD.gn)

Committed: https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f
Cr-Commit-Position: refs/heads/master@{#426868}
==========

[commit-bot: I haz the power, 2016-10-21 20:00:08]

Patchset 5 (id:??) landed as
https://crrev.com/62ed2afce28cc27f6acd19115a7715f41dcc5e8f
Cr-Commit-Position: refs/heads/master@{#426868}