506116 - speed up makeURI(str) and use it in browser.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dietrich Ayala (:dietrich)]

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

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 2

[Nochum Sossonko [:Natch]]

Taking. Patch up in a few.

Comment 3

[Nochum Sossonko [:Natch]]

Attachment: patch

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.

Comment 4

[John P Baker]

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);

Comment 5

[Dietrich Ayala (:dietrich)]

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 6

[Dietrich Ayala (:dietrich)]

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 7

[Nochum Sossonko [:Natch]]

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.

Comment 8

[Nochum Sossonko [:Natch]]

Attachment: patch ver. 2

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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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...

Comment 10

[Nochum Sossonko [:Natch]]

Attachment: comments addressed

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 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 390990 [details] [diff] [review]
comments addressed

Looks great to me.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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

Comment 13

[Nochum Sossonko [:Natch]]

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 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/870f451d8385

Comment 15

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

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

Comment 17

[Dão Gottwald [:dao]]

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.

Comment 18

[Dão Gottwald [:dao]]

See also bug 506111 comment 12.

Comment 19

[Dão Gottwald [:dao]]

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.

Comment 20

[Nochum Sossonko [:Natch]]

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.

Comment 21

[Nochum Sossonko [:Natch]]

Attachment: patch

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 22

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 23

[Nochum Sossonko [:Natch]]

Attachment: patch

Ok, fair enough.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

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)?

Comment 25

[Nochum Sossonko [:Natch]]

Attachment: nit addressed

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

Comment 26

[Nochum Sossonko [:Natch]]

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

Comment 27

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a7a8c6169d63