Last Comment Bug 792968 - Replace some regular expression string matches with String.startsWith and replace /^https?/ URI scheme tests with /^https?$/
: Replace some regular expression string matches with String.startsWith and rep...
Status: RESOLVED FIXED
[good first bug][mentor=dao][lang=js]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Yosy
:
Mentors:
Depends on: 797430 845055
Blocks: 934103
  Show dependency treegraph
 
Reported: 2012-09-20 13:10 PDT by Dão Gottwald [:dao]
Modified: 2013-11-02 06:44 PDT (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
candidates (3.59 KB, text/plain)
2012-09-20 13:18 PDT, Dão Gottwald [:dao]
no flags Details
patch (24.37 KB, patch)
2012-09-22 09:34 PDT, Yosy
no flags Details | Diff | Splinter Review
fixed patch (24.45 KB, patch)
2012-09-22 09:48 PDT, Yosy
dao+bmo: review+
Details | Diff | Splinter Review
patch with review comments addressed (19.09 KB, patch)
2012-09-23 07:45 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review
patch with review comments addressed, 2nd attempt (19.10 KB, patch)
2012-09-23 12:10 PDT, Dão Gottwald [:dao]
no flags Details | Diff | Splinter Review

Description Dão Gottwald [:dao] 2012-09-20 13:10:38 PDT
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] 2012-09-20 13:18:59 PDT
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.
Comment 2 Yosy 2012-09-21 09:23:04 PDT
I can do this.. this should be on "mozilla-central" (currently I have "integerations/fx-team")?
Comment 3 Dão Gottwald [:dao] 2012-09-21 12:12:02 PDT
fx-team will work just as well.
Comment 4 Yosy 2012-09-22 09:07:02 PDT
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 2012-09-22 09:34:49 PDT
Created attachment 663713 [details] [diff] [review]
patch
Comment 6 Yosy 2012-09-22 09:35:53 PDT
submitted a patch, I won't be here in the next weeks, but I will follow from my smartphone.
Comment 7 Yosy 2012-09-22 09:48:57 PDT
Created attachment 663716 [details] [diff] [review]
fixed patch

Forgot to remove the old if statements in two files.
Comment 8 Dão Gottwald [:dao] 2012-09-23 07:25:24 PDT
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] 2012-09-23 07:40:52 PDT
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] 2012-09-23 07:43:11 PDT
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] 2012-09-23 07:45:34 PDT
Created attachment 663812 [details] [diff] [review]
patch with review comments addressed
Comment 13 :Benjamin Peterson 2012-09-23 09:28:25 PDT
Backed out for causing mochitest-oth failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a21e1c1d2ba8
Comment 14 Dão Gottwald [:dao] 2012-09-23 12:10:26 PDT
Created attachment 663837 [details] [diff] [review]
patch with review comments addressed, 2nd attempt

another missing closing parenthesis
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-23 18:29:49 PDT
https://hg.mozilla.org/mozilla-central/rev/9e32aa6fe544
Comment 17 Marco Bonardo [::mak] 2012-10-03 12:51:31 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.