Closed
Bug 563262
Opened 14 years ago
Closed 14 years ago
Add API to directly get an nsIURI from an installed Addon's files
Categories
(Toolkit :: Add-ons Manager, enhancement)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b1
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: davemgarrett, Assigned: mkaply)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [rewrite])
Attachments
(2 files, 2 obsolete files)
4.50 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
6.50 KB,
patch
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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().
Reporter | ||
Comment 2•14 years ago
|
||
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•14 years ago
|
||
There is a similar problem with AddonInstall.sourceURL
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
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.
Updated•14 years ago
|
blocking2.0: --- → ?
Comment 6•14 years ago
|
||
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.
blocking2.0: ? → -
Assignee | ||
Comment 7•14 years ago
|
||
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•14 years ago
|
||
(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?
Assignee | ||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 12•14 years ago
|
||
(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•14 years ago
|
||
(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.
Assignee | ||
Comment 14•14 years ago
|
||
Assignee: nobody → mozilla
Attachment #452705 -
Flags: review?
Assignee | ||
Comment 15•14 years ago
|
||
Testcase and patch attached.
Assignee | ||
Updated•14 years ago
|
Attachment #452705 -
Flags: review? → review?(dtownsend)
Reporter | ||
Comment 16•14 years ago
|
||
Blair: You might want to file a new bug for AddonInstall.sourceURL (comment 3)
Updated•14 years ago
|
Attachment #452706 -
Attachment mime type: application/x-javascript → text/plain
Updated•14 years ago
|
Attachment #452705 -
Flags: review?(dtownsend) → review+
Comment 17•14 years ago
|
||
Comment on attachment 452705 [details] [diff] [review] Rename of API Looks great, thanks
Comment 18•14 years ago
|
||
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("xpcshell@tests.mozilla.org", "XPCShell", "1", "1"); > > const dataDir = do_get_file("data"); Doesn't seem to be used. Otherwise looks good.
Attachment #452706 -
Flags: review+
Assignee | ||
Comment 19•14 years ago
|
||
Updated testcase My commit access expired due to inactivity so I'll need someone else to check this in...
Attachment #452706 -
Attachment is obsolete: true
Updated•14 years ago
|
Keywords: checkin-needed
Comment 20•14 years ago
|
||
the test patch is not patching any file and is not pushable. I guess you wanted to add a new test file.
Assignee | ||
Comment 21•14 years ago
|
||
The test file is just a new JS file that goes in mozilla/toolkit/mozapps/extensions/test/xpcshell
Comment 22•14 years ago
|
||
you can create the file and then add it to the patch using hg add path_to_test_file
Assignee | ||
Comment 23•14 years ago
|
||
Marco, thanks for the tip. Here's an hg export.
Assignee | ||
Updated•14 years ago
|
Attachment #453059 -
Attachment is patch: true
Attachment #453059 -
Attachment mime type: application/octet-stream → text/plain
Reporter | ||
Updated•14 years ago
|
Attachment #452865 -
Attachment is obsolete: true
Reporter | ||
Updated•14 years ago
|
Attachment #453059 -
Attachment description: hg export → full patch with added file
Comment 24•14 years ago
|
||
Landed: http://hg.mozilla.org/mozilla-central/rev/d13eb6e8a53b
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Updated•14 years ago
|
Keywords: checkin-needed
Comment 25•14 years ago
|
||
Looks like the implementation passes any of the automated tests. Marking as verified fixed.
Status: RESOLVED → VERIFIED
Keywords: dev-doc-needed
Comment 26•14 years ago
|
||
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•14 years ago
|
||
This is already documented, so I'm updating the keyword.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•