758044 - Clean up naming of natively-installed webapp installation directory and uninstall registry key

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P3)

Description

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

The current naming scheme for the installation directory of natively-installed webapps is:
  "host;scheme;port" with "-1" used if the port is the default for the scheme

The current naming scheme for the uninstall registry key is:
  "scheme://host:port" with "-1" used if the port is the default for the scheme

The "-1" appears because `nsIURI` happens to use "-1" as the value for "default port" [https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIURI.idl?rev=f4157e8c4107#139]

These naming schemes look a little jarring and came about due to an implementation detail of `nsIURI`.  Elsewhere (for example, in the JSON blobs that we store per webapp in the user's Firefox profile), we store the origin as:
  "scheme://host:port" with the ":port" omitted if it is the default port for the scheme

Let's change the naming scheme for installation directory and uninstall registry key.

Comment 1

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

The proposed new naming scheme for installation directory is:
  "scheme;host;port" with ";port" omitted if the port is the default for the scheme

The proposed new naming scheme for the uninstall registry key is:
  "scheme://host:port" with ":port" omitted if the port is the default for the scheme

Comment 2

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

I'm going out of town until 2012-05-30.  No point in me hogging this bug while I'm gone.

Comment 3

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

Attachment: Patch v1 - Initial WIP patch

I haven't tested this patch, but it shows the idea.

Comment 4

[Jason Smith [:jsmith]]

Could changing the naming scheme be risky for apps a user already has installed? What value do we get out this outside of consistency?

Comment 5

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

Tim: if someone doesn't get to it by the time you return, feel free to pick it back up!

Comment 6

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

(In reply to Jason Smith [:jsmith] from comment #4)
> Could changing the naming scheme be risky for apps a user already has
> installed?

A user who has app A installed and tries to reinstall it will get a duplicate app A.  Both should run fine and both will have entries in the Windows uninstall widget.

> What value do we get out this outside of consistency?

The value we get is:
  1) The new scheme looks slightly prettier
  2) The new scheme doesn't rely on an implementation detail of `nsIURI`.  This means that if `nsIURI` changes that detail (or we use some other class for holding URIs) we don't have to write new logic that says "if the port is default, put a -1 here"
  3) The manifests that we store in the user's Firefox profile store origins as "scheme://host:port" with ":port" omitted if it is default.  Using the same scheme for the registry keys means that we can more easily match an origin to its uninstall key (see bug 756306)

Comment 7

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

Comment on attachment 626662 [details] [diff] [review]
Patch v1 - Initial WIP patch

I ran this patch through try along with the patch in bug 756306:
  https://tbpl.mozilla.org/?tree=Try&rev=903a50f8b5b0

It looks like the only issues that cropped up were the broken mochitests that are caused by that patch.

Felipe, could you take a look at this patch and let me know what you think?

Comment 8

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

Attachment: Patch v2 - Fix typo

The last patch had a typo.  This patch works in local testing.  It's been pushed to tryserver but tryserver is really backed up on Windows builds:
  https://tbpl.mozilla.org/?tree=Try&rev=d250c8fe81b9

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 629340 [details] [diff] [review]
Patch v2 - Fix typo

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

::: browser/modules/WebappsInstaller.jsm
@@ +197,1 @@
>  

I spent some time at first thinking you'd just need to replace : for ;, then realized there's ipv6 and thought this regexp would fail, then noticed ipv6 are enclosed by [] so this still works
.. which is to say, I think I prefer the "dumb" version of 

if (uri.port != -1) {
  this.installDir.leafName += ";" + uri.port;
}

instead of using a smart regexp. But no need for an extra review for this change

Comment 10

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

Attachment: Patch v3 - KISS

(In reply to Felipe Gomes (:felipe) from comment #9)
> Comment on attachment 629340 [details] [diff] [review]
> Patch v2 - Fix typo
> 
> Review of attachment 629340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/WebappsInstaller.jsm
> @@ +197,1 @@
> >  
> 
> I spent some time at first thinking you'd just need to replace : for ;, then
> realized there's ipv6 and thought this regexp would fail, then noticed ipv6
> are enclosed by [] so this still works
> .. which is to say, I think I prefer the "dumb" version of 
> 
> if (uri.port != -1) {
>   this.installDir.leafName += ";" + uri.port;
> }
> 
> instead of using a smart regexp. But no need for an extra review for this
> change

I prefer the simpler version as well :)

Carrying forward r+.  I'm running this through try and doing some local testing.  I'll land this today if all goes well.

Comment 11

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

Attachment: Patch v4 - Remove unrelated change

A change from a different patch snuck into the last one I posted here.  Removing that; sorry for the spam.

Comment 12

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

Attachment: Patch v5 - Correctly create dir name if port is non-default

So it turns out that the patch didn't work right for apps installed from a non-default port.  Since it was calling `append` an extra time, it was creating a subdirectory.  This updated patch fixes the issue and works correctly for default and non-default ports in my testing.

Felipe; the patch has changed non-trivially.  Do you mind re-reviewing?

Comment 13

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

Comment on attachment 629711 [details] [diff] [review]
Patch v5 - Correctly create dir name if port is non-default

Err, actually the changes were pretty trivial.  Carrying forward r+ and landing on inbound:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/030fa99e5bde

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/030fa99e5bde

Comment 15

[Jason Smith [:jsmith]]

This sounds like something I should verify, as the naming scheme is being changed. Tim - What should I be looking out for here in %APPDATA%? What differences exist now with this implementation when I install an app? What differences exist now if I re-install an app that already exists before this patch?

Comment 16

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

(In reply to Jason Smith [:jsmith] from comment #15)
> This sounds like something I should verify, as the naming scheme is being
> changed. Tim - What should I be looking out for here in %APPDATA%? What
> differences exist now with this implementation when I install an app? What
> differences exist now if I re-install an app that already exists before this
> patch?

For the below:
  The install dir is in %APPDATA%
  The uninstall reg key is in HKCU\Software\Microsoft\Windows\CurrentVersion\Uninstall

Before this patch:
  Install dir name: host;scheme;port (or -1 if default)
  Uninstall key name: scheme://host:port (or -1 if default)

After this patch:
  Install dir name: scheme;host (;port if not default)
  Uninstall key name: scheme://host (:port if not default)


Any apps that were installed before this patch will fail to be overwritten if you reinstall it; you will have to manually uninstall the old version.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Some comments in the file were mentioning the old naming scheme, I updated them in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9298b8a841e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9298b8a841e