The default bug view has changed. See this FAQ.

Replace String.indexOf(...) != -1 with String.contains(...) in browser/base/

RESOLVED FIXED in Firefox 18

Status

()

Firefox
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dao, Assigned: AMoz)

Tracking

Trunk
Firefox 18
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 9 obsolete attachments)

(Reporter)

Description

5 years ago
Since bug 772733 landed, 'foo.indexOf(bar) != -1' can be simplified to 'foo.contains(bar)'.

This only applies to strings, not arrays.

Here's a list of candidates:
http://mxr.mozilla.org/mozilla-central/search?string=.indexOf%28&case=1&find=%2Fbrowser%2Fbase%2F&filter=-1
(Assignee)

Comment 1

5 years ago
Sir, I would like to contribute to this bug..
(Assignee)

Comment 2

5 years ago
Sir, I would like to contribute to this bug. I would be highly obliged if u assign the bug to me...Will u explain the bug in detail ?
(Reporter)

Comment 3

5 years ago
Do you already have a local copy of the Firefox code base?
Assignee: nobody → amod.narvekar
(Assignee)

Comment 4

5 years ago
no..had prob in linux. so after reinstalling linux..i am currently downloading the source code.. but i am confused about what exactly the bug is..
(Reporter)

Comment 5

5 years ago
Code is currently using indexOf to check whether a string contains a sub-string. There is no real "bug", i.e. that code should already be working correctly. But it can be simplified, because we have the new 'contains' string method. We should use that instead of indexOf.
(Assignee)

Comment 6

5 years ago
I have downloaded the entire source code. Build took 5 hrs, So sir u can tell me which file has the source code concerned to the bug and what exactly I need to do.
(Reporter)

Comment 7

5 years ago
The 'indexOf' pattern appears in multiple files. They are listed here:
http://mxr.mozilla.org/mozilla-central/search?string=.indexOf%28&case=1&find=%2Fbrowser%2Fbase%2F&filter=-1

However, some of these search results are for arrays. You'll need to figure out where 'indexOf' is used on a string and replace it with 'contains' there.
(Assignee)

Comment 8

5 years ago
Created attachment 654281 [details] [diff] [review]
diff file of aboutSyncTab.js

i have uploaded the patch for /browser/base/content/sync/aboutSyncTabs.js.

In /browser/base/content/newtab/updater.js, aSites is the array of sites remaining in the grid. so i dint replace the indexOf() with contains()..

I am new to this environment hence i apologise for any mistake.
(Assignee)

Comment 9

5 years ago
if my approach is appropriate for this file then i will proceed for the rest of the files.
(Reporter)

Comment 10

5 years ago
Comment on attachment 654281 [details] [diff] [review]
diff file of aboutSyncTab.js

>-        if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
>-            item.getAttribute("title").toLowerCase().indexOf(val) == -1)
>+        if (item.getAttribute("url").toLowerCase().contains(val) == -1 &&
>+            item.getAttribute("title").toLowerCase().contains(val) == -1)

indexOf returns an integer (-1 if the substring wasn't found). contains returns a boolean (false if the substring wasn't found). So you need to replace this:

A.indexOf(B) == -1

with this:

!A.contains(B)
(Assignee)

Comment 11

5 years ago
Created attachment 654643 [details] [diff] [review]
corrected diff file of aboutSyncTab.js

extremely sorry Sir...i have attached patch for /browser/base/content/sync/aboutSyncTabs.js.
sir.. like wise there are 28 candidates.. So shall i upload patch for every file..i think that would be a cumbersome in terms of viewing later..
Attachment #654281 - Attachment is obsolete: true
(Reporter)

Comment 12

5 years ago
Comment on attachment 654643 [details] [diff] [review]
corrected diff file of aboutSyncTab.js

This looks good codewise. The formatting isn't correct, though. You should use spaces rather than tab stops and keep the indentation like it was before.
Attachment #654643 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #654643 - Attachment is obsolete: false
Attachment #654643 - Attachment is patch: true
(Reporter)

Comment 13

