777402 - Implement support for packaged apps via the installPackage function in the mozapps DOM API in desktop web runtime

Status: RESOLVED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Jason Smith [:jsmith]]

Add support for the mozapps installPackage function for packaged apps on desktop web apps. See the corresponding https://wiki.mozilla.org/Apps/PackagingProposal for details of spec for packaged apps and the API in bug 772363.

Comment 1

[Marco Castelluccio [:marco]]

Can I get rid of mozIDOMApplicationRegistry2 when we'll support it also on desktop?

Comment 2

[Jason Smith [:jsmith]]

FYI - For testing, feel free to take advantage of packaged app test cases at http://mozqa.com/webapi-permissions-tests/.

Comment 3

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #1)
> Can I get rid of mozIDOMApplicationRegistry2 when we'll support it also on
> desktop?

Absolutely.

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation

Comment 6

[Jason Smith [:jsmith]]

Comment on attachment 774984 [details] [diff] [review]
Packaged apps installation

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

Note - It's probably worthwhile to get fabrice to review the Core DOM:Apps code changes.

::: dom/apps/tests/Makefile.in
@@ -19,5 @@
>    file_hosted_app.template.webapp \
>    test_app_update.html \
> -  $(NULL)
> -
> -ifdef MOZ_B2G

Is this really going to work? https://bugzilla.mozilla.org/show_bug.cgi?id=873567 indicates these tests won't work on FxAndroid right now. We might want to kick off a try run to verify if this works.

Comment 7

[Marco Castelluccio [:marco]]

(In reply to Jason Smith [:jsmith] from comment #6)
> Comment on attachment 774984 [details] [diff] [review]
> Packaged apps installation
> 
> Review of attachment 774984 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Note - It's probably worthwhile to get fabrice to review the Core DOM:Apps
> code changes.
> 
> ::: dom/apps/tests/Makefile.in
> @@ -19,5 @@
> >    file_hosted_app.template.webapp \
> >    test_app_update.html \
> > -  $(NULL)
> > -
> > -ifdef MOZ_B2G
> 
> Is this really going to work?
> https://bugzilla.mozilla.org/show_bug.cgi?id=873567 indicates these tests
> won't work on FxAndroid right now. We might want to kick off a try run to
> verify if this works.

Oh, thank you. I was wondering why they weren't being run on Android :)
However I've sent the patch to try: https://tbpl.mozilla.org/?tree=Try&rev=8b9405da55c8

Comment 8

[Marco Castelluccio [:marco]]

> However I've sent the patch to try:
> https://tbpl.mozilla.org/?tree=Try&rev=8b9405da55c8

Green on every platform but Win8! But I forgot Android... :)

Comment 9

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio [:marco] from comment #8)
> > However I've sent the patch to try:
> > https://tbpl.mozilla.org/?tree=Try&rev=8b9405da55c8
> 
> Green on every platform but Win8! But I forgot Android... :)

B2G as well.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation v2

Updated patch and triggered new tests also for android and b2g: https://tbpl.mozilla.org/?tree=Try&rev=fb28e406ac74

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 775092 [details] [diff] [review]
Packaged apps installation v2

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

r=me once Myk will review webappsUI.jsm,  WebappsInstaller.jsm and WebappsHandler.jsm that I'm not familiar with.

Comment 12

[Jason Smith [:jsmith]]

Note per the try run - the packaged app mochitests appear to be always failing on FxAndroid and intermittently failing on desktop.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Jason Smith [:jsmith] from comment #12)
> Note per the try run - the packaged app mochitests appear to be always
> failing on FxAndroid and intermittently failing on desktop.

In the first try run it failed only in Win8, in the second it failed on both Win7 and Win8.
Android is constantly failing, so I guess we should disable the test here until it's fixed.

Comment 14

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

Comment on attachment 775092 [details] [diff] [review]
Packaged apps installation v2

This fails to apply to my local clone of the Git mirror of mozilla-central:

  07-15 16:25 > git apply ~/packagedApps
  /Users/myk/packagedApps:612: trailing whitespace.
   
  /Users/myk/packagedApps:614: trailing whitespace.
   
  /Users/myk/packagedApps:622: trailing whitespace.
      } 
  error: patch failed: dom/apps/src/Webapps.jsm:1747
  error: dom/apps/src/Webapps.jsm: patch does not apply
  error: patch failed: toolkit/webapps/WebappsInstaller.jsm:12
  error: toolkit/webapps/WebappsInstaller.jsm: patch does not apply

