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

VERIFIED FIXED in Firefox 15

Status

Firefox Graveyard
Web Apps
P3
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

Tracking

Trunk
Firefox 15
x86_64
Windows 7
Bug Flags:
in-moztrap -

Details

(Whiteboard: [qa!])

Attachments

(1 attachment, 4 obsolete attachments)

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.
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
I'm going out of town until 2012-05-30.  No point in me hogging this bug while I'm gone.
Assignee: tabraldes → nobody
Created attachment 626662 [details] [diff] [review]
Patch v1 - Initial WIP patch

I haven't tested this patch, but it shows the idea.
Could changing the naming scheme be risky for apps a user already has installed? What value do we get out this outside of consistency?
Assignee: nobody → tabraldes
Assignee: tabraldes → nobody
Tim: if someone doesn't get to it by the time you return, feel free to pick it back up!
Status: ASSIGNED → NEW
Keywords: helpwanted
Priority: -- → P3
Target Milestone: --- → Firefox 15
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
(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 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?
Attachment #626662 - Flags: review?(felipc)
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
Attachment #626662 - Attachment is obsolete: true
Attachment #626662 - Flags: review?(felipc)
Attachment #629340 - Flags: review?(felipc)
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
Attachment #629340 - Flags: review?(felipc) → review+
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.
Attachment #629340 - Attachment is obsolete: true
Attachment #629626 - Flags: review+
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.
Attachment #629626 - Attachment is obsolete: true
Attachment #629627 - Flags: review+
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?
Attachment #629627 - Attachment is obsolete: true
Attachment #629711 - Flags: review?(felipc)
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
Attachment #629711 - Flags: review?(felipc) → review+
https://hg.mozilla.org/mozilla-central/rev/030fa99e5bde
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: helpwanted
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?
Whiteboard: [qa+]
(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.

Updated

5 years ago
Whiteboard: [qa+] → [qa+:jsmith]

Updated

5 years ago
Flags: in-moztrap?(jsmith)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Whiteboard: [qa+:jsmith] → [qa!]
Some comments in the file were mentioning the old naming scheme, I updated them in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9298b8a841e
https://hg.mozilla.org/mozilla-central/rev/d9298b8a841e

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Flags: in-moztrap?(jsmith) → in-moztrap-
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.