792968 - Replace some regular expression string matches with String.startsWith and replace /^https?/ URI scheme tests with /^https?$/

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Instead of /^foo/.test(string), we can write string.startsWith("foo"). The latter is faster.

Here's a list of candidates:

http://mxr.mozilla.org/mozilla-central/search?string=%2F^&find=%2Fbrowser%2F[bc]&filter=/\.test\%28

Not all regular expressions in that list can be replaced, only the most simple ones.

Comment 1

[Dão Gottwald [:dao]]

Attachment: candidates

It looks like the MXR has started choking on that search. Luckily I still had the results page cached. Here's the list.

Comment 2

[Yosy]

I can do this.. this should be on "mozilla-central" (currently I have "integerations/fx-team")?

Comment 3

[Dão Gottwald [:dao]]

fx-team will work just as well.

Comment 4

[Yosy]

I am using mozilla-central finally, I have done the changes how I can test them? that I don't break any functionality?

Comment 5

[Yosy]

Attachment: patch

Comment 6

[Yosy]

submitted a patch, I won't be here in the next weeks, but I will follow from my smartphone.

Comment 7

[Yosy]

Attachment: fixed patch

Forgot to remove the old if statements in two files.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 663716 [details] [diff] [review]
fixed patch

>-    if (/^https?/.test(feedURI.scheme))
>+    if(feedURI.scheme.startsWith("http") || feedURI.scheme.startsWith("https"))

This conversion (which is formally correct) shows that the original code actually didn't make sense. When the scheme is https, the first condition (starts with "http") is already satisfied. So we could (1) drop the second condition (starts with "https"), or we could (2) convert the whole thing to:

if (feedURI.scheme == "http" || feedURI.scheme == "https")

or we could (3) change the regular expression to test the scheme from the beginning to the end:

if (/^https?$/.test(feedURI.scheme))

I'm leaning towards the third option, as it's the briefest, stricter than the first option, and since the code paths where we test this don't seem to be performance-critical.


browser/base/content/browser-places.js:

>-        if (/^https?:/.test(iconURL))
>+        if(iconURL.startsWith("http:") || iconURL.startsWith("https:"))

browser/base/content/pageinfo/pageInfo.js:

>-  if (!(/^https?:/.test(url)) || imagePref == 2)
>+  if(!url.startsWith("http:") || !url.startsWith("https:") || imagePref == 2)

browser/components/sessionstore/content/aboutSessionRestore.js:

>-      if (/^https?:/.test(iconURL))
>+      if (iconURL.startsWith("https") || iconURL.startsWith("http"))

browser/components/tabview/favicons.js:

>-    if (/^https?:/.test(tabImage)) {
>+    if(tabImage.startsWith("https:") || tabImage.startsWith("http:")) {

I think we should just keep the regular expression tests in these cases.


browser/components/places/content/controller.js:

>       if (ip.isTag && ip.orientation == Ci.nsITreeView.DROP_ON &&
>           dragged.type != PlacesUtils.TYPE_X_MOZ_URL &&
>           (dragged.type != PlacesUtils.TYPE_X_MOZ_PLACE ||
>-           /^place:/.test(dragged.uri)))
>+            dragged.uri.startsWith("place:"))

nit: the new line is indented too far

general style nit: there should be a space between "if" and "("

I can make these changes before checking this in.

Thanks!

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 663716 [details] [diff] [review]
fixed patch

>--- a/browser/base/content/pageinfo/pageInfo.js	Sat Sep 22 08:28:28 2012 -0400
>+++ b/browser/base/content/pageinfo/pageInfo.js	Sat Sep 22 19:47:26 2012 +0300

>     if ((item instanceof HTMLLinkElement || item instanceof HTMLInputElement ||
>          item instanceof HTMLImageElement ||
>          item instanceof SVGImageElement ||
>-        (item instanceof HTMLObjectElement && /^image\//.test(mimeType)) || isBG) && isProtocolAllowed) {
>+        (item instanceof HTMLObjectElement && mimeType.startsWith("image/") || isBG) && isProtocolAllowed) {

You erroneously removed a closing round parenthesis.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 663716 [details] [diff] [review]
fixed patch

>--- a/browser/components/places/content/controller.js	Sat Sep 22 08:28:28 2012 -0400
>+++ b/browser/components/places/content/controller.js	Sat Sep 22 19:47:26 2012 +0300
>@@ -1420,23 +1420,23 @@ let PlacesControllerDragHelper = {
>       catch (e) {
>         return false;
>       }
> 
>       // Only bookmarks and urls can be dropped into tag containers.
>       if (ip.isTag && ip.orientation == Ci.nsITreeView.DROP_ON &&
>           dragged.type != PlacesUtils.TYPE_X_MOZ_URL &&
>           (dragged.type != PlacesUtils.TYPE_X_MOZ_PLACE ||
>-           /^place:/.test(dragged.uri)))
>+            dragged.uri.startsWith("place:"))
>         return false;

same here

Comment 11

[Dão Gottwald [:dao]]

Attachment: patch with review comments addressed

Comment 12

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca1f19e4c2a

Comment 13

[:Benjamin Peterson]

Backed out for causing mochitest-oth failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21e1c1d2ba8

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch with review comments addressed, 2nd attempt

another missing closing parenthesis

Comment 15

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e32aa6fe544

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e32aa6fe544

Comment 17

[Marco Bonardo [:mak]]

bug 797430 has been caused by replacing the valid /regex/.test(undefined) case. I quickly skimmed the patch here and other cases look fine (the values look always defined), though better remember this case for future reference.