Comment 15

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

It fails to import into my Hg local clone of mozilla-inbound too:

  07-15 16:29 > hg import --no-commit ~/packagedApps 
  applying /Users/myk/packagedApps
  patching file dom/apps/src/Webapps.jsm
  Hunk #1 FAILED at 1746
  1 out of 4 hunks FAILED -- saving rejects to file dom/apps/src/Webapps.jsm.rej
  patching file toolkit/webapps/WebappsInstaller.jsm
  Hunk #1 FAILED at 11
  Hunk #3 FAILED at 209
  2 out of 9 hunks FAILED -- saving rejects to file toolkit/webapps/WebappsInstaller.jsm.rej
  abort: patch failed to apply

Comment 16

[Marco Castelluccio [:marco]]

Myk I forgot to say this applies on top of the patch in bug 778277

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation v3

Updated, applies on top of the other patch.

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin

I left some debugging cruft in that patch...

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 778722 [details] [diff] [review]
Packaged apps installation v3

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

::: dom/apps/src/Webapps.jsm
@@ +1855,5 @@
>          }
>  
> +        // Disallow reinstalls from the same manifest URL for now.
> +        // See https://bugzilla.mozilla.org/show_bug.cgi?id=821288
> +        let appid = this._appIdForManifestURL(app.manifestURL);

Why are you moving that after the download? Also if we keep it there, change the comment since this code is not from bug 821288 anymore.

@@ +2671,5 @@
>         }
>         download();
>      };
>  
> +    let getDeviceStorage = Services.wm.getMostRecentWindow("navigator:browser").navigator.getDeviceStorage;

This line is too long.

@@ +2673,5 @@
>      };
>  
> +    let getDeviceStorage = Services.wm.getMostRecentWindow("navigator:browser").navigator.getDeviceStorage;
> +    if (getDeviceStorage) {
> +      let deviceStorage = getDeviceStorage("apps");

You need to test if deviceStorage is not null before using it.

::: toolkit/webapps/WebappOSUtils.jsm
@@ +18,5 @@
>      let name;
>      if (aApp.name) {
>        name = aApp.name;
>      } else {
> +      name = (aApp.updateManifest) ? aApp.updateManifest.name : aApp.manifest.name;

Here and elsewhere, you're picking the default name and the localized one if it exists. The ManifestHelper from AppsUtils.jsm can help you here.

::: toolkit/webapps/WebappsInstaller.jsm
@@ +84,3 @@
>      };
>      Services.obs.notifyObservers(null, "webapp-installed", JSON.stringify(data));
> + 

nit: trailing whitespace.

@@ +104,5 @@
>  
> +  let jsonManifest = aData.isPackage ? aData.app.updateManifest : aData.app.manifest;
> +  this.appName = sanitize(jsonManifest.name);
> +  this.appNameAsFilename = stripStringForFilename(this.appName);
> + 

here too

Comment 21

[[:fabrice] Fabrice Desré]

Comment on attachment 778738 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin

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

Can you explain your changes in the test .json files? If they are not directly related to this bug, move them somewhere else.

::: dom/apps/src/Webapps.jsm
@@ +1780,5 @@
>            return;
>          }
>  
> +        // Disallow multiple apps installations from the same origin for now.
> +        // We will remove this junk after multiple apps per origin are supported (bug 778277).

"this junk" does not belong here please.

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin v2

(In reply to Fabrice Desré [:fabrice] from comment #21)
> Can you explain your changes in the test .json files? If they are not
> directly related to this bug, move them somewhere else.

The only changes strictly needed are those in automation.py.in (otherwise there's a test failing, dom/tests/mochitest/localstorage/test_app_uninstall.html).
The problem is that we're now relying on the application name to get the installation directory, so it must be present in the webapps registry.

> "this junk" does not belong here please.

Sorry, I didn't know it was an offensive word. I just meant "ugly code".

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation v4

> Why are you moving that after the download? Also if we keep it there, change
> the comment since this code is not from bug 821288 anymore.

We now need the manifest to check if an application is locally installed, because we need the application name.

Comment 24

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

Comment on attachment 778991 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin v2

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

::: dom/apps/src/Webapps.jsm
@@ +19,5 @@
>  Cu.import("resource://gre/modules/PermissionsInstaller.jsm");
>  Cu.import("resource://gre/modules/OfflineCacheInstaller.jsm");
>  Cu.import("resource://gre/modules/SystemMessagePermissionsChecker.jsm");
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
> +Cu.import("resource://gre/modules/WebappOSUtils.jsm");

Is WebappOSUtils.jsm guaranteed to exist on all platforms (including B2G and Android)?  I'm not the expert, but b2g/app.mozbuild and mobile/android/app.mozbuild both reference /toolkit/toolkit.mozbuild, which doesn't include /toolkit/webapps (although toolkit/moz.build does).  If this module doesn't get built and packaged on B2G/Android, then we need to import it conditionally here.

@@ +1365,5 @@
>    },
>  
>    // Returns the MD5 hash of the manifest.
>    computeManifestHash: function(aManifest) {
> +    return AppsUtils.computeHash(JSON.stringify(aManifest));

Nit: now that this is a one-liner, I would unrefactor it and call AppsUtils.computeHash directly at the three call sites that currently call this function.

::: toolkit/webapps/WebappOSUtils.jsm
@@ +19,5 @@
> +    if (aApp.name) {
> +      name = aApp.name;
> +    } else {
> +      name = aApp.manifest.name;
> +    }

It would be useful to add a comment here explaining the circumstances under which an app won't have aApp.name.

@@ +39,2 @@
>      try {
> +      // Try with the new format first, if it fails try with the old format

Nit: it's possible to figure out what the old and new formats are by reading the code, but it would be helpful to explain them in a comment on the function declaration, something like:

  /**
   * Launch the given app, identifying it by its unique name, which is in either
   * the new format or the old format.
   *
   * The new format ensures a readable and unique name for an app by combining
   * its name with a hash of its manifest URL.  The old format uses its origin,
   * which is only unique until we support multiple apps per origin.
   */
  launch: function(aApp) {

@@ +64,3 @@
>          appRegKey.close();
>        }
>      }

This is a bit of a maze of twisty passages.  I would prefer to avoid nesting try/catch blocks, to the extent possible, and I think it's mostly possible here, since appRegKey won't be defined if both *open* calls throw, so it isn't necessary to close it in the *finally* block in that case.

Here's an alternative that does the same thing with one less level of nesting:

    // Try with the new format first, if it fails try with the old format
    try {
      appRegKey = open(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
                       "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\" +
                       uniqueName, Ci.nsIWindowsRegKey.ACCESS_READ);
    } catch (ex) {
      try {
        appRegKey = open(Ci.nsIWindowsRegKey.ROOT_KEY_CURRENT_USER,
                         "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\" +
                         aApp.origin, Ci.nsIWindowsRegKey.ACCESS_READ);
      } catch (ex) {
        return false;
      }
    }

    let appFilename, installLocation;
    try {
      appFilename = appRegKey.readStringValue("AppFilename");
      installLocation = appRegKey.readStringValue("InstallLocation");
    } catch (ex) {
      return false;
    } finally {
      appRegKey.close();
    }

    let launchTarget = initWithPath(installLocation);
    launchTarget.append(appFilename + ".exe");
    let process = initProcess(launchTarget);
    process.runwAsync([], 0);

@@ +91,1 @@
>      }

Nit: this code would be a bit simpler if you applied the same pattern as in the Windows code: try twice to retrieve appPath, then call launchAppWithIdentifier once:

    // Try with the new format first, if it fails try with the old format
    let appPath;
    try {
      appPath = mwaUtils.pathForAppWithIdentifier(uniqueName);
    } catch (e) {
      try {
        appPath = mwaUtils.pathForAppWithIdentifier(aApp.origin);
      } catch (e) {}
    }

    if (appPath) {
      mwaUtils.launchAppWithIdentifier(uniqueName);
      return true;
    }

    return false;

(Aside: It's unfortunate that pathForAppWithIdentifier can both throw and return an invalid path.  It would be nice if it did one or the other, so we didn't have to both catch an exception and test the return value.)

@@ +92,5 @@
>  
>      return false;
>  #elifdef XP_UNIX
> +    let process = Cc["@mozilla.org/process/util;1"]
> +                    .createInstance(Ci.nsIProcess);

Why move this to the outer block?  It doesn't look like it's used anywhere else.

@@ +97,3 @@
>  
>      let exeFile = Services.dirsvc.get("Home", Ci.nsIFile);
> +    let exeFileMan = exeFile.clone();

It isn't clear what "Man" means in this name, but in any case this code creates two nsIFile objects before knowing whether or not it needs to look for the app via the old format.  It would be simpler to assume the new format until we know the app doesn't exist there, i.e.:

    // Try with the new format first, if it fails try with the old format
    let exeFile = Services.dirsvc.get("Home", Ci.nsIFile);
    exeFile.append("." + uniqueName);

    if (!exeFile.exists()) {
      exeFile = Services.dirsvc.get("Home", Ci.nsIFile);

      let origin = Services.io.newURI(aApp.origin, null, null);
      let installDir = "." + origin.scheme + ";" +
                       origin.host +
                       (origin.port != -1 ? ";" + origin.port : "");

      exeFile.append(installDir);
    }

@@ +152,1 @@
>      exeFile.append("webapprt-stub");

Nit: since this block is identical to the one in the *launch* function, we should factor it out into a *getExecutable* function defined in an `#ifdef XP_MACOSX` block that both *launch* and *uninstall* call to get a reference to the executable.

@@ +201,5 @@
> +    if (!mwaUtils.pathForAppWithIdentifier(uniqueName)) {
> +      return !!mwaUtils.pathForAppWithIdentifier(aApp.origin);
> +    } else {
> +      return true;
> +    }

Returning `!!mwaUtils.pathForAppWithIdentifier` makes this harder to read at first glance, and the slight increase in efficiency isn't worth the cost to readability.

Also, nit: Mozilla convention is to not use *else* after *return*, as it is unnecessary.

Finally, pathForAppWithIdentifier can throw, so this should catch those exceptions.

Thus I would make this something like:

    try {
      if (mwaUtils.pathForAppWithIdentifier(uniqueName)) {
        return true;
      }
      if (mwaUtils.pathForAppWithIdentifier(aApp.origin)) {
        return true;
      }
    } catch(ex) {}

    return false;

@@ +209,5 @@
> +    let xdg_data_home_env;
> +    try {
> +      xdg_data_home_env = env.get("XDG_DATA_HOME");
> +    } catch(ex) {
> +    }

Nit: write empty catch blocks as `{}`.

@@ +220,5 @@
> +    }
> +    desktopINI.append("applications");
> +
> +    // Try with the new format first, if it fails try with the old format
> +    let desktopINIman = desktopINI.clone();

