Closed Bug 792968 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: dao, Assigned: yosy101)

References

Details

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

Attachments

(3 files, 2 obsolete files)

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.
Attached file candidates
It looks like the MXR has started choking on that search. Luckily I still had the results page cached. Here's the list.
I can do this.. this should be on "mozilla-central" (currently I have "integerations/fx-team")?
fx-team will work just as well.
Assignee: nobody → yosy101
I am using mozilla-central finally, I have done the changes how I can test them? that I don't break any functionality?
Attached patch patch (obsolete) — Splinter Review
submitted a patch, I won't be here in the next weeks, but I will follow from my smartphone.
Attached patch fixed patchSplinter Review
Forgot to remove the old if statements in two files.
Attachment #663713 - Attachment is obsolete: true
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+
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 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
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?$/
Backed out for causing mochitest-oth failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21e1c1d2ba8
another missing closing parenthesis
Attachment #663812 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/9e32aa6fe544
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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.
Depends on: 845055
You need to log in before you can comment on or make changes to this bug.