Update WebAPK after the user closes WebAPK

Update WebAPK after the user closes WebAPK

This CL hooks up updating WebAPKs to BackgroundTaskScheduler.
This has the following benefits:
- Preventing WebAPKs from being installed as a result of an update after a user
  uninstalls a WebAPK
- Making WebAPKs always occur when the WebAPK is not running. Previously,
  updates were triggered as a result of WebApkActivity#onStop(). If the
  onStop()-triggered update failed 3 times, the WebAPK switched to
  triggering updates on launch. (on update completion the running WebAPK would
  crash).

This CL also:
- Renames the --check-for-web-manifest-update-on-startup to --debug-webapk-updates

BUG=713655

[pkotwicz, 2017-06-20 02:42:07]

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

[pkotwicz, 2017-06-20 02:42:08]

Xi, can you please take a look? This CL depends on
https://codereview.chromium.org/2943913002/

[Xi Han, 2017-06-20 19:18:01]

Thanks for fixing this!

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java:58:
public static WebApkActivity getRunningActivity(String webApkPackageName) {
Does this function needs to be declared here? Maybe move it to WebApkUpdateTask
as a private function?

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:214:
return;
Do you need to call the callback at least once?

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:283:
if (mDebugMode) {
Is there any particular reason to move this check after the package name prefix
check?

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateTask.java:84:
callback.taskFinished(false);
I have a hard time to understand why passing "false" here. Please update to:
callback.taskFinished(false /* needsReschedule */);

Besides, is the |mRunCallback| necessary? Don't you need to call the callback
anyway to release the wakelock
(https://cs.chromium.org/chromium/src/components/background_task_scheduler/and...

[fgorski1, 2017-08-08 16:46:09]

fgorski@google.com changed reviewers:
+ fgorski@google.com

[fgorski1, 2017-08-08 16:46:10]

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:195:
info.webApkPackageName(), info.manifestStartUrl(), info.shortName());
Do you in general only allow a single APK? Or would there be multiple APKs that
you would want to update that way?

If the latter is the case, this will not work.
Extras don't differentiate the tasks, JOB_ID does.

This means that in situation where you have 2 APKs, your second task will
overwrite the first one.

My recommendation is to have a generic upgrade task and a separate list of web
apks that need updating. When the upgrade task is run, it checks and try to
update them.

If by the time the task is done there is still work left, you reschedule.

Let me know if I misunderstood your code however.

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateTask.java:84:
callback.taskFinished(false);
On 2017/06/20 19:18:01, Xi Han wrote:
> I have a hard time to understand why passing "false" here. Please update to:
> callback.taskFinished(false /* needsReschedule */);
> 
> Besides, is the |mRunCallback| necessary? Don't you need to call the callback
> anyway to release the wakelock
>
(https://cs.chromium.org/chromium/src/components/background_task_scheduler/and...

I don't think there is a risk here, because onStopTask has relationship between
onStopTask and the wakelock.
I agree that we can do here without mRunCallback.

[pkotwicz, 2017-09-08 03:42:34]

Xi and fgorski@ can you please take another look? Sorry for the long delay

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

https://codereview.chromium.org/2948713002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateManager.java:195:
info.webApkPackageName(), info.manifestStartUrl(), info.shortName());
I have changed WebApkUpdateTask to be able to deal with multiple pending updates

[Xi Han, 2017-09-11 15:24:24]

Mostly nits!

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java
(right):

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:61:
public static final String HISTOGRAM_UPDATE_REQUEST_SENT =
Nit: this isn't necessary now. Please also update histograms.xml.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:64:
public static final String HISTOGRAM_UPDATE_REQUEST_QUEUED =
"WebApk.Update.RequestQueued";
Nit: this isn't necessary now. Please also update histograms.xml.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:75:
public static void recordUpdateRequestSent(int type) {
You can remove this function now.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/metrics/WebApkUma.java:86:
public static void recordUpdateRequestQueued(int times) {
You can remove this too.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateTask.java
(right):

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkUpdateTask.java:25:
* Handles servicing of background WebAPK update requests coming via
background_task_scheduler
It is worthy mentioning that: the scheduled task will update all WebAPKs that
are out-of-date. It is possible that multiple background tasks are created when
there are multiple webapks are launched and need to be updated, and it is fine
since the background tasks will be executed one by one.

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

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:324:
if (storeUpdateRequestCallback == null) return;
Nit: add a new line here, and remove line 326.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:329:
if (updateCallback == null) return;
Nit: add a new line here.

https://codereview.chromium.org/2948713002/diff/60001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/webapps/WebApkUpdateManagerTest.java:511:
ManifestData manifestData = defaultManifestData();
The block from line 511 to line 518 looks the same as the block from line 536 to
line 548, can you put them in one helper function like: File
createPendingUpdateFile()?

If returning a File object doesn't work for the assumption in line 524, you can
simply return a string in the helper function.