731541 - Install web app on host OS - Windows

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Web apps need to be installed on the host os as a shortcut to Firefox with some parameters like the -webapp mode (bug 725408).

This will require a common API and platform-specific implementation for each. I'll likely break down each platform in a separate bug and use this one for the common API

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows

This is the Windows part of the installation process. I'll post the Mac part on a separate patch. Also the glue that will actually call into this patch will be separate.

Brian, this implements the installation procedure for webapps in Windows. The procedure was mostly created by Tim Abraldes and it is described here: https://github.com/michaelrhanson/mozilla-central/blob/master/webapprt/README.md
A proof of concept was written in an add-on and now I've ported it (updating it to the latest specs) to a patch for Firefox.

I'll do a better error handling and small fixes in follow-ups very soon. The main goal of this patch is to have the main architectural piece for the installation process working in the tree so that the webapps work can use it.

Note: the WindowsShortcutService.jsm and redit.exe used in this patch are in the webapp runtime team's repository (https://github.com/michaelrhanson/mozilla-central/blob/master/webapprt/modules/WindowsShortcutService.jsm) and were written by TimA AFAIK. That will land in mozilla-central together with this work

Comment 2

[Brian R. Bondy [:bbondy]]

Comment on attachment 604481 [details] [diff] [review]
Webapps Native Install - Windows

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

Please add licensing info to WebappsIconHelpers.js, I think the same as WebappsInstaller.jsm is ok.
Please also add javadoc comments to all functions.
Look good overall but there are a couple bugs and some formatting nits.

::: browser/modules/Makefile.in
@@ +60,5 @@
>  	$(NULL)
>  
> +EXTRA_PP_JS_MODULES = \
> +	WebappsInstaller.jsm \
> +  $(NULL)

nit:
I personally prefer 2 spaces here but since the rest of the file uses tabs please use a tab before the $(NULL) instead of spaces.

::: browser/modules/WebappsIconHelpers.js
@@ +3,5 @@
> +  // and the values are the URL. E.g.:
> +  // aIcons = {
> +  //   "16px": "http://www.example.org/icon16.png",
> +  //   "32px": "http://www.example.org/icon32.png"
> +  // };

Please put the aIcons parameter description above the function in javadoc

