Closed Bug 783025 Opened 13 years ago Closed 13 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 18

People

(Reporter: dao, Assigned: AMoz)

Details

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

Attachments

(1 file, 9 obsolete files)

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
Sir, I would like to contribute to this bug..
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 ?
Do you already have a local copy of the Firefox code base?
Assignee: nobody → amod.narvekar
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..
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.
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.
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.
Attached patch diff file of aboutSyncTab.js (obsolete) — Splinter Review
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.
if my approach is appropriate for this file then i will proceed for the rest of the files.
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)
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
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
Attachment #654643 - Attachment is obsolete: false
Attachment #654643 - Attachment is patch: true
(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.
I have made changes in the files..Some candidates were arrays, so I didn't make any changes.
Attachment #654643 - Attachment is obsolete: true
Sir, I will make necessary changes in indentations once you approve the files correct as codewise.
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").
Attached patch rectified errors ! (obsolete) — Splinter Review
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.
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.
Sir, but the latest patch which I uploaded now don't highlight any changes in browser-tabPreivew.js
(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.
Attached patch single patch of all files. (obsolete) — Splinter Review
Sorry Sir. it seems that I had forgot to include this file in patch.
Attachment #657386 - Attachment is patch: true
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.
Attached patch patch. (obsolete) — Splinter Review
Attachment #657204 - Attachment is obsolete: true
Attachment #657319 - Attachment is obsolete: true
Attachment #657386 - Attachment is obsolete: true
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
Comment on attachment 657431 [details] [diff] [review] patch. This should be good to go once comment 24 is addressed.
Attachment #657431 - Flags: feedback+
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.
Attachment #659468 - Flags: review?
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?
Attached patch ptach of all the files !!! (obsolete) — Splinter Review
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
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.
Attached patch patch ! (obsolete) — Splinter Review
Attachment #659484 - Attachment is obsolete: true
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.
Attached patch patch !Splinter Review
Attachment #659502 - Attachment is obsolete: true
Attachment #659508 - Attachment is patch: true
Comment on attachment 659508 [details] [diff] [review] patch ! Thanks!
Attachment #659508 - Flags: review+
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 ?
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.
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
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
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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?
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.

Attachment

General

Creator:
Created:
Updated:
Size: