Closed Bug 777402 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox Graveyard :: Web Apps, defect)

16 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 25

People

(Reporter: jsmith, Assigned: marco)

References

()

Details

(Whiteboard: DesktopWebRT2)

Attachments

(3 files, 10 obsolete files)

45.79 KB, patch
fabrice
: review+
myk
: checkin+
Details | Diff | Splinter Review
26.63 KB, patch
marco
: review+
myk
: checkin+
Details | Diff | Splinter Review
1.03 KB, patch
marco
: review+
Details | Diff | Splinter Review
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.
Keywords: productwanted
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Can I get rid of mozIDOMApplicationRegistry2 when we'll support it also on desktop?
Flags: needinfo?(fabrice)
FYI - For testing, feel free to take advantage of packaged app test cases at http://mozqa.com/webapi-permissions-tests/.
(In reply to Marco Castelluccio [:marco] from comment #1)
> Can I get rid of mozIDOMApplicationRegistry2 when we'll support it also on
> desktop?

Absolutely.
Flags: needinfo?(fabrice)
Duplicate of this bug: 842296
Blocks: 892837
Attached patch Packaged apps installation (obsolete) — Splinter Review
Attachment #774984 - Flags: review?(myk)
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.
(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
> 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... :)
(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.
Attached patch Packaged apps installation v2 (obsolete) — Splinter Review
Updated patch and triggered new tests also for android and b2g: https://tbpl.mozilla.org/?tree=Try&rev=fb28e406ac74
Attachment #774984 - Attachment is obsolete: true
Attachment #774984 - Flags: review?(myk)
Attachment #775092 - Flags: review?(myk)
Attachment #775092 - Flags: review?(fabrice)
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.
Attachment #775092 - Flags: review?(fabrice) → review+
Note per the try run - the packaged app mochitests appear to be always failing on FxAndroid and intermittently failing on desktop.
(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 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
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
Myk I forgot to say this applies on top of the patch in bug 778277
Blocks: 895538
Attached patch Packaged apps installation v3 (obsolete) — Splinter Review
Updated, applies on top of the other patch.
Attachment #775092 - Attachment is obsolete: true
Attachment #775092 - Flags: review?(myk)
Attachment #778722 - Flags: review?(myk)
Attachment #778722 - Flags: review?(fabrice)
I left some debugging cruft in that patch...
Attachment #778721 - Attachment is obsolete: true
Attachment #778721 - Flags: review?(myk)
Attachment #778721 - Flags: review?(fabrice)
Attachment #778738 - Flags: review?(myk)
Attachment #778738 - Flags: review?(fabrice)
Attachment #778738 - Attachment is patch: true
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
Attachment #778722 - Flags: review?(fabrice) → review-
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.
Attachment #778738 - Flags: review?(fabrice)
(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".
Attachment #778738 - Attachment is obsolete: true
Attachment #778738 - Flags: review?(myk)
Attachment #778991 - Flags: review?(myk)
Attachment #778991 - Flags: review?(fabrice)
Attached patch Packaged apps installation v4 (obsolete) — Splinter Review
> 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.
Attachment #778722 - Attachment is obsolete: true
Attachment #778722 - Flags: review?(myk)
Attachment #779258 - Flags: review?(myk)
Attachment #779258 - Flags: review?(fabrice)
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?
Attachment #778991 - Flags: review?(myk) → review-
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?
Attachment #779258 - Flags: review?(wjohnston)
Attachment #779258 - Flags: review?(myk)
Attachment #779258 - Flags: review-
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).
Attachment #778991 - Attachment is obsolete: true
Attachment #778991 - Flags: review?(fabrice)
Attachment #780093 - Flags: review?(myk)
Attachment #780093 - Flags: review?(fabrice)
Attached patch Packaged apps installation v5 (obsolete) — Splinter Review
Attachment #779258 - Attachment is obsolete: true
Attachment #779258 - Flags: review?(wjohnston)
Attachment #779258 - Flags: review?(fabrice)
Attachment #780096 - Flags: review?(myk)
Attachment #780096 - Flags: review?(fabrice)
Attachment #780096 - Flags: review?(wjohnston)
(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 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.
Attachment #780093 - Flags: review?(myk) → review+
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);
Attachment #780096 - Flags: review?(myk) → review+
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?
Attachment #780096 - Flags: review?(wjohnston) → review+
Keywords: productwanted
Whiteboard: DesktopRT2
Whiteboard: DesktopRT2 → DesktopWebRT2
(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 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 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)
Attachment #780096 - Flags: review?(fabrice)
(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.
Depends on: 898448
Blocks: 873567
Attached patch manifestUrl (obsolete) — Splinter Review
In the meantime I've fixed the nits.
Attachment #780093 - Attachment is obsolete: true
Attachment #780093 - Flags: review?(fabrice)
Attachment #784491 - Flags: review?(fabrice)
Attachment #780096 - Attachment is obsolete: true
Attachment #784492 - Flags: review?(fabrice)
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
Attachment #784491 - Flags: review?(fabrice) → review+
Attachment #784492 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
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)
Attached patch manifestUrlSplinter Review
Attachment #784491 - Attachment is obsolete: true
Attachment #784666 - Flags: review+
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.
Attached patch fix test dataSplinter Review
Actually the code that was in automation.py.in has been moved to another file. This is the new fix, that was already reviewed.
Attachment #784757 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fe6643be288d
https://hg.mozilla.org/mozilla-central/rev/668bb7634d04
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.