bah, mistyped an enter early. We should fix our erroneous use of urlSecurityCheck params and match toolkit's use. This so that we do not find ourselves having extensions (or even, in-app-code) accidentally hurt by the order. Patch upcoming

this should do it, (took a bit more parity work than simply param order. Also included the same test toolkit uses to keep us working here.

(In reply to comment #1) >We should fix our erroneous use of urlSecurityCheck params You mean, we should match toolkit's erroneous use :-P Maybe we should rename all our existing uses to checkLoadURIStrWithPrincipal?

Can we fix toolkit?

Or are they basically saying they disagree with the design of the security API (for that specific method) and find their order of arguments more intuitive (for themselves and extension developers)? While we could rename our existing uses to checkLoadURIStrWithPrincipal, you'll still want the toolkit compat function, which apparently can take both a string and an nsIURI, so it seems like you'd end up duplicating quite a bit of code, or checkLoadURIStrWithPrincipal() just calls into urlSecurityCheck(), in which case why not just take the patch Callek wrote?

(In reply to comment #5) > Or are they basically saying they disagree with the design of the security API > (for that specific method) and find their order of arguments more intuitive > (for themselves and extension developers)? You'd have to ask Mano (and you can blame me for not noticing the disparity when reviewing bug 342485's patch). I wouldn't be opposed to fixing it, but we already took the extension compat hit once, I'm not sure that changing things again this late in the cycle would be a good idea.

I looked through about 100+ Firefox/Thunderbird extensions I have on my hard disk and none of them call "urlSecurityCheck". Unfortunately A.M.O. doesn't yet have LXR/MXR.

Well, there's bug 372304 for one - it's hard to know exactly how many extensions we have the potential of breaking. You could file a bug against AMO to request a manual grep of the extensions there (e.g. bug 421589). It may be that most extensions haven't yet been updated for Firefox 3, so maybe if we do it now we can get away with it.

Yeah, I think we shouldn't break extensions this late in the game. Besides, I kinda like having the URL before the principal; it's the object you're interested in, and it matches the order of the "types" in the function names.

Comment on attachment 307411 [details] [diff] [review] rearrange params + unit test >+ * or as a nsIURI object. an nsIURI object >+ * The principal of the document from which aURL came. Actually this should be the node, not the document (although currently you get the same principal). >+ * Flags to be passed to checkLoadURIStr. If undefined, ...WithPrincipal >+ * nsIScriptSecurityManager.STANDARD will be passed. Just say that nsIScriptSecurityManager.STANDARD is the default value. >+ if (aFlags === undefined) >+ aFlags = nsIScriptSecurityManager.STANDARD; >+ Unnecessary. We left it out for a reason :-P r=me with those fixed.

I'll check this in later

Checking in browser/linkToolbarOverlay.js; /cvsroot/mozilla/suite/browser/linkToolbarOverlay.js,v <-- linkToolbarOverlay.js new revision: 1.18; previous revision: 1.17 done Checking in common/Makefile.in; /cvsroot/mozilla/suite/common/Makefile.in,v <-- Makefile.in new revision: 1.5; previous revision: 1.4 done Checking in common/contentAreaUtils.js; /cvsroot/mozilla/suite/common/contentAreaUtils.js,v <-- contentAreaUtils.js new revision: 1.153; previous revision: 1.152 done Checking in common/nsContextMenu.js; /cvsroot/mozilla/suite/common/nsContextMenu.js,v <-- nsContextMenu.js new revision: 1.124; previous revision: 1.123 done Checking in common/utilityOverlay.js; /cvsroot/mozilla/suite/common/utilityOverlay.js,v <-- utilityOverlay.js new revision: 1.111; previous revision: 1.110 done RCS file: /cvsroot/mozilla/suite/common/tests/Makefile.in,v done Checking in common/tests/Makefile.in; /cvsroot/mozilla/suite/common/tests/Makefile.in,v <-- Makefile.in initial revision: 1.1 done RCS file: /cvsroot/mozilla/suite/common/tests/unit/test_contentAreaUtils.js,v done Checking in common/tests/unit/test_contentAreaUtils.js; /cvsroot/mozilla/suite/common/tests/unit/test_contentAreaUtils.js,v <-- test_contentAreaUtils.js initial revision: 1.1 done Checking in makefiles.sh; /cvsroot/mozilla/suite/makefiles.sh,v <-- makefiles.sh new revision: 1.9; previous revision: 1.8 done