5 years ago
(In reply to Amod from comment #11)
> Created attachment 654643 [details] [diff] [review]
> corrected diff file of aboutSyncTab.js
> 
> extremely sorry Sir...i have attached patch for
> /browser/base/content/sync/aboutSyncTabs.js.
> sir.. like wise there are 28 candidates.. So shall i upload patch for every
> file..i think that would be a cumbersome in terms of viewing later..

One monolithic patch would be preferable.
(Assignee)

Comment 14

5 years ago
Created attachment 657204 [details] [diff] [review]
it is a monolithic patch of all the 17 files mentioned

I have made changes in the files..Some candidates were arrays, so I didn't make any changes.
Attachment #654643 - Attachment is obsolete: true
(Assignee)

Comment 15

5 years ago
Sir, I will make necessary changes in indentations once you approve the files correct as codewise.
(Reporter)

Comment 16

5 years ago
Comment on attachment 657204 [details] [diff] [review]
it is a monolithic patch of all the 17 files mentioned

>--- a/browser/base/content/browser-places.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/browser-places.js	Fri Aug 31 02:52:37 2012 +0000
>@@ -977,17 +977,17 @@ var PlacesStarButton = {
>         Components.utils.reportError("PlacesStarButton did not receive current URI");
>         return;
>       }
> 
>       // It's possible that onItemAdded gets called before the async statement
>       // calls back.  For such an edge case, retain all unique entries from both
>       // arrays.
>       this._itemIds = this._itemIds.filter(
>-        function (id) aItemIds.indexOf(id) == -1
>+        function (id) !aItemIds.contains(id)
>       ).concat(aItemIds);

aItemIds is an array.

>@@ -1031,17 +1031,17 @@ var PlacesStarButton = {
>   function PSB_onItemAdded(aItemId, aFolder, aIndex, aItemType, aURI)
>   {
>     if (!this._starIcon) {
>       return;
>     }
> 
>     if (aURI && aURI.equals(this._uri)) {
>       // If a new bookmark has been added to the tracked uri, register it.
>-      if (this._itemIds.indexOf(aItemId) == -1) {
>+      if (!this._itemIds.indexOf(aItemId)) {

This change is wrong, the return value of indexOf still needs to be compared to -1.

>--- a/browser/base/content/browser.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/browser.js	Fri Aug 31 02:52:37 2012 +0000
>@@ -1043,17 +1043,17 @@ var gBrowserInit = {
>           .QueryInterface(Ci.nsIInterfaceRequestor)
>           .getInterface(Ci.nsIXULWindow)
>           .XULBrowserWindow = window.XULBrowserWindow;
>     window.QueryInterface(Ci.nsIDOMChromeWindow).browserDOMWindow =
>       new nsBrowserAccess();
> 
>     // set default character set if provided
>     if ("arguments" in window && window.arguments.length > 1 && window.arguments[1]) {
>-      if (window.arguments[1].indexOf("charset=") != -1) {
>+      if (window.arguments[1].contains("charset=")) {

The original code isn't quite correct, it should have checked whether ...indexOf("charset=") is 0. Similarly, we should use "startsWith" rather than "contains":

      if (window.arguments[1].startsWith("charset=")) {

>@@ -3485,17 +3485,17 @@ function FillHistoryMenu(aParent) {
> 
>     aParent.appendChild(item);
>   }
>   return true;
> }
> 
> function addToUrlbarHistory(aUrlToAdd) {
>   if (aUrlToAdd &&
>-      aUrlToAdd.indexOf(" ") == -1 &&
>+      aUrlToAdd.contains(" ") &&

this should be !aUrlToAdd.contains(" ")

>--- a/browser/base/content/tabbrowser.xml	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/tabbrowser.xml	Fri Aug 31 02:52:37 2012 +0000
>@@ -1804,24 +1804,24 @@
>                 return !tab.closing;
>               }, this);
>             }
> 
>             // Try to find a remaining tab that comes after the given tab
>             var tab = aTab;
>             do {
>               tab = tab.nextSibling;
>-            } while (tab && remainingTabs.indexOf(tab) == -1);
>+            } while (tab && !remainingTabs.contains(tab));
> 
>             if (!tab) {
>               tab = aTab;
> 
>               do {
>                 tab = tab.previousSibling;
>-              } while (tab && remainingTabs.indexOf(tab) == -1);
>+              } while (tab && !remainingTabs.contains(tab));

remainingTabs is an array.

>@@ -2011,17 +2011,17 @@
>         </body>
>       </method>
> 
>       <method name="showOnlyTheseTabs">
>         <parameter name="aTabs"/>
>         <body>
>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>-            if (aTabs.indexOf(tab) == -1)
>+            if (!aTabs.contains(tab))

aTabs is an array.

>--- a/browser/base/content/test/browser_bug555224.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/test/browser_bug555224.js	Fri Aug 31 02:52:37 2012 +0000
>@@ -14,17 +14,17 @@ function afterZoomAndLoad(aCallback, aTa
>       executeSoon(aCallback);
>   }, true);
>   let oldAPTS = FullZoom._applyPrefToSetting;
>   FullZoom._applyPrefToSetting = function(value, browser) {
>     if (!value)
>       value = undefined;
>     oldAPTS.call(FullZoom, value, browser);
>     // Don't reset _applyPrefToSetting until we've seen the about:blank load(s)
>-    if (browser && (browser.currentURI.spec.indexOf("http:") != -1)) {
>+    if (browser && browser.currentURI.spec.contains("http:")) {

We should also use startsWith here.

>--- a/browser/base/content/test/browser_popupNotification.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/test/browser_popupNotification.js	Fri Aug 31 02:52:37 2012 +0000
>@@ -751,17 +751,17 @@ function triggerSecondaryCommand(popup, 
>     for (let i = 0; i <= index; i++)
>       EventUtils.synthesizeKey("VK_DOWN", {});
> 
>     // Activate
>     EventUtils.synthesizeKey("VK_ENTER", {});
>   }, false);
> 
>   // One down event to open the popup
>-  EventUtils.synthesizeKey("VK_DOWN", { altKey: (navigator.platform.indexOf("Mac") == -1) });
>+  EventUtils.synthesizeKey("VK_DOWN", { altKey: (!navigator.platform.contains("Mac")) });

You can remove the parentheses around !navigator.platform.contains("Mac").
(Assignee)

Comment 17

5 years ago
Created attachment 657319 [details] [diff] [review]
rectified errors !

Sir, i am still confused to distinguish between array and string. I tried to use function typeOfx=="string" but was not sure of how to run the script.
(Reporter)

Comment 18

5 years ago
In the latest patch you seem to have reverted changes that were fine in the previous patch, such as the one in browser-tabPreviews.js.
(Assignee)

Comment 19

5 years ago
Sir, but the latest patch which I uploaded now don't highlight any changes in browser-tabPreivew.js
(Reporter)

Comment 20

5 years ago
(In reply to Amod from comment #19)
> Sir, but the latest patch which I uploaded now don't highlight any changes
> in browser-tabPreivew.js

Yes, but it should! :) Those changes were correct.
(Assignee)

Comment 21

5 years ago
Created attachment 657386 [details] [diff] [review]
single patch of all files.

Sorry Sir. it seems that I had forgot to include this file in patch.
(Reporter)

Updated

5 years ago
Attachment #657386 - Attachment is patch: true
(Reporter)

Comment 22

5 years ago
Comment on attachment 657386 [details] [diff] [review]
single patch of all files.

>--- a/browser/base/content/browser-places.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/browser-places.js	Fri Aug 31 12:03:53 2012 +0000
>@@ -1031,17 +1031,17 @@ var PlacesStarButton = {
>   function PSB_onItemAdded(aItemId, aFolder, aIndex, aItemType, aURI)
>   {
>     if (!this._starIcon) {
>       return;
>     }
> 
>     if (aURI && aURI.equals(this._uri)) {
>       // If a new bookmark has been added to the tracked uri, register it.
>-      if (this._itemIds.indexOf(aItemId) == -1) {
>+      if (!this._itemIds.contains(aItemId)) {
>         this._itemIds.push(aItemId);

this._itemIds is an array.
(Assignee)

Comment 23

5 years ago
Created attachment 657431 [details] [diff] [review]
patch.
Attachment #657204 - Attachment is obsolete: true
Attachment #657319 - Attachment is obsolete: true
Attachment #657386 - Attachment is obsolete: true
(Reporter)

Comment 24

5 years ago
Comment on attachment 657431 [details] [diff] [review]
patch.

Alright, this should be correct now. Now some nitpicking:

>--- a/browser/base/content/browser-places.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/browser-places.js	Fri Aug 31 13:46:15 2012 +0000
>@@ -977,17 +977,17 @@ var PlacesStarButton = {
>         Components.utils.reportError("PlacesStarButton did not receive current URI");
>         return;
>       }
> 
>       // It's possible that onItemAdded gets called before the async statement
>       // calls back.  For such an edge case, retain all unique entries from both
>       // arrays.
>       this._itemIds = this._itemIds.filter(
>-        function (id) aItemIds.indexOf(id) == -1
>+      function (id) aItemIds.indexOf(id) == -1

please restore the original indentation

>--- a/browser/base/content/sync/aboutSyncTabs.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/sync/aboutSyncTabs.js	Fri Aug 31 13:46:15 2012 +0000
>@@ -52,19 +52,18 @@ let RemoteTabViewer = {
>     let val = event.target.value.toLowerCase();
>     let numTabs = this._tabsList.getRowCount();
>     let clientTabs = 0;
>     let currentClient = null;
>     for (let i = 0;i < numTabs;i++) {
>       let item = this._tabsList.getItemAtIndex(i);
>       let hide = false;
>       if (item.getAttribute("type") == "tab") {
>-        if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
>-            item.getAttribute("title").toLowerCase().indexOf(val) == -1)
>-          hide = true;
>+	if(!item.getAttribute("url").toLowerCase().contains(val) && !item.getAttribute("title").toLowerCase().contains(val))
>+	hide = true;

ditto

>--- a/browser/base/content/tabbrowser.xml	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/tabbrowser.xml	Fri Aug 31 13:46:15 2012 +0000
>@@ -2011,17 +2011,17 @@
>         </body>
>       </method>
> 
>       <method name="showOnlyTheseTabs">
>         <parameter name="aTabs"/>
>         <body>
>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>-            if (aTabs.indexOf(tab) == -1)
>+            if(aTabs.indexOf(tab) == -1)

please add back the removed space
(Reporter)

Comment 25

5 years ago
Comment on attachment 657431 [details] [diff] [review]
patch.

This should be good to go once comment 24 is addressed.
Attachment #657431 - Flags: feedback+
(Assignee)

Comment 26

5 years ago
Yes Sir, since i had very much problem with my system, i was not able to attend to it. Now i am back and will solve it within 1-2 days.
(Assignee)

Comment 27

5 years ago
Created attachment 659468 [details] [diff] [review]
edited final as mentioned in comment 24.
Attachment #659468 - Flags: review?
(Reporter)

Comment 28

5 years ago
Comment on attachment 659468 [details] [diff] [review]
edited final as mentioned in comment 24.

We need a complete patch like attachment 657431 [details] [diff] [review], with the changes from comment 24 incorporated.
Attachment #659468 - Attachment is obsolete: true
Attachment #659468 - Flags: review?
(Assignee)

Comment 29

5 years ago
Created attachment 659484 [details] [diff] [review]
ptach of all the files !!!

Due to my system problem i was not able to attend earlier. Sorry for the same. I have uploaded a patch of all the files. If there is still any error then mention it. Thanks !!!
Attachment #657431 - Attachment is obsolete: true
(Reporter)

Comment 30

5 years ago
Comment on attachment 659484 [details] [diff] [review]
ptach of all the files !!!

>--- a/browser/base/content/browser-places.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/browser-places.js	Sat Sep 08 17:32:01 2012 +0530
>@@ -977,17 +977,17 @@ var PlacesStarButton = {
>         Components.utils.reportError("PlacesStarButton did not receive current URI");
>         return;
>       }
> 
>       // It's possible that onItemAdded gets called before the async statement
>       // calls back.  For such an edge case, retain all unique entries from both
>       // arrays.
>       this._itemIds = this._itemIds.filter(
>-        function (id) aItemIds.indexOf(id) == -1
>+      	 function (id) aItemIds.indexOf(id) == -1

You replaced spaces with a tab stop here. Please restore the original indentation using spaces only.

>--- a/browser/base/content/sync/aboutSyncTabs.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/sync/aboutSyncTabs.js	Sat Sep 08 17:32:01 2012 +0530
>@@ -52,19 +52,21 @@ let RemoteTabViewer = {
>     let val = event.target.value.toLowerCase();
>     let numTabs = this._tabsList.getRowCount();
>     let clientTabs = 0;
>     let currentClient = null;
>     for (let i = 0;i < numTabs;i++) {
>       let item = this._tabsList.getItemAtIndex(i);
>       let hide = false;
>       if (item.getAttribute("type") == "tab") {
>-        if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
>-            item.getAttribute("title").toLowerCase().indexOf(val) == -1)
>-          hide = true;
>+	if
>+(!item.getAttribute("url").toLowerCase().contains(val) && 
>+
>+!item.getAttribute("title").toLowerCase().contains(val))
>+	hide = true;

This code should be indented exactly as it was before, i.e. the same amount of spaces, no tab stops and no additional line breaks.

>--- a/browser/base/content/tabbrowser.xml	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/tabbrowser.xml	Sat Sep 08 17:32:01 2012 +0530

>       <method name="showOnlyTheseTabs">
>         <parameter name="aTabs"/>
>         <body>
>         <![CDATA[
>           Array.forEach(this.tabs, function(tab) {
>-            if (aTabs.indexOf(tab) == -1)
>+            if	(aTabs.indexOf(tab) == -1)
>               this.hideTab(tab);
>             else
>               this.showTab(tab);
>           }, this);

You introduced a tab stop here as well.

Also, attachment 657431 [details] [diff] [review] contained a change to browser_popupNotification.js, which you seem to have missed here.
(Assignee)

Comment 31

5 years ago
Created attachment 659502 [details] [diff] [review]
patch !
Attachment #659484 - Attachment is obsolete: true
(Reporter)

Comment 32

5 years ago
Comment on attachment 659502 [details] [diff] [review]
patch !

>--- a/browser/base/content/sync/aboutSyncTabs.js	Sat Aug 18 07:22:18 2012 -0400
>+++ b/browser/base/content/sync/aboutSyncTabs.js	Sat Sep 08 21:18:56 2012 +0530

>-        if (item.getAttribute("url").toLowerCase().indexOf(val) == -1 &&
>-            item.getAttribute("title").toLowerCase().indexOf(val) == -1)
>-          hide = true;
>+	if(!item.getAttribute("url").toLowerCase().contains(val) && 
>+          !item.getAttribute("title").toLowerCase().contains(val))
>+	hide = true;

The first line is indented with a tab stop; should be eight spaces instead.
There should also be a space between "if" and "(".
The second line needs to be indented by more two spaces.
The third line is indented with a tab stop; should be ten spaces instead.

Looks great otherwise. Let me know if you'd prefer if I made the above changes.
(Assignee)

Comment 33

5 years ago
Created attachment 659508 [details] [diff] [review]
patch !
Attachment #659502 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #659508 - Attachment is patch: true
(Reporter)

Comment 34

5 years ago
Comment on attachment 659508 [details] [diff] [review]
patch !

Thanks!
Attachment #659508 - Flags: review+
(Assignee)

Comment 35

5 years ago
Finally it was done..Thanks a lot sir. Also do i need to use commit statement. Many of my colleagues told me to do so. I am not much aware of it. Also after this what stages this bug would go under ?
(Reporter)

Comment 36

5 years ago
See https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Committing_the_patch

However you don't need to do anything in this case. I'll land your patch in a bit.
(Reporter)

Comment 37

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c6782197172
(Assignee)

Comment 38

5 years ago
thank you very much !!! Now Looking for another bug mentored by you.
This patch caused a number of test failures (see this log as an example: <https://tbpl.mozilla.org/php/getParsedLog.php?id=15078676&tree=Mozilla-Inbound>) so I backed it out:

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

Comment 40

5 years ago
The browser_bug521216.js change is wrong ('actual' is an array). Re-landed with that dropped:

https://hg.mozilla.org/integration/mozilla-inbound/rev/11afe59db504
https://hg.mozilla.org/mozilla-central/rev/11afe59db504
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
(Assignee)

Comment 42

5 years ago
Thanks !!!
Comment on attachment 659508 [details] [diff] [review]
patch !

Review of attachment 659508 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1047,5 @@
>        new nsBrowserAccess();
>  
>      // set default character set if provided
>      if ("arguments" in window && window.arguments.length > 1 && window.arguments[1]) {
> +      if (window.arguments[1].startsWith("charset=")) {

Why is this startsWith instead of contains?
(Reporter)

Comment 44

5 years ago
Because "charset=" isn't expected to be preceded by anything.
You need to log in before you can comment on or make changes to this bug.