396203 - Unify checks for DOMLinkAdded

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

We currently register 3 distinct listeners for DOMLinkAdded[1] and then check whether the current link applies to any of three on every page load. This can be simplified to add only one listener and have all the checks run from a central location where we can bail once we have a match.

[1]http://lxr.mozilla.org/seamonkey/source/browser/base/content/tabbrowser.xml#856 http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#2675
http://lxr.mozilla.org/seamonkey/source/browser/base/content/browser.js#5242

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch + tests

Comment 2

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch -w

This also takes care of some wacky whitespace in BrowserSearch, so here's the same -w'd.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v1.1

Doh, fixed a couple typos.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 281978 [details] [diff] [review]
Patch v1.1

>Index: base/content/browser.js
...
>+  // setup our common DOMLinkAdded listener
>+  gBrowser.addEventListener("DOMLinkAdded",
>+                            function (event) { DOMLinkHandler.onLinkAdded(event); },
>+                            false);

Why not |gBrowser.addEventListener("DOMLinkAdded", DOMLinkHandler, false)|? DOMLinkHandler could be a function or it could implement handleEvent.

>+    var rel = link.rel && link.rel.toLowerCase();
>+    var rels = {};
>+    if (rel) {
>+      for each (let relVal in rel.split(/\s+/))
>+        rels[relVal] = true;
>+    }

|for each| for arrays is slow.

>+    else if (rels.search) {
>+      var type = link.type && link.type.toLowerCase();
>+      type = type.replace(/^\s+/, "");
>+      type = type.replace(/\s+$/, "");
>+      type = type.replace(/\s*;.*/, "");

I don't know how regexp compilation works, but maybe that's easier in one run: replace(/^\s+|\s*(?:;.*)?$/g, "")

Of course that are only nits and not where real time will be spent.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #4)
> Why not |gBrowser.addEventListener("DOMLinkAdded", DOMLinkHandler, false)|?
> DOMLinkHandler could be a function or it could implement handleEvent.
> 
It's immutable to prevent others from redefining it and there are already patches to make us listen to DOMLinkRemoved, so I think it's a lot cleaner to just have the individual properties and share the same body for both.

> |for each| for arrays is slow.
Not my code and really, really negligible for the number of elements that'll typically see.

> I don't know how regexp compilation works, but maybe that's easier in one run:
> replace(/^\s+|\s*(?:;.*)?$/g, "")
Also not mine :)
I'd hope that the overhead of creating and compiling three small regexps objects would be greater than that of one larger, global one. I'll test it and replace if that's indeed the case.

Comment 6

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v2

Updated to better handle some of the quirkier things we'll run into.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 283126 [details] [diff] [review]
Patch v2

looks good, please get this landed ASAP (on a quiet tree)

Comment 8

[Ryan Flint [:rflint] (ping via IRC for reviews)]

mozilla/browser/base/content/browser.js         1.857
mozilla/browser/base/content/tabbrowser.xml 	1.244
mozilla/browser/base/content/utilityOverlay.js 	1.54
mozilla/browser/base/content/pageinfo/feeds.js 	1.4
mozilla/browser/base/content/test/Makefile.in 	1.5
mozilla/browser/base/content/test/autodiscovery.html 	1.1
mozilla/browser/base/content/test/browser_autodiscovery.js 	1.1
mozilla/browser/base/content/test/moz.png 1.1

Looks like around a 1% win on Talos' pageset, but there was a regression checked in after this so we'll need a couple more runs to know for sure.

Comment 9

[Nickolay_Ponomarev]

(In reply to comment #5)
> (In reply to comment #4)
> > Why not |gBrowser.addEventListener("DOMLinkAdded", DOMLinkHandler, false)|?
> > DOMLinkHandler could be a function or it could implement handleEvent.
> > 
> It's immutable to prevent others from redefining it and there are already
> patches to make us listen to DOMLinkRemoved, so I think it's a lot cleaner to
> just have the individual properties and share the same body for both.
> 

I don't follow this comment. What is immutable and how does DOMLinkRemoved prevent us from adding a handleEvent method on DOMLinkHandler which does the right thing depending on the event name?

Right now you're creating an unnecessary closure, which wastes some resources (probably not too much in this case), and makes it easier to waste more resources when prepareForStartup gets modified. (The local variables in prepareForStartup only get collected when the window is closed.)

Comment 10

[Nickolay_Ponomarev]

Oh and for DOMLinkRemoved - do you have the bug #? I could only find bug 380639 / bug 396056.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #9)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Why not |gBrowser.addEventListener("DOMLinkAdded", DOMLinkHandler, false)|?
> > > DOMLinkHandler could be a function or it could implement handleEvent.
> > > 
> > It's immutable to prevent others from redefining it and there are already
> > patches to make us listen to DOMLinkRemoved, so I think it's a lot cleaner to
> > just have the individual properties and share the same body for both.
> > 
> 
> I don't follow this comment.

Yeah, I didn't really understand that either. -> filed bug 398399.

Comment 12

[[e-mail]]

Comment on attachment 283126 [details] [diff] [review]
Patch v2

>+  if (type)
>+    aData.type = type;
Bah, silly in/out parameter :s