563262 - Add API to directly get an nsIURI from an installed Addon's files

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Dave Garrett]

In bug 552743, AddonWrapper.getResourceURL(path) was added for the new Addon Manager API to get a URL for an addon file. This function seems to be getting an nsIURI and then returning nsIURI.spec. For those that are going to need to use the nsIURI, this means taking the returned path and then generating the nsIURI object again. I'd like to request a way to skip this back-and-forth and have an AddonWrapper.getResourceURI(path) in addition to the existing function that just returns the nsIURI object directly. (and thus getResourceURL(path) would simply be this.getResourceURI(path).spec) It's a very minor optimization, but it would make things a little simpler in this use case.

Comment 1

[Wladimir Palant]

I agree, on the one occasion where I used that API I was somewhat irritated by having to pass the result back to ioService.newURI().

Comment 2

[Dave Garrett]

getResourceURI could probably even replace getResourceURL altogether. If you need to just get paths to files, a chrome or resource line in chrome.manifest is sufficient. If you're resorting to getting at it through this method then that implies you need more for something and probably will need the URI object. In the event that you don't, doing URI.spec yourself is trivial so dropping a helper function to do that might make sense if you want to simplify the API a bit.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

There is a similar problem with AddonInstall.sourceURL

Comment 4

[Dave Townsend [:mossop]]

Do you have any thoughts here Rob? On the one hand I designed the API to be simple JS so just using a string rather than the heavier nsIURI object seemed appropriate. On the other hand it does seem a little pointless to use an nsIURI internally then throw it away when many consumers will need an nsIURI again.

I don't think having two different methods makes much sense but maybe switching getResourceURL to returning an nsIURI would make more sense.

Comment 5

[Dave Garrett]

This is a suggested API change (2 changes with Blair's comment 3), so the sooner a decision is made the better. It might be a near meaningless performance gain but it is a little simpler with one fewer line needed and one fewer service for the consumer to bother with in this action. All that aside, it just feels a little dumb to have to parse a string for no reason.

Comment 6

[Dave Townsend [:mossop]]

Can't justify blocking on it. If someone wants to provide the patch to change getResourceURI to return the nsIURI directory then I'll review it and we should land it before beta1.

Comment 7

[Mike Kaply [:mkaply]]

I'm working on the patch.

So just to be clear, are we changing getResourceURL to return a URI?

or adding getResourceURI and leaving getResourceURL?

Or replacing getResourceURL with getResourceURI to return a URI

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> I'm working on the patch.
> 
> So just to be clear, are we changing getResourceURL to return a URI?
> 
> or adding getResourceURI and leaving getResourceURL?
> 
> Or replacing getResourceURL with getResourceURI to return a URI

Call the method getResourceURI. I think we should also just remove getResourceURL since having both is not really useful and it should only be a small number of people affected by the change at this point.

Could you please also add a test for the case we talked about on IRC where calling getResourceURI should return a uri to the installation directory of the add-on?

Comment 9

[Mike Kaply [:mkaply]]

Is there a simple way to run the tests without doing a full build?

Even if I do a full build, how exactly do I run the tests locally?

Thanks

Comment 10

[Dave Townsend [:mossop]]

(In reply to comment #9)
> Is there a simple way to run the tests without doing a full build?

Not that I know of unfortunately

> Even if I do a full build, how exactly do I run the tests locally?

From the object dir, make -C toolkit/mozapps/extensions/test xpcshell-tests

Comment 11

[Wladimir Palant]

(In reply to comment #8)
> Call the method getResourceURI. I think we should also just remove
> getResourceURL since having both is not really useful and it should only be a
> small number of people affected by the change at this point.

The About dialog in Adblock Plus 1.2 will be affected. It can deal with either a string or nsIURI returned by getResourceURL but not with a name change for this method.

Comment 12

[Mike Kaply [:mkaply]]

(In reply to comment #11)
> The About dialog in Adblock Plus 1.2 will be affected. It can deal with either
> a string or nsIURI returned by getResourceURL but not with a name change for
> this method.

I understand, but since the beta has not been released yet, this API is not marked final, so it is still open to change.

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #11)
> (In reply to comment #8)
> > Call the method getResourceURI. I think we should also just remove
> > getResourceURL since having both is not really useful and it should only be a
> > small number of people affected by the change at this point.
> 
> The About dialog in Adblock Plus 1.2 will be affected. It can deal with either
> a string or nsIURI returned by getResourceURL but not with a name change for
> this method.

I understand that this is problematic but I think if we're changing the return type of the method we should also change the method name. This should provide a more reliable indicator of the change when developers see the old method no longer exists and is also something that can be easily checked against to support both old and new forms. I wouldn't consider doing this after beta 1 was released but we still have the opportunity to do it right now.

Comment 14

[Mike Kaply [:mkaply]]

Attachment: Rename of API

Comment 15

[Mike Kaply [:mkaply]]

Attachment: Testcase

Testcase and patch attached.

Comment 16

[Dave Garrett]

Blair: You might want to file a new bug for AddonInstall.sourceURL (comment 3)

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 452705 [details] [diff] [review]
Rename of API

Looks great, thanks

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 452706 [details]
Testcase

>/* ***** BEGIN LICENSE BLOCK *****

> * ***** END LICENSE BLOCK *****
> */

Please use the public domain license for new test files: http://www.mozilla.org/MPL/license-policy.html

>function run_test() {

Can you add a short comment explaining that this test is testing the getResourceURI function.

>  do_test_pending();
>  createAppInfo("[e-mail]", "XPCShell", "1", "1");
>
>  const dataDir = do_get_file("data");

Doesn't seem to be used.

Otherwise looks good.

Comment 19

[Mike Kaply [:mkaply]]

Attachment: Comments addressed

Updated testcase


My commit access expired due to inactivity so I'll need someone else to check this in...

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the test patch is not patching any file and is not pushable. I guess you wanted to add a new test file.

Comment 21

[Mike Kaply [:mkaply]]

The test file is just a new JS file that goes in 

mozilla/toolkit/mozapps/extensions/test/xpcshell

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

you can create the file and then add it to the patch using hg add path_to_test_file

Comment 23

[Mike Kaply [:mkaply]]

Attachment: full patch with added file

Marco, thanks for the tip.

Here's an hg export.

Comment 24

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/d13eb6e8a53b

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks like the implementation passes any of the automated tests. Marking as verified fixed.

Comment 26

[Wladimir Palant]

Confirming, current Adblock Plus development build works without having to re-create the URI object (https://hg.adblockplus.org/adblockplus/rev/4b29fcebc9eb). Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a6pre) Gecko/20100624 Minefield/3.7a6pre

Comment 27

[Eric Shepherd [:sheppy]]

This is already documented, so I'm updating the keyword.