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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: dao, Assigned: Yosy)

Tracking

Trunk
Firefox 18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=dao][lang=js])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 663134 [details]
candidates

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

Comment 2

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

Comment 3

5 years ago
fx-team will work just as well.
Assignee: nobody → yosy101
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 663713 [details] [diff] [review]
patch
(Assignee)

Comment 6

5 years ago
submitted a patch, I won't be here in the next weeks, but I will follow from my smartphone.
(Assignee)

Comment 7

5 years ago
Created attachment 663716 [details] [diff] [review]
fixed patch

Forgot to remove the old if statements in two files.
Attachment #663713 - Attachment is obsolete: true
(Reporter)

Comment 8

5 years ago
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!
Attachment #663716 - Flags: review+
(Reporter)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
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
(Reporter)

Comment 11

5 years ago
Created attachment 663812 [details] [diff] [review]
patch with review comments addressed
(Reporter)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca1f19e4c2a
Summary: Replace some regular expression string matches with String.startsWith → Replace some regular expression string matches with String.startsWith and replace /^https?/ URI scheme tests with /^https?$/

Comment 13

5 years ago
Backed out for causing mochitest-oth failures:

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

Comment 14

5 years ago
Created attachment 663837 [details] [diff] [review]
patch with review comments addressed, 2nd attempt

another missing closing parenthesis
Attachment #663812 - Attachment is obsolete: true
(Reporter)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e32aa6fe544
https://hg.mozilla.org/mozilla-central/rev/9e32aa6fe544
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Depends on: 797430
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.

Updated

4 years ago
Depends on: 845055
Blocks: 934103
You need to log in before you can comment on or make changes to this bug.