[WebAPK] Refactor "unsigned sources" installation path

[WebAPK] Refactor "unsigned sources" installation path

This CL:
- Refactors WebApkInstaller::StartInstallingDownloadedWebApk() and
  WebApkInstaller::StartUpdateUsingDownloadedWebApk() to not have a
  return value.
- Makes both Google Play and "unsigned sources" installs use
  WebApkInstaller#notify().
- Combines the install and update methods in WebApkInstaller to get rid of some
  duplicate code.

BUG=696132
Review-Url: https://codereview.chromium.org/2746723002
Cr-Commit-Position: refs/heads/master@{#456888}
Committed: https://chromium.googlesource.com/chromium/src/+/01bf47ad61ffef17d3a4e96ada6e2703ae67cf18

[pkotwicz, 2017-03-12 05:26:24]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2017-03-12 05:28:55]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2017-03-12 05:31:50]

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

[pkotwicz, 2017-03-12 05:31:50]

Dominick, can you please take a look?

I wrote this CL in response to your comments in
https://codereview.chromium.org/2733543002/

[dominickn, 2017-03-13 02:18:55]

Thanks for this, I like the reduction in duplication.

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

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:83:
private void installOrUpdateDownloadedWebApkAsync(
The boolean argument is a little bit subtle now. The notify() refactor here is
quite nice, but can we keep the two separate methods for clarity? Something
like:

private void installDownloadedWebApkAsync(String filePath, String packageName) {
  // Start monitoring the installation
  notify(installOrUpdateDownloadedWebAplImpl(filepath));
}

private void updateDownloadedWebApkAsync(String filePath, String packageName) {
  notify(installOrUpdateDownloadedWebAplImpl(filepath));
}

This still removes a bunch of duplication, but is more clear.

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:106:
// InstallerDelegate cannot detect whether updates are successful. Assume that
update is
InstallerDelegate shouldn't have been created if isUpdate is true right?

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:122:
private void installOrUpdateWebApkFromGooglePlayAsync(boolean isUpdate, String
packageName,
Again, could we keep the two separate methods for clarity? It could be something
like:

private void InstallWebApkFromGooglePlayAsync(...) {
  if (isWebApkInstalled(packageName) {
    notify(true);
    return;
  }
  updateWebApkFromGooglePlayAsync(...);
}

private void updateWebApkFromGooglePlayAsync() {
  // check install delegate
  // set up callback
  // call installAsync
}

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:154:
if (mListener != null) {
Previously adding the shortcut was the very last thing completed at this step.
Is there a reason for the change in order? Perhaps unregister the listener
before creating the shortcut at least?

[pkotwicz, 2017-03-13 20:00:14]

Dominick, can you please take another look?

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

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:83:
private void installOrUpdateDownloadedWebApkAsync(
We don't want to notify immediately for installDownloadedWebApkAsync(). We only
want to notify when the installation succeeds (or InstallerDelegate times out)

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:106:
// InstallerDelegate cannot detect whether updates are successful. Assume that
update is
I have clarified the comment. The comment explains why we don't create an
InstallerDelegate for updates

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:122:
private void installOrUpdateWebApkFromGooglePlayAsync(boolean isUpdate, String
packageName,
Back to two methods. I kept it a bit more verbose than what you suggested.
Calling updateWebApkFromGooglePlayAsync() from
InstallWebApkFromGooglePlayAsync() is confusing

https://codereview.chromium.org/2746723002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:154:
if (mListener != null) {
Restored original order

[dominickn, 2017-03-13 23:13:37]

Mainly nits now.

https://codereview.chromium.org/2746723002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java
(right):

https://codereview.chromium.org/2746723002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkInstaller.java:96:
if (!installOrUpdateDownloadedWebApkImpl(filePath)) {
Nit: can you add a comment explaining that we only want to notify in case of
failure, and otherwise we wait for the InstallerDelegate to tell us if it
succeeded.

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

https://codereview.chromium.org/2746723002/diff/80001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:610: return;
Nit: move the webapk_package_ conversion into the INSTALL branch so it's not run
for the UDPATE branch.

Also consider removing the early return since the two branches are so small.

[pkotwicz, 2017-03-14 00:06:52]

Dominick, can you please take another look?

[dominickn, 2017-03-14 00:10:11]

lgtm, thanks

[pkotwicz, 2017-03-14 19:01:27]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-03-14 19:01:28]

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/2746723002/#ps140001
(title: "Merge branch 'master' into start")

[commit-bot: I haz the power, 2017-03-14 19:02:40]

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-03-14 20:37:35]

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

[commit-bot: I haz the power, 2017-03-14 20:37:36]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pkotwicz, 2017-03-14 21:51:57]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2017-03-14 21:51:58]

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/2746723002/#ps160001
(title: "Merge branch 'master' into start")

[commit-bot: I haz the power, 2017-03-14 21:52: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-03-14 23:26:37]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489528317840150,
"parent_rev": "086b8855ab10dd52271b283c7220922e9d680054", "commit_rev":
"01bf47ad61ffef17d3a4e96ada6e2703ae67cf18"}

[commit-bot: I haz the power, 2017-03-14 23:28:26]

Description was changed from

==========
[WebAPK] Refactor "unsigned sources" installation path

This CL:
- Refactors WebApkInstaller::StartInstallingDownloadedWebApk() and
  WebApkInstaller::StartUpdateUsingDownloadedWebApk() to not have a
  return value.
- Makes both Google Play and "unsigned sources" installs use
  WebApkInstaller#notify().
- Combines the install and update methods in WebApkInstaller to get rid of some
  duplicate code.

BUG=696132
==========

to

==========
[WebAPK] Refactor "unsigned sources" installation path

This CL:
- Refactors WebApkInstaller::StartInstallingDownloadedWebApk() and
  WebApkInstaller::StartUpdateUsingDownloadedWebApk() to not have a
  return value.
- Makes both Google Play and "unsigned sources" installs use
  WebApkInstaller#notify().
- Combines the install and update methods in WebApkInstaller to get rid of some
  duplicate code.

BUG=696132

Review-Url: https://codereview.chromium.org/2746723002
Cr-Commit-Position: refs/heads/master@{#456888}
Committed:
https://chromium.googlesource.com/chromium/src/+/01bf47ad61ffef17d3a4e96ada6e...
==========

[commit-bot: I haz the power, 2017-03-14 23:28:27]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/01bf47ad61ffef17d3a4e96ada6e...