398399 - make DOMLinkHandler implement nsIDOMEventListener

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Spin-off from bug 396203. See bug 396203 comment 9 for details.

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

this also contains drive-by cleanup for onLinkAdded

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch v1.00001

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 283351 [details] [diff] [review]
patch v1.00001


>-              // Verify that the load of this icon is legal.
>-              // error pages can load their favicon, to be on the safe side,
>-              // only allow chrome:// favicons
>-              const aboutNeterr = "about:neterror?";
>-              if (targetDoc.documentURI.substr(0, aboutNeterr.length) != aboutNeterr ||
>-                  !uri.schemeIs("chrome")) {
>+            // Verify that the load of this icon is legal.
>+            // error pages can load their favicon, to be on the safe side,
>+            // only allow chrome:// favicons
>+            if (!/^about:neterror\?/.test(targetDoc.documentURI) ||
>+                !uri.schemeIs("chrome")) {

what's the point of this change?  I'm pretty sure its marginally slower.  substr or indexOf are about the same speed, regex for straight strings is overkill.

Comment 4

[Dão Gottwald [:dao]]

I think ideally the regex would be compiled once and could be even faster then. But I don't really care either way, I just thought it was cleaner (more compact).

Comment 5

[Nickolay_Ponomarev]

An indexOf check is as compact as a regexp test and shouldn't be slower than the current code.

Comment 6

[Dão Gottwald [:dao]]

Yeah, my first thought was to use indexOf, but then I realized that scanning the entire string is waste -- we don't want to know the index but if the strings starts with "about:neterror?". So substr makes a little bit more sense, as well as the regex.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 8

[Reed Loden [:reed]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.862; previous revision: 1.861
done

Comment 9

[John P Baker]

(In reply to comment #6)
> Yeah, my first thought was to use indexOf, but then I realized that scanning
> the entire string is waste -- we don't want to know the index but if the
> strings starts with "about:neterror?". So substr makes a little bit more sense,
> as well as the regex.

I doubt we are talking significant (if any) savings here but I tend to program this as

targetDoc.documentURI.lastIndexOf(aboutNeterr, 0) != 0