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)
Firefox
General
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)
12.93 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Sir, I would like to contribute to this bug..
Assignee | ||
Comment 2•13 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•13 years ago
|
||
Do you already have a local copy of the Firefox code base?
Assignee: nobody → amod.narvekar
Assignee | ||
Comment 4•13 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•13 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•13 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•13 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•13 years ago
|
||
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•13 years ago
|
||
if my approach is appropriate for this file then i will proceed for the rest of the files.
Reporter | ||
Comment 10•13 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•13 years ago
|
||
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•13 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•13 years ago
|
Attachment #654643 -
Attachment is obsolete: false
Attachment #654643 -
Attachment is patch: true
Reporter | ||
Comment 13•13 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•13 years ago
|
||
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•13 years ago
|
||
Sir, I will make necessary changes in indentations once you approve the files correct as codewise.
Reporter | ||
Comment 16•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
Sir, but the latest patch which I uploaded now don't highlight any changes in browser-tabPreivew.js
Reporter | ||
Comment 20•13 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•13 years ago
|
||
Sorry Sir. it seems that I had forgot to include this file in patch.
Reporter | ||
Updated•13 years ago
|
Attachment #657386 -
Attachment is patch: true
Reporter | ||
Comment 22•13 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•13 years ago
|
||
Attachment #657204 -
Attachment is obsolete: true
Attachment #657319 -
Attachment is obsolete: true
Attachment #657386 -
Attachment is obsolete: true
Reporter | ||
Comment 24•13 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•13 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•13 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•13 years ago
|
||
Attachment #659468 -
Flags: review?
Reporter | ||
Comment 28•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #659484 -
Attachment is obsolete: true
Reporter | ||
Comment 32•13 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•13 years ago
|
||
Attachment #659502 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #659508 -
Attachment is patch: true
Reporter | ||
Comment 34•13 years ago
|
||
Comment on attachment 659508 [details] [diff] [review]
patch !
Thanks!
Attachment #659508 -
Flags: review+
Assignee | ||
Comment 35•13 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•13 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•13 years ago
|
||
Assignee | ||
Comment 38•13 years ago
|
||
thank you very much !!! Now Looking for another bug mentored by you.
Comment 39•13 years ago
|
||
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•13 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
Comment 41•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 42•13 years ago
|
||
Thanks !!!
Comment 43•13 years ago
|
||
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•13 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.
Description
•