right now there are of intanciations of ioservice all over the place

We already have makeURI from contentAreaUtils.js used in a few places, but it doesn't keep a reference to the IO service. We should fix that and then convert other places to use it.

Taking. Patch up in a few.

So, just adding a variable in an external helper js file seemed wrong to me, so I wrapped it in an object. IMO the whole file should be wrapped in that object, but that requires updating the whole tree and isn't necessary for this bug. I'd be willing to do it though, so feel free to file a bug and assign it to me if there's an interest. This contains the changes to browser.js and contentAreaUtils.js.

I don't know whether or not it is recommended practice, but the other callers of makeURI seem to be happy not passing the trailing null parameters as the undefined values will become null when passed on in > return ContentAreaUtils.ioService.newURI(aURL, aOriginCharset, aBaseURI);

Thanks for the quick patch Natch! I agree w/ comment #4 - change makeURI so that the subsequent arguments are optional, making it more caller-friendly.

Comment on attachment 390678 [details] [diff] [review] patch clarified on irc, fixup the function if needed. please fix callers to not pass null arguments where not necessary. r=me otherwise. please get second review from a browser peer and a toolkit peer, as i'm neither. (get gavin to review, and you'll get both!)

Comment on attachment 390678 [details] [diff] [review] patch Actually, I'll try to get two other reviewers seeing as gavin is pretty swamped with reviews.

Silly mistake, could've sworn I had changed it...

Comment on attachment 390971 [details] [diff] [review] patch ver. 2 >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js A couple of places you explicitly pass null, null for the last two arguments to makeURI - common practice is to just omit them and let xpconnect do the conversion, but I don't feel strongly about that. > _getManifestURI: function(aWindow) { >- var ios = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService); >- >- var contentURI = ios.newURI(aWindow.location.href, null, null); >- return ios.newURI(attr, aWindow.document.characterSet, contentURI); >+ var contentURI = makeURI(aWindow.location.href, null, null); >+ return makeURI(attr, aWindow.document.characterSet, contentURI); This method should really be using aWindow.document.documentURIObject as the baseURI, rather than creating its own URI. Can do that in a followup, I guess, if you want to avoid scope creeping this patch. > // XXX: duplicated in preferences/advanced.js > _getOfflineAppUsage: function (host, groups) Probably worth avoiding this change just to keep these two methods in sync. This isn't a particularly perf-sensitive code path. > observe: function (aSubject, aTopic, aState) > { >- var uri = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService). >- newURI(aSubject.location.href, null, null); >+ var uri = makeURI(aSubject.location.href, null, null); Same here: aSubject.document.documentURIObject. r=me, but I think the new policy requires sr on this patch...

Thanks Gavin. I addressed all the comments, including the change from makeURI to documentURIObject. I ran all the browser-chrome tests and the mochitest-plain offline tests under dom/ and all is well, so this should be fine.

Comment on attachment 390990 [details] [diff] [review] comments addressed Looks great to me.

Though I should ask -- why did the io service move to ContentAreaUtils? That seems odd, especially in light of bug 506111.

This file needs it and it can be used in any file that wants it. Previously this file would call getService every time this function was called. Now there's a smart getter for it, I just figured it wasn't right to declare a global variable in a shared js file. See comment 3. Thanks.

Comment on attachment 390990 [details] [diff] [review] comments addressed >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > _getManifestURI: function(aWindow) { >- if (!attr) return null; >- >- try { >- var ios = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService); >- >- var contentURI = ios.newURI(aWindow.location.href, null, null); >- return ios.newURI(attr, aWindow.document.characterSet, contentURI); >- } catch (e) { >- return null; >- } >+ return attr ? aWindow.document.documentURIObject : null; This is wrong - you need to use the "attr" value still - the documentURIObject is only useful as the baseURI (i.e. the third argument to makeURI). I'm surprised we don't have a test that breaks because of this... we really should.

Backed that out: https://hg.mozilla.org/mozilla-central/rev/4ef37aa4511d

Comment on attachment 390990 [details] [diff] [review] comments addressed >--- a/toolkit/content/contentAreaUtils.js >+++ b/toolkit/content/contentAreaUtils.js > function openURL(aURL) > { >- var ios = Components.classes["@mozilla.org/network/io-service;1"] >- .getService(Components.interfaces.nsIIOService); >- var uri = ios.newURI(aURL, null, null); >+ var uri = makeURI(aURL, null, null); You missed this one when removing the trailing null parameters.

See also bug 506111 comment 12.

Comment on attachment 390990 [details] [diff] [review] comments addressed >--- a/toolkit/content/contentAreaUtils.js >+++ b/toolkit/content/contentAreaUtils.js > function openURL(aURL) > { >- var ios = Components.classes["@mozilla.org/network/io-service;1"] >- .getService(Components.interfaces.nsIIOService); >- var uri = ios.newURI(aURL, null, null); >+ var uri = makeURI(aURL, null, null); ios is still used in that function.

Right, sorry about that. I ran the tests and nothing came up, and I misunderstood what gavin had said earlier, I'll make both changes. For the openURL issue (that it still uses ios) I'll use ContentAreaUtils.ioService.

OK, all the comments were addressed. Sorry for the confusion, if someone could point me out to how and when _getManifestURI works I'll try to create a test.

Comment on attachment 391098 [details] [diff] [review] patch >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > _getManifestURI: function(aWindow) { > if (!aWindow.document.documentElement) return null; > var attr = aWindow.document.documentElement.getAttribute("manifest"); > if (!attr) return null; >- >- try { >- var ios = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService); >- >- var contentURI = ios.newURI(aWindow.location.href, null, null); >- return ios.newURI(attr, aWindow.document.characterSet, contentURI); >- } catch (e) { >- return null; >- } >+ return makeURI(attr, >+ aWindow.document.characterSet, >+ aWindow.document.documentURIObject); Don't think you can get rid of the try/catch, since the "attr" may be bogus. This method only seems to be used in response to "offline-cache-update-completed", which doesn't seem to be covered by existing tests. Perhaps test_offlineNotification.html could be modified to cover that case? I'm not an expert on that offline cache code, so perhaps we should just defer the documentURIObject changes to a followup to reduce the risk/churn here.

Ok, fair enough.

Comment on attachment 391122 [details] [diff] [review] patch >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > observe: function (aSubject, aTopic, aState) >- var uri = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService). >- newURI(aSubject.location.href, null, null); >+ var uri = aSubject.document.documentURIObject; Defer this change to the followup as well and just use makeURI(aSubject.location.href)?

Does this need re-super-review? Remove checkin-needed if it doesn.

Tests will come in the followup for the two changes skipped in this bug. Those are the only functional changes here.