Closed Bug 398579 Opened 13 years ago Closed 13 years ago

Allow using file: URLs in Components.utils.import()

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: ecfbugzilla, Assigned: WeirdAl)

References

Details

Attachments

(1 file)

The restriction to resource:// URLs limits the usefulness of Components.utils.import() considerably. I would like to use this feature in TomTom HOME but it would require moving JavaScript files out of a JAR file and accessing them via unusual scheme. Also, this restriction means that extensions cannot have own modules.

Bug 380970 talks about the requirement that the URL corresponds to an actual file on disk. From what I can see, this is mostly for reasons of hashing - any good reason why we cannot hash by URL? Other than that the file is passed to mozJSComponentLoader::GlobalForLocation() which should be able to take a URL and read from a stream (similar to mozJSSubScriptLoader). One issue is the __LOCATION__ property set in components - but setting it for file:/// URLs only should be acceptable. The other is the XPCONNECT_STANDALONE flag - I am not sure which configuration uses it but if we don't have NS_GetURLSpecFromFile() there we might have other issues working with URLs as well.
Patches accepted. It's important to get the fastload file dependencies correct.

We should definitely allow file:/// URIs... they would work right now I think except for the check at http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#1408 which I think can be safely removed.
Assignee: nobody → ajvincent
Depends on: 380970
Attached patch patch, v1Splinter Review
Attachment #285489 - Flags: superreview?
Attachment #285489 - Flags: review?(sayrer)
Attachment #285489 - Flags: superreview? → superreview?(benjamin)
Flags: blocking1.9?
Attachment #285489 - Flags: superreview?(benjamin) → superreview+
Not blocking on this, but do ask for approval once the patch is reviewed.
Flags: blocking1.9? → blocking1.9-
Attachment #285489 - Flags: review?(sayrer) → review+
Comment on attachment 285489 [details] [diff] [review]
patch, v1

per comment 3, requesting approval1.9
Attachment #285489 - Flags: approval1.9?
Attachment #285489 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in js/src/xpconnect/loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.147; previous revision: 1.146
done
Checking in js/src/xpconnect/tests/unit/test_import.js;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v  <--  test_import.js
new revision: 1.6; previous revision: 1.5
done
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M10
Um, this only fixed file: URLs, not chrome: URLs, right?  Or did this bug morph?  Or do chrome: URLs also implement nsIFileURL and I just didn't know it?
General consensus on #developers is that comment 6 is right, but that this bug did make an improvement and should remain closed - that a new bug should be filed.

<WeirdAl> chrome can never be a remote URL, right?
<bsmedberg> WeirdAl: well... officially it can't, but you could trick it if you really wanted to
<bsmedberg> it certainly doesn't have to map to only a file or JAR
<WeirdAl> that gives me a whole lot of incentive NOT to permit chrome imports
<WeirdAl> especially if we can't guarantee the source is local
<bsmedberg> you could have a chrome URI map to a data: if you were evil

As for the bug morphing, see comment 1 - that's probably where I got confused.
Reopened to see if this caused the perf regression.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded.

Checking in js/src/xpconnect/loader/mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.149; previous revision: 1.148
done
Checking in js/src/xpconnect/tests/unit/test_import.js;
/cvsroot/mozilla/js/src/xpconnect/tests/unit/test_import.js,v  <--  test_import.js
new revision: 1.8; previous revision: 1.7
done
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Okay, actually morphing the title, then, and noting a testcase was committed...
Flags: in-testsuite+
Summary: Allow using chrome:// URLs in Components.utils.import() → Allow using file: URLs in Components.utils.import()
(In reply to comment #7)
> General consensus on #developers is that comment 6 is right, but that this bug
> did make an improvement and should remain closed - that a new bug should be
> filed.

You mean, I have to file an identical bug - because you hijacked this one?
Morphing, let's not say hijacking, happens, but it ought to be done with more grace -- either help filing the new bug (Gavin, you could do that) or at least a "sorry, we're better off morphing -- please file a new bug."

/be
(In reply to comment #13)
> Morphing, let's not say hijacking, happens, but it ought to be done with more
> grace -- either help filing the new bug (Gavin, you could do that) or at least
> a "sorry, we're better off morphing -- please file a new bug."

Indeed - I didn't mean to sound cavalier, having to file a new bug because someone morphed the original isn't any fun. Wladimir had already filed bug 406538 when I made my comment, though, so my comment was just meant to confirm that he had done the right thing.
You need to log in before you can comment on or make changes to this bug.