@@ +6,5 @@
> +  //   "32px": "http://www.example.org/icon32.png"
> +  // };
> +  let icon = 0;
> +  if (icons) {
> +    for (z in icons) {

This should be aIcons not icons.
Also I think you should have an extra if condition around everything in this for loop:
if (aIcons.hasOwnProperty(z))
Otherwise you're including prototype inherited items.

@@ +16,5 @@
> +  }
> +  if (icon === 0) {
> +    return null;
> +  } else {
> +    return icons[icon];

This will actually fail in if you pass in strings like 16px as the property names.
Here icon would be equal to 32 but aIcons[32] is undefined.
aIcons["32px"] would correctly return "http://www.example.org/icon32.png".

@@ +20,5 @@
> +    return icons[icon];
> +  }
> +}
> +
> +function getIconForApp(shell, callback) {

nit: use aShell, aCallback
Also please add javadoc to this function with a description of what it is doing and descriptions of the parameters.

@@ +23,5 @@
> +
> +function getIconForApp(shell, callback) {
> +  let iconURI = shell.iconURI;
> +  let mimeService = Cc["@mozilla.org/mime;1"]
> +                      .getService(Ci.nsIMIMEService);

I think this should be formatted with the . on the previous line and getService aligned with Cc.

@@ +28,5 @@
> +
> +  let mimeType;
> +  try {
> +    if("data" === iconURI.scheme) {
> +      let tIndex = iconURI.path.indexOf(";");

Check return value here for -1

@@ +82,5 @@
> +    throw("getIconFromURI - Failure getting icon (" + e + ")");
> +  }
> +}
> +
> +function onIconDownloaded(shell, mimeType, aStatusCode, icon, callback) {

nit: consistency on parameter prefix: aShell, aMimeType, aStatusCode, aIcon, aCallback

::: browser/modules/WebappsInstaller.jsm
@@ +19,5 @@
> +    let shell = new WinNativeApp(aData);
> +#endif
> +
> +    // Remove previously installed app (for update purposes)
> +    shell.removeInstallation();

Wouldn't this be a problem on non windows platforms? It would be undefined here.

@@ +30,5 @@
> +      shell.removeInstallation();
> +      throw(ex);
> +    }
> +
> +    getIconForApp(shell, function() {

nit: aShell

@@ +41,5 @@
> +
> +// Common Windows//Mac property startup
> +function NativeApp(aData) {
> +  let app = aData.app;
> +  let manifest = app.manifest;

about half the time you use app.manifest below and the other half just manifest.  Probably best to just remove this and change all references to app.manifest.

@@ +46,5 @@
> +
> +  let origin = Services.io.newURI(app.origin, null, null);
> +
> +  if (app.manifest.launch_path) {
> +    this.launchURI = Services.io.newURI(origin.resolve(app.manifest.launch_path), null, null);

nit: 80 char line limit

@@ +53,5 @@
> +  }
> +
> +  let biggestIcon = getBiggestIcon(app.manifest.icons);
> +  if (biggestIcon) {
> +    if(0 !== biggestIcon.indexOf("/")) {

Please add a comment here about why you are checking for the first char being /

@@ +99,5 @@
> +
> +    this.installDir = Services.dirsvc.get("AppData", Ci.nsIFile);
> +    this.installDir.append(this.launchURI.host + ";" + 
> +                           this.launchURI.scheme + ";" +
> +                           this.launchURI.port);

Comment pls

@@ +129,5 @@
> +  removeInstallation : function() {
> +    let uninstallKey;
> +    try {
> +      uninstallKey = Cc["@mozilla.org/windows-registry-key;1"]
> +                     .createInstance(Ci.nsIWindowsRegKey);

nit: formatting

@@ +132,5 @@
> +      uninstallKey = Cc["@mozilla.org/windows-registry-key;1"]
> +                     .createInstance(Ci.nsIWindowsRegKey);
> +      uninstallKey.open(uninstallKey.ROOT_KEY_CURRENT_USER,
> +                        "SOFTWARE\\Microsoft\\Windows\\"
> +                      + "CurrentVersion\\Uninstall",

nit + should be on the previous line

@@ +138,5 @@
> +      if(uninstallKey.hasChild(this.uninstallSubkeyStr)) {
> +        uninstallKey.removeChild(this.uninstallSubkeyStr);
> +      }
> +    } finally {
> +      if(uninstallKey) uninstallKey.close();

nit please put this on 2 lines.

@@ +191,5 @@
> +    let applicationINI = this.appDir.clone().QueryInterface(Ci.nsILocalFile);
> +    applicationINI.append("application.ini");
> +
> +    let factory = Cc["@mozilla.org/xpcom/ini-processor-factory;1"]
> +                    .getService(Ci.nsIINIParserFactory);

nit: formatting

@@ +259,5 @@
> +    let target = this.appDir.clone();
> +    target.append(this.appNameAsFilename + ".exe");
> +
> +    setShortcut(shortcut, target, this.appDir.clone(), null,
> +                this.shortDescription, /* todo iconfile */);

todo? when? More detail here pls.

@@ +298,5 @@
> +
> +      try {
> +        // Embed the icon to the .exe using redit.exe
> +        let redit = this.processFolder.clone();
> +        redit.append("redit.exe");

Can you explain here what redit.exe is?

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Attachment: Updated version

updated version, not yet ready for next review

myk: the previous patch only had the installer module but it didn't have the glue to make the UI call the installation process. I've included it in this one so you guys can test it.

To test this patch I'm using the HEAD in https://github.com/michaelrhanson/mozilla-central/, plus fabrice's two patches that landed in inbound today (https://hg.mozilla.org/integration/mozilla-inbound/rev/17de225179ac and https://hg.mozilla.org/integration/mozilla-inbound/rev/374977a5f8c6)

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v2

Comment 5

[:Felipe Gomes (needinfo for replies!)]

I updated the install process with the latest specifications from timA, and also the comments pointed out above. Some comments:
 
> @@ +6,5 @@
> > +  //   "32px": "http://www.example.org/icon32.png"
> > +  // };
> > +  let icon = 0;
> > +  if (icons) {
> > +    for (z in icons) {
> 
> This should be aIcons not icons.
> Also I think you should have an extra if condition around everything in this
> for loop:
> if (aIcons.hasOwnProperty(z))
> Otherwise you're including prototype inherited items.

I included the hasOwnProperty check and rewrote this function. I had imported this function from the add-on but as we don't need to support the "px" in the string it could be made more straightforward.

> @@ +23,5 @@
> > +
> > +function getIconForApp(shell, callback) {
> > +  let iconURI = shell.iconURI;
> > +  let mimeService = Cc["@mozilla.org/mime;1"]
> > +                      .getService(Ci.nsIMIMEService);
> 
> I think this should be formatted with the . on the previous line and
> getService aligned with Cc.

I believe most of the browser code use this format for Cc[].getService(Ci). I've left it as is in the patch but let me know if you'd prefer me to change it.

> 
> @@ +28,5 @@
> > +
> > +  let mimeType;
> > +  try {
> > +    if("data" === iconURI.scheme) {
> > +      let tIndex = iconURI.path.indexOf(";");
> 
> Check return value here for -1

done

> ::: browser/modules/WebappsInstaller.jsm
> @@ +19,5 @@
> > +    let shell = new WinNativeApp(aData);
> > +#endif
> > +
> > +    // Remove previously installed app (for update purposes)
> > +    shell.removeInstallation();
> 
> Wouldn't this be a problem on non windows platforms? It would be undefined
> here.

yeah, this will become an #else for MacOS in another patch. I tried to make the steps to be platform generic but it's just unecessary pain, so I've include the steps itself in a .install() function that will be platform specific

> 
> @@ +53,5 @@
> > +  }
> > +
> > +  let biggestIcon = getBiggestIcon(app.manifest.icons);
> > +  if (biggestIcon) {
> > +    if(0 !== biggestIcon.indexOf("/")) {
> 
> Please add a comment here about why you are checking for the first char
> being /
> 

> 
> @@ +132,5 @@
> > +      uninstallKey = Cc["@mozilla.org/windows-registry-key;1"]
> > +                     .createInstance(Ci.nsIWindowsRegKey);
> > +      uninstallKey.open(uninstallKey.ROOT_KEY_CURRENT_USER,
> > +                        "SOFTWARE\\Microsoft\\Windows\\"
> > +                      + "CurrentVersion\\Uninstall",
> 
> nit + should be on the previous line

(I forgot these two changes before qref'ing and posting the patch. consider them done.


> 
> @@ +259,5 @@
> > +    let target = this.appDir.clone();
> > +    target.append(this.appNameAsFilename + ".exe");
> > +
> > +    setShortcut(shortcut, target, this.appDir.clone(), null,
> > +                this.shortDescription, /* todo iconfile */);
> 
> todo? when? More detail here pls.

This is gone now, I actually don't need the iconfile parameter as the icon is already embedded in the file
> 
> @@ +298,5 @@
> > +
> > +      try {
> > +        // Embed the icon to the .exe using redit.exe
> > +        let redit = this.processFolder.clone();
> > +        redit.append("redit.exe");
> 
> Can you explain here what redit.exe is?

Redit was this small utility: http://mxr.mozilla.org/mozilla-central/source/xulrunner/tools/redit/redit.cpp to embed an icon to an executable, but I'm not using it anymore in this version of the patch as timA converted that to js-ctypes

Comment 6

[Brian R. Bondy [:bbondy]]

I'm away in Florida on PTO this week but I'll try to look at it tonight or tomorrow night.

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 606504 [details] [diff] [review]
Webapps Native Install - Windows - v2

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

Sorry for the delay in reviewing and thanks for the patch.
There are a couple things, should be a quick r+ after the next pass.

::: browser/modules/WebappsIconHelpers.js
@@ +17,5 @@
> + * @returns the URL string for the largest specified icon
> + */
> +function getBiggestIconURL(aIcons) {
> +  let iconSizes = [];
> +  for each (let key in Object.keys(aIcons)) {

for each ... in gets the values of properties and iterates over them.  But you really want the keys only, so just use for ... in and don't do Object.keys(aIcons).
Also it is best practice to not use for in and for each in with arrays (only objects), and Object.keys(aIcons) returns an array.

@@ +24,5 @@
> +    }
> +  }
> +  iconSizes.sort(function(a, b) a - b);
> +  return aIcons[iconSizes.pop()];
> +}

I think this whole function could just be simplified to something like:

let iconSizes = Object.keys(aIcons);
iconSizes.sort(function(a, b) { return a - b; });
return aIcons[iconSizes.pop()];

Object.keys already does the equivalent of "aIcons.hasOwnProperty(key)" internally. 
Please check for 0 sized arrays here too.

::: browser/modules/WebappsInstaller.jsm
@@ +26,5 @@
> +    let shell = new WinNativeApp(aData);
> +#endif
> +
> +    try {
> +      shell.install();

I think you mentioned another patch fixes this, but why does it need to be wrong in this patch? Will the patches land at the same time? I'm not sure why shell needs to be undefined on all platforms except windows currently.

@@ +461,5 @@
> +  let stripBackRE = new RegExp("\\W*$","gi");
> +
> +  let stripped = aPossiblyBadFilenameString.replace(stripFrontRE, "");
> +  stripped = stripped.replace(stripBackRE, "");
> +  return stripped;

What if we have a blank string here, should there be some kind of default value? Perhaps instead of here from where you call it.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v3

(In reply to Brian R. Bondy [:bbondy] from comment #7)

 
> I think this whole function could just be simplified to something like:
> 
> let iconSizes = Object.keys(aIcons);
> iconSizes.sort(function(a, b) { return a - b; });
> return aIcons[iconSizes.pop()];
> 
> Object.keys already does the equivalent of "aIcons.hasOwnProperty(key)"
> internally. 
> Please check for 0 sized arrays here too.

Done. I had not done this before because even though I read the docs about Object.keys returning only self properties, it was not the case when I tested it by adding something to Object.prototype (not sure if it's a special case or not). But this is not very important as there will be a validator for the manifest files, so I used your simplified version (+ checking for 0 length)

> 
> ::: browser/modules/WebappsInstaller.jsm
> @@ +26,5 @@
> > +    let shell = new WinNativeApp(aData);
> > +#endif
> > +
> > +    try {
> > +      shell.install();
> 
> I think you mentioned another patch fixes this, but why does it need to be
> wrong in this patch? Will the patches land at the same time? I'm not sure
> why shell needs to be undefined on all platforms except windows currently.

Yeah, the patches should land together but in any case I've added an #else here for patch correctness

> 
> @@ +461,5 @@
> > +  let stripBackRE = new RegExp("\\W*$","gi");
> > +
> > +  let stripped = aPossiblyBadFilenameString.replace(stripFrontRE, "");
> > +  stripped = stripped.replace(stripBackRE, "");
> > +  return stripped;
> 
> What if we have a blank string here, should there be some kind of default
> value? Perhaps instead of here from where you call it.

Done. I added in a different point in the caller as after this function there is another pass of platform-specific sanitization. The default for windows will be webapp.exe

> > +  if (biggestIcon) {
> > +    if(0 !== biggestIcon.indexOf("/")) {
> 
> Please add a comment here about why you are checking for the first char
> being /
> 

This was meant to accept either relative paths (starting with "/") or data URLs. But I changed the code to be more strict and clear about this.


Here's an interdiff from the latest patch for easier review: http://pastebin.mozilla.org/1530035

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v3

Sorry, attached the previous patch

Comment 10

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v4

Sorry for the bug churn, I'm just including something extra here after talking with Tim Abraldes. We'll generate the uninstall.log file during this process instead of having it pre-generated in the firefox folder and copied over.

Final patch attached, and this is an interdiff from the last version you reviewed (v2): http://pastebin.mozilla.org/1530163

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 608145 [details] [diff] [review]
Webapps Native Install - Windows - v4

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

Nice work, thanks for the patch and quickly implementing the previous review comments :)

::: browser/modules/WebappsInstaller.jsm
@@ +17,5 @@
> +   * Creates a native installation of the web app in the OS
> +   *
> +   * @param aData the manifest data provided by the web app
> +   *
> +   * @returns bool true on success, false if an error was thrown

nit: take out the extra newline between param and returns.
nit2: It should be @return (not @returns)

@@ +165,5 @@
> +  _init: function() {
> +    let filenameRE = new RegExp("[<>:\"/\\\\|\\?\\*]", "gi");
> +
> +    this.appNameAsFilename = this.appNameAsFilename.replace(filenameRE, "");
> +    if (this.appNameAsFilename == "") {

nit: ===

@@ +222,5 @@
> +        uninstallKey.removeChild(this.uninstallSubkeyStr);
> +      }
> +    } finally {
> +      if(uninstallKey)
> +        uninstallKey.close();

nit: It is possible here that uninstallKey is non null but has not been opened. Please verify if a .close() call can cause any unexpected results when it has not been opened.

Comment 12

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

Attachment: Webapps Native Install - Windows - v5

Over in bug 725408 (WebappRT launcher/shell), I made some changes to the names of various files and fields for consistency and clarity, along with a change in where certain configuration information is stored for robustness against profile location changes.

Here's an updated version of Felipe's patch for Windows native installation that makes the necessary changes for compatibility.  Except for one change to the contents of a configuration file, only names have been changed.

bbondy: The changes don't seem significant enough to warrant re-review, but I'll let you be the judge of that.

Comment 13

[Brian R. Bondy [:bbondy]]

Comment on attachment 608646 [details] [diff] [review]
Webapps Native Install - Windows - v5

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

Did a diff between the 2 patches and it looks good.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

I'll keep the OSX implementation on a separate bug -- bug 739636

Comment 15

[Marco Castelluccio [:marco]]

Is there a bug yet about Linux?

Comment 16

[Jason Smith [:jsmith]]

(In reply to Marco Castelluccio from comment #15)
> Is there a bug yet about Linux?

For the 1st release, I think Linux is not going to be supported. See bug 706634.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

I've been playing with this and found a couple of problems with uninstall:

* The uninstall doesn't start as the path in the registry is pointing to the "install dir" not the "uninstall dir".  At line 185 in WebappsInstaller.jsm, the line:

    this.uninstallerFile = this.installDir.clone();

should be:

    this.uninstallerFile = this.uninstallDir.clone();

which allows the uninstall to proceed.

* After uninstalling, the shortcut is left in the start menu and the desktop.  The directory also has files left over - the exe itself, the .lnk shortcut, profiles.ini and the profiles directory.  I didn't try and debug those problems - is there a way to debug the install process?

* After uninstall, Firefox still reported the app was installed

Comment 18

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Mark Hammond (:markh) from comment #17)
>
> [...]
>
> * After uninstalling, the shortcut is left in the start menu and the
> desktop.  The directory also has files left over - the exe itself, the .lnk
> shortcut, profiles.ini and the profiles directory.  I didn't try and debug
> those problems - is there a way to debug the install process?
> 

You can track progress on the webapp uninstaller in bug 735518.	If the EXE and/or shortcuts are left over, those are bugs.  The profile information is intentionally not removed by our uninstaller per bug 710790.

> * After uninstall, Firefox still reported the app was installed

There are two notions of "installation" - native installation (which includes the generation of the EXE file) and "installed to the browser" (I don't think this is the official term, but that's the way I think of it).  When you run the uninstaller the native bits get uninstalled but the app is still technically "installed."  I don't think there's a clear vision of how the two notions of "installation" are going to be reconciled.

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v6

Removed the code that created the shortcut as the .jsm service that it uses will change; we will re-add that in a separate bug (bug 744674).
Also included the fix for bug 731824.

carrying forward r+

Comment 20

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v7

This patch includes the shortcut creation using the xpcom service that replaces the previous js-ctypes implementation.
Tim kept the same API for the shortcut creation through the service. The change from what was reviewed was trivial, so I'm carrying forward r+.

The change was just replacing the Cu.import("resource:///modules/WindowsShortcutService.jsm");  to Cc[...].getService(Ci..), and passing the icon file path to the call (I didn't bother passing it before since the icon was embedded in the file, but now it won't be anymore so we specify the icon for the shortcut)

Comment 21

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

Attachment: Webapps Native Install - Windows - v8

Another minor update to maintain compatibility with latest setShortcut API in bug 738501.

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Attachment: Webapps Native Install - Windows - v9

Just changed the name of the file from webapprt.exe to webapprt-stub.exe

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 615494 [details] [diff] [review]
Webapps Native Install - Windows - v9

[Approval Request Comment]

This is part of the webapps work and implement the installation support for Windows, which we are targeting for Firefox 14.

Risk for Fennec: none, all code here is browser/ only.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/364a45282572

Comment 25

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/364a45282572

Comment 26

[Jason Smith [:jsmith]]

Verified that this feature has landed. Followup bugs will be tracked in the webapps component typically.