915480 - Get the installation path of web apps in a more precise way

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Marco Castelluccio [:marco]]

This isn't a real blocker for updates, but it's nice to have to avoid problems with some corner cases.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: verify_old_installations

I think we can start with something simple (using the old naming scheme only for hosted apps) and then introduce complex checks if we find those corner cases occurring too often.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 804563 [details] [diff] [review]
verify_old_installations

This seems like a good idea and useful refinement!

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 808814 [details] [diff] [review]
Patch

Review of attachment 808814 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/webapps/WebappOSUtils.jsm
@@ +63,5 @@
>        appRegKey = open(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
>                         "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\" +
>                         uniqueName, Ci.nsIWindowsRegKey.ACCESS_READ);
>      } catch (ex) {
> +      // Fallback to the old installation naming scheme

Nit: Fallback -> Fall back (here and elsewhere, because it's being used as a verb in this case).

@@ +113,5 @@
>      try {
>        let path;
>        if (path = mwaUtils.pathForAppWithIdentifier(aApp.origin)) {
> +        if (!this.verifyOldInstallationPath(aApp, installLocation)) {
> +          return false;

installLocation -> path

Also, callers expect the return value to be an array of two values, so this case should return [null, null].  But you don't actually need to return that here, as you can simply combine the conditionals and let the return statement below them do it, i.e.:

    // Fall back to the old installation naming scheme.
    try {
      let path;
      if ((path = mwaUtils.pathForAppWithIdentifier(aApp.origin)) &&
          this.verifyOldInstallationPath(aApp, path)) {
        return [ aApp.origin, path ];
      }
    } catch(ex) {}

    return [ null, null ];

@@ +139,5 @@
>        exeFile.append(installDir);
>        exeFile.append("webapprt-stub");
>  
> +      if (!exeFile.exists() ||
> +          !this.verifyOldInstallationPath(aApp, exeFile.parnet.path)) {

Erm, "parnet"?

@@ +291,5 @@
>    /**
> +   * Returns true if the given install path (in the old format) actually
> +   * belongs to the given application.
> +   */
> +  verifyOldInstallationPath: function(aApp, aInstallPath) {

Nit: given that it returns a boolean, this name would be a bit easier to understand with "verify" changed to "is"; and I'd also shorten "Installation" to "Install" to make it the more compact *isOldInstallPath*.

@@ +294,5 @@
> +   */
> +  verifyOldInstallationPath: function(aApp, aInstallPath) {
> +    if (aApp.origin.startsWith("app")) {
> +      return false;
> +    }

Nit: it'd be useful to put a comment here explaining why this is the case, i.e. because apps with an origin that starts with "app" are packaged apps, and packaged apps were never installed in the old format, so they can never have an old install path.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Patch

I forgot that, since the last version of the patch, things are a little changed in Weba

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 811006 [details] [diff] [review]
Patch

Review of attachment 811006 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/webapps/WebappOSUtils.jsm
@@ +289,5 @@
> +   * belongs to the given application.
> +   */
> +  isOldInstallPath: function(aApp, aInstallPath) {
> +    // Applications with an origin that starts with "app" are packaged apps and
> +    // packaged apps have never been installed in the old format.

Nit: earlier the code refers to the old "scheme", but here it calls it a "format".  It would be a bit easier to read this code if it used one of those terms consistently.

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Patch

I've also changed the name of the isOldInstallPath function to "isOldInstallPathValid", I think it makes clearer what the function actually does.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0029abd9afdd

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0029abd9afdd