I have the same comment here as earlier about desktopINIman not being necessary.

@@ +250,5 @@
> +    let stripBackRE = new RegExp("\\s*$","gi");
> +
> +    let stripped = aPossiblyBadFilenameString.replace(stripFrontRE, "");
> +    stripped = stripped.replace(stripBackRE, "");
> +    stripped = stripped.replace(/[^a-z0-9_\-]/gi, "");

This seems to do unnecessary work.  The final *replace* call will strip whitespace from the end of the string, so stringBackRE seems unnecessary.  And it'll do most of what stripFrontRE does, too, except that it'll leave hyphens at the beginning of the name.

But it isn't clear why we care about such hyphens (but not hyphens at the end of the name).  I'm inclined to say that this can simply be implemented as the final *replace*.  But perhaps I just misunderstand.  Can you explain the reasoning behind the other two regexes?

Comment 25

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

Comment on attachment 779258 [details] [diff] [review]
Packaged apps installation v4

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

Looks pretty good!  Just a few issues, mostly nits!

::: dom/apps/src/Webapps.js
@@ +148,5 @@
> +                appId: principal.appId,
> +                isBrowser: principal.isInBrowserElement
> +              };
> +
> +    return obj;

Nit: since you return the object immediately after defining it, you might as well just return the object instead of assigning it to a variable, i.e.:

    return { app: {
      …

@@ +167,1 @@
>      return request;

Nit: the only source of uncertainty in _prepareInstall is the result of `this._ensureForeground(request)`, but the consequence of factoring out that call into _prepareInstall is that both call sites have to check the return value and branch.

That seems like a case of overgeneralization.  It does make sense to factor out the other common code into _prepareInstall, but I would continue to have each call site call _ensureForeground itself and branch accordingly, which will make the code in the call sites simpler to read (even if there is a little bit of code duplication):

    if (this._ensureForeground(request)) {
      let obj = this._prepareInstall(uri, request, aParams);
      cpmm.sendAsyncMessage("WebappsInstall", obj);
    }

    return request;

Also, nit: *obj* is a very generic name for a variable.  It would be better to call it something like *appInfo*; or, since it is only ever passed to sendAsyncMessage, simply use the return value of _prepareInstall without assigning it to a variable first:

      cpmm.sendAsyncMessage("WebappsInstall",
                            this._prepareInstall(uri, request, aParams));

@@ +218,5 @@
>      let request = this.createRequest();
>  
> +    let obj = this._prepareInstall(uri, request, aParams);
> +    if (obj) {
> +      obj.isPackage = true;

Nit: I would make isPackage a parameter of _prepareInstall and have _prepareInstall define the property on the object, since that centralizes definition of the object in the _prepareInstall function, which makes it safer to reuse that function in other contexts.

::: dom/apps/tests/Makefile.in
@@ +19,5 @@
>    file_hosted_app.template.webapp \
>    test_app_update.html \
>    $(NULL)
>  
> +ifneq (android,$(MOZ_WIDGET_TOOLKIT))

More testing FTW!

::: dom/interfaces/apps/nsIDOMApplicationRegistry2.idl
@@ -6,5 @@
> -
> -interface nsIDOMDOMRequest;
> -
> -[scriptable, uuid(34498a66-3aee-4b80-8b8b-a9c5d5ba32b6)]
> -interface mozIDOMApplicationRegistry2 : mozIDOMApplicationRegistry

I'm not sure it's a good idea to remove this interface entirely.  That will break extensions that attempt to QI the registry to it.  It seems better to leave it in the tree, but deprecate it.  It's probably ok to move installPackage to mozIDOMApplicationRegistry, though (provided you change its interface ID), since mozIDOMApplicationRegistry2 inherits from mozIDOMApplicationRegistry, so any code that QIs the registry to the former will obtain the latter interface too.

::: mobile/android/chrome/content/browser.js
@@ +6667,3 @@
>      let showPrompt = true;
>  
> +    if (!showPrompt || Services.prompt.confirm(null, Strings.browser.GetStringFromName("webapps.installTitle"), manifest.name + "\n" + aData.app.origin)) {

Requesting review from :wesj on the mobile/android/ bits.

::: toolkit/webapps/WebappOSUtils.jsm
@@ +20,5 @@
>        name = aApp.name;
>      } else {
> +      let jsonManifest = aApp.updateManifest ? aApp.updateManifest : aApp.manifest;
> +      let localeManifest = new ManifestHelper(jsonManifest, aApp.origin);
> +      name = localeManifest.name;

Nit: the jsonManifest assignment exceeds 80 characters.  You can wrap at the ":" in the ternary operator:

      let jsonManifest = aApp.updateManifest ? aApp.updateManifest
                                             : aApp.manifest;


But I would avoid declaring jsonManifest and simply pass `aApp.updateManifest || aApp.manifest` into the ManifestHelper constructor:

      let localeManifest =
        new ManifestHelper(aApp.updateManifest || aApp.manifest, aApp.origin);
      name = localeManifest.name;

@@ +25,3 @@
>      }
>  
>      return this.sanitizeStringForFilename(name).toLowerCase() + "-" + AppsUtils.computeHash(aApp.manifestURL);

While you're in the area, any chance you can wrap this statement so it fits into 80 columns?

    return this.sanitizeStringForFilename(name).toLowerCase() + "-" +
           AppsUtils.computeHash(aApp.manifestURL);

::: toolkit/webapps/WebappsInstaller.jsm
@@ +102,4 @@
>   *
>   */
>  function NativeApp(aData) {
> +  this.uniqueName = WebappOSUtils.getUniqueName(aData.app);

Nit: it would be easier to understand exactly which properties the NativeApp constructor defines if we declared its prototype object and properties explicitly.  I know the current code doesn't do this, and your code isn't changing anything in that regard, but while you're here, and since you're adding properties, it's a good time to make this change.

@@ +106,3 @@
>  
> +  let jsonManifest = aData.isPackage ? aData.app.updateManifest : aData.app.manifest;
> +  let localeManifest = new ManifestHelper(jsonManifest, aData.app.origin);

Nit: sometimes you assign ManifestHelper instances to a *manifest* variable, while other times you assign them to a *localeManifest* one.  That's ok, although it seems like *manifest* is a sufficiently expressive name for the variable, and it isn't necessary to qualify it further.

@@ +117,5 @@
> +   * @param aData the data object provided to the install function
> +   * @param aManifest the manifest data provided by the web app
> +   *
> +   */
> +  this.init = function(aData, aManifest) {

Instead of redefining this property each time the NativeApp constructor is called, assign it to the NativeApp prototype and then make the platform-specific *NativeApp prototypes inherit from the NativeApp prototype.

@@ +140,2 @@
>  
> +    if(localeManifest.developer && localeManifest.developer.name) {

Nit: add space between if and "(".

@@ +140,4 @@
>  
> +    if(localeManifest.developer && localeManifest.developer.name) {
> +      let devName = localeManifest.developer.name.substr(0, 128);
> +      devName = sanitize(devName);

Nit: you might as well sanitize while declaring:

      let devName = sanitize(localeManifest.developer.name.substr(0, 128));

@@ +150,5 @@
> +      let firstLine = localeManifest.description.split("\n")[0];
> +      let shortDesc = firstLine.length <= 256
> +                      ? firstLine
> +                      : firstLine.substr(0, 253) + "...";
> +      this.shortDescription = sanitize(shortDesc);

Unrelated to your changes, but we should investigate replacing the three dots here with an actual ellipsis character (…); or not truncating the string and letting the native API implementations that use it determine whether and where to truncate.

@@ +319,5 @@
>     */
>    _createDirectoryStructure: function() {
>      if (!this.installDir.exists())
>        this.installDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);
> +

It's good thinking to clarify the control flow here with whitespace, but it would be even clearer if you surrounded the conditional block with brackets!

::: webapprt/WebappRT.jsm
@@ +20,3 @@
>  this.WebappRT = {
>    _config: null,
> +  _launchURI: null,

This doesn't get used anywhere.  Cruft left over from an earlier version of the patch?

Comment 26

[Marco Castelluccio [:marco]]

Attachment: Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin v3 + webapposutils refactoring

I've integrated your comments into my local patch for the refactoring of WebappOSUtils.jsm and joined the two patches together.

On Linux, we could probably avoid to check for the desktop file existence and directly check for the executable file existence (this would allow having the same code for all the three platforms in isLaunchable).

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation v5

Comment 28

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> Is WebappOSUtils.jsm guaranteed to exist on all platforms (including B2G and
> Android)?  I'm not the expert, but b2g/app.mozbuild and
> mobile/android/app.mozbuild both reference /toolkit/toolkit.mozbuild, which
> doesn't include /toolkit/webapps (although toolkit/moz.build does).  If this
> module doesn't get built and packaged on B2G/Android, then we need to import
> it conditionally here.

The try build on Android is green, so it's imported: https://tbpl.mozilla.org/?tree=Try&rev=233179d8faba

Comment 29

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

Comment on attachment 780093 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin v3 + webapposutils refactoring

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

Looks good, r=myk!  Just a few nits to fix at your discretion.

::: toolkit/webapps/WebappOSUtils.jsm
@@ +19,5 @@
> +
> +    // During the installation of a new app, the aApp object
> +    // doesn't have a name property. We then need to use the manifest.
> +    // For some mozApps calls, the aApp object doesn't have a manifest
> +    // associated, and so we need to use the name property.

Thanks for this comment; it's very helpful!  It does raise one additional question: are aApp.name and aApp.manifest.name guaranteed to be identical?  We need to ensure that's the case if we're going to use them interchangeably.

@@ +93,5 @@
> +    exeFile.append("webapprt-stub");
> +
> +    if (!exeFile.exists()) {
> +      exeFile = Services.dirsvc.get("Home", Ci.nsIFile);
> + 

Nit: trailing whitespace.

@@ +247,5 @@
>  #endif
>    },
>  
> +  /**
> +   * Naively sanitize the filename (accepts only a-z, 0-9, - and _)

I'm not sure how naive this really is. ;-)

::: toolkit/webapps/WebappsInstaller.jsm
@@ +129,5 @@
>   * Windows app installer
>   *
>   * The Windows installation process will generate the following files:
>   *
> + * ${FolderName} = app name + "-" + manifest url hash

Nit: might be worth pointing out that "app name" is actually "sanitized app name".

@@ +646,5 @@
>  }
>  
>  LinuxNativeApp.prototype = {
>    _init: function() {
> +    // The ${InstallDir} and desktop entry filename are: app name + "-" + manifest url hash

Nit: wrap this line at 80 columns.

Comment 30

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

Comment on attachment 780096 [details] [diff] [review]
Packaged apps installation v5

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

Looks good, just a few nits! r=myk

::: dom/apps/src/Webapps.js
@@ +117,5 @@
>                                         Ci.nsIThread.DISPATCH_NORMAL);
>      return false;
>    },
>  
> +  _prepareInstall: function(uri, request, aParams, isPackage) {

Nit: I just noticed that the convention in this file is to prefix parameter names with "a", so these should be aURI, aRequest, aParams, and aIsPackage.  I would also make the first parameter "aURL", as it'll definitely be a URL (i.e. it won't be a URN), and we use "URL" elsewhere in this function.

::: dom/apps/src/Webapps.jsm
@@ +1860,5 @@
> +        let appid = this._appIdForManifestURL(app.manifestURL);
> +        if (appid !== null && this._isLaunchable(app)) {
> +          sendError("REINSTALL_FORBIDDEN");
> +          return;
> +        }

Very minor style nit: in cases like this, where code calls functions and then uses their return values only once, I tend to use the return values directly:

        if (this._appIdForManifestURL(app.manifestURL) !== null &&
            this._isLaunchable(app)) {
          sendError("REINSTALL_FORBIDDEN");
          return;
        }

Or I use a "let statement" that limits the scope of the let-declared variable to the code that uses it:

        let (appid = this._appIdForManifestURL(app.manifestURL)) {
          if (appid !== null && this._isLaunchable(app)) {
            sendError("REINSTALL_FORBIDDEN");
            return;
          }
        }

So as to constrain the scope of the value to the code for which it is relevant.  Obviously you can take that to an extreme and overdo it, and it's not a significant issue, but it can improve code robustness over time, reducing the chance of errors creeping into long functions due to reuse of variable names.

::: mobile/android/chrome/content/browser.js
@@ +6677,5 @@
>        });
>        if (profilePath) {
>          let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>          file.initWithPath(profilePath);
>    

Nit: not your fault!  But while you're making changes in the vicinity, perhaps you can remove this trailing whitespace?

::: toolkit/webapps/WebappsInstaller.jsm
@@ +166,5 @@
> +    } else {
> +      this.shortDescription = this.appName;
> +    }
> +
> +    this.categories = aData.app.categories;

Nit: for a bit of extra safety, slice this array to clone it, so it can't be modified afterward unintentionally:

    this.categories = aData.app.categories.slice(0);

Comment 31

[Wesley Johnston (:wesj)]

Comment on attachment 780096 [details] [diff] [review]
Packaged apps installation v5

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

Android looks ok if you can answer why you're changing this.

::: mobile/android/chrome/content/browser.js
@@ -6663,5 @@
>  
>    doInstall: function doInstall(aData) {
>      let jsonManifest = aData.isPackage ? aData.app.updateManifest : aData.app.manifest;
>      let manifest = new ManifestHelper(jsonManifest, aData.app.origin);
> -    let name = manifest.name ? manifest.name : manifest.fullLaunchPath();

Are you sure we can do this? Is manifest.name always guaranteed to be a non-empty string?

Comment 32

[Marco Castelluccio [:marco]]

(In reply to Wesley Johnston (:wesj) from comment #31)
> Are you sure we can do this? Is manifest.name always guaranteed to be a
> non-empty string?

"name" is a required manifest attribute: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#254

Comment 33

[[:fabrice] Fabrice Desré]

Comment on attachment 780093 [details] [diff] [review]
Use app name + manifest url hash as unique name for apps but forbid multiple apps per origin v3 + webapposutils refactoring

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

We need to figure out this app.name issue first.

::: dom/apps/src/Webapps.jsm
@@ +1780,5 @@
>            return;
>          }
>  
> +        // Disallow multiple hosted apps installations from the same origin for now.
> +        // We will remove this ugly code after multiple apps per origin are supported (bug 778277).

s/ugly//

@@ +1789,5 @@
> +              this._isLaunchable(this.webapps[id])) {
> +            sendError("MULTIPLE_APPS_PER_ORIGIN_FORBIDDEN");
> +            return;
> +          }
> +        }

I'm sorry but I really don't like moving that here. If the issue is that app.name is not set, we should fix that rather than letting users download a potentially big package just to drop it.

Comment 34

[[:fabrice] Fabrice Desré]

Comment on attachment 780096 [details] [diff] [review]
Packaged apps installation v5

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

We're getting close. pending r+ on one point I'd like to make sure about:
app.name is set by just reading manifest.name (we added that to b2g to help with displaying the app name in some low level reporting code), but in some places you use the name property returned by a ManifestHelper. Since ManifestHelper applies localization, this name can be different for some locales. If you rely on them being always equal, that will not work.

::: dom/apps/src/Webapps.jsm
@@ +2678,5 @@
> +    let deviceStorage = null;
> +    if (getDeviceStorage) {
> +      deviceStorage = getDeviceStorage("apps");
> +    }
> +

Let's block on bug 898448 instead.

::: dom/apps/tests/Makefile.in
@@ +19,5 @@
>    file_hosted_app.template.webapp \
>    test_app_update.html \
>    $(NULL)
>  
> +ifneq (android,$(MOZ_WIDGET_TOOLKIT))

There is android support since bug 813736 landed. we should enable that on all platform (except metro, but I don't think we run these tests there anyway)

Comment 35

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #33)
> @@ +1789,5 @@
> > +              this._isLaunchable(this.webapps[id])) {
> > +            sendError("MULTIPLE_APPS_PER_ORIGIN_FORBIDDEN");
> > +            return;
> > +          }
> > +        }
> 
> I'm sorry but I really don't like moving that here. If the issue is that
> app.name is not set, we should fix that rather than letting users download a
> potentially big package just to drop it.

Here we're just downloading the update manifest, not the package. The package is downloaded in confirmInstall.

> We're getting close. pending r+ on one point I'd like to make sure about:
> app.name is set by just reading manifest.name (we added that to b2g to help
> with displaying the app name in some low level reporting code), but in some
> places you use the name property returned by a ManifestHelper. Since
> ManifestHelper applies localization, this name can be different for some
> locales. If you rely on them being always equal, that will not work.

The app.name is set in confirmInstall through manifest.name, but the manifest here is a ManifestHelper object.
In my patch, I'm always using ManifestHelper.
So, in general, we're always using ManifestHelper, right?

> ::: dom/apps/tests/Makefile.in
> @@ +19,5 @@
> >    file_hosted_app.template.webapp \
> >    test_app_update.html \
> >    $(NULL)
> >  
> > +ifneq (android,$(MOZ_WIDGET_TOOLKIT))
> 
> There is android support since bug 813736 landed. we should enable that on
> all platform (except metro, but I don't think we run these tests there
> anyway)

The problem is that Android is failing that test. There's bug 873567 to track this issue.

Comment 36

[Marco Castelluccio [:marco]]

Attachment: manifestUrl

In the meantime I've fixed the nits.

Comment 37

[Marco Castelluccio [:marco]]

Attachment: Packaged apps installation

Comment 38

[Marco Castelluccio [:marco]]

The last try run (that also contains the fix for the test that is in bug 873567) is here: https://tbpl.mozilla.org/?tree=Try&rev=6d68e1c65bad

Comment 39

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

Importing attachment #784491 [details] [diff] [review] (manifestUrl) into an inbound clone is failing for me:

08-01 16:38 > hg import ~/manifestUrlReduced
applying /Users/myk/manifestUrlReduced
patching file build/automation.py.in
Hunk #1 FAILED at 316
1 out of 1 hunks FAILED -- saving rejects to file build/automation.py.in.rej
abort: patch failed to apply

Comment 40

[Marco Castelluccio [:marco]]

That file was refactored in bug 688667, the change shouldn't be needed anymore (the problem was that the registry file they were writing lacked app names, now the file they're using contains the names: http://mxr.mozilla.org/mozilla-central/source/testing/profiles/webapps_mochitest.json)

Comment 41

[Marco Castelluccio [:marco]]

Attachment: manifestUrl

Comment 42

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

Comment on attachment 784666 [details] [diff] [review]
manifestUrl

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1971c2f5232

Comment 43

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

Comment on attachment 784492 [details] [diff] [review]
Packaged apps installation

https://hg.mozilla.org/integration/mozilla-inbound/rev/357e720b5a37

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f57e629bb4c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=26048034&tree=Mozilla-Inbound

Comment 45

[Marco Castelluccio [:marco]]

I overlooked, the test data in webapps_mochitest.json is wrong.
This commit https://hg.mozilla.org/mozilla-central/rev/c22896a27564 broke the test data.

Comment 46

[Marco Castelluccio [:marco]]

Attachment: fix test data

Actually the code that was in automation.py.in has been moved to another file. This is the new fix, that was already reviewed.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Re-landed with the follow-up patch folded into the second.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe6643be288d
https://hg.mozilla.org/integration/mozilla-inbound/rev/668bb7634d04

Comment 48

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fe6643be288d
https://hg.mozilla.org/mozilla-central/rev/668bb7634d04