Last Comment Bug 758044 - Clean up naming of natively-installed webapp installation directory and uninstall registry key
: Clean up naming of natively-installed webapp installation directory and unins...
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: x86_64 Windows 7
: P3 normal
: Firefox 15
Assigned To: Tim Abraldes [:TimAbraldes] [:tabraldes]
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 15:43 PDT by Tim Abraldes [:TimAbraldes] [:tabraldes]
Modified: 2016-02-04 15:00 PST (History)
6 users (show)
jsmith: in‑moztrap-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 - Initial WIP patch (2.27 KB, patch)
2012-05-23 18:29 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
no flags Details | Diff | Review
Patch v2 - Fix typo (2.27 KB, patch)
2012-06-01 14:17 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
felipc: review+
Details | Diff | Review
Patch v3 - KISS (3.09 KB, patch)
2012-06-03 11:53 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Review
Patch v4 - Remove unrelated change (2.33 KB, patch)
2012-06-03 11:57 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Review
Patch v5 - Correctly create dir name if port is non-default (2.38 KB, patch)
2012-06-04 00:06 PDT, Tim Abraldes [:TimAbraldes] [:tabraldes]
timabraldes: review+
Details | Diff | Review

Description Tim Abraldes [:TimAbraldes] [:tabraldes] 2012-05-23 15:43:37 PDT
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] 2012-05-23 15:43:47 PDT
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] 2012-05-23 16:25:23 PDT
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] 2012-05-23 18:29:38 PDT
Created attachment 626662 [details] [diff] [review]
Patch v1 - Initial WIP patch

I haven't tested this patch, but it shows the idea.
Comment 4 Jason Smith [:jsmith] 2012-05-23 23:44:33 PDT
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] 2012-05-25 16:35:16 PDT
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] 2012-05-31 11:06:14 PDT
(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] 2012-05-31 11:12:16 PDT
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] 2012-06-01 14:17:37 PDT
Created attachment 629340 [details] [diff] [review]
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 me!) [offline until Jun 24] 2012-06-01 23:33:44 PDT
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] 2012-06-03 11:53:02 PDT
Created attachment 629626 [details] [diff] [review]
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] 2012-06-03 11:57:18 PDT
Created attachment 629627 [details] [diff] [review]
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] 2012-06-04 00:06:55 PDT
Created attachment 629711 [details] [diff] [review]
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] 2012-06-04 00:24:59 PDT
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 :Ehsan Akhgari (busy, don't ask for review please) 2012-06-04 09:31:42 PDT
https://hg.mozilla.org/mozilla-central/rev/030fa99e5bde
Comment 15 Jason Smith [:jsmith] 2012-06-04 10:13:21 PDT
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] 2012-06-04 12:24:20 PDT
(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 me!) [offline until Jun 24] 2012-06-22 15:22:35 PDT
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] 2012-06-23 05:44:35 PDT
https://hg.mozilla.org/mozilla-central/rev/d9298b8a841e

Note You need to log in before you can comment on or make changes to this bug.