Closed Bug 1072889 Opened 10 years ago Closed 5 years ago

Replace array.indexOf(...) != -1 with array.includes(...) in browser/base/

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1339461
Milestone:
Firefox 35

People

(Reporter: dao, Assigned: ayushmishra.iit)

References

Details

Attachments

(1 file, 1 obsolete file)

Indentation fixed.
16.18 KB, patch
dao
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #783025 +++

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

Here's a list of candidates:
http://mxr.mozilla.org/mozilla-central/search?string=.indexOf%28&case=1&find=%2Fbrowser%2Fbase%2F&filter=-1
Can you give some more info about the bug? Which file need to be changed?
(In reply to Ayush Mishra from comment #1)
> Can you give some more info about the bug? Which file need to be changed?

Basically all files listed on the search results page I linked to in comment 0.
What does line 830 of this file do?
/browser/base/content/test/general/browser_devices_get_user_media.js
How do I change the arguments passed to this function?
(In reply to Ayush Mishra from comment #3)
> What does line 830 of this file do?
> /browser/base/content/test/general/browser_devices_get_user_media.js
> How do I change the arguments passed to this function?

You could leave this as is or change it to:

  ok(!labels.contains(alwaysLabel), "The 'Always Allow' item isn't shown");
And what about lines 215 and 214 in browser/base/content/test/general/browser_contentAreaClick.js ?
Basically the same thing:

  ok(gInvokedMethods.contains(aExpectedMethodName),
     gCurrentTest.desc + ":" + aExpectedMethodName + " was invoked");
All the places where array.indexOf(someThing) == -1 was there, I did !array.contains(someThing). And places where array.indexOf(someThing)!=-1 was there, I did array.contains(someThing). 

Is this fine?
Yes, exactly!
Can you assign this bug to me?
Assignee: nobody → ayushmishra.iit
Attached patch Patch for this bug. (obsolete) — — Splinter Review 
Created patch, please review it. :)
Attachment #8495897 - Flags: review?(dao)
Comment on attachment 8495897 [details] [diff] [review]
Patch for this bug.

>--- a/browser/base/content/sync/setup.js
>+++ b/browser/base/content/sync/setup.js
>@@ -147,19 +147,19 @@ var gSyncSetup = {
> 
>     // Generate a new passphrase so that Weave.Service.login() will
>     // actually do something.
>     let passphrase = Weave.Utils.generatePassphrase();
>     Weave.Service.identity.syncKey = passphrase;
> 
>     // Only open the dialog if username + password are actually correct.
>     Weave.Service.login();
>-    if ([Weave.LOGIN_FAILED_INVALID_PASSPHRASE,
>+    if (![Weave.LOGIN_FAILED_INVALID_PASSPHRASE,
>          Weave.LOGIN_FAILED_NO_PASSPHRASE,
>-         Weave.LOGIN_SUCCEEDED].indexOf(Weave.Status.login) == -1) {
>+         Weave.LOGIN_SUCCEEDED].contains(Weave.Status.login)) {

Please indent the last two lines with another space.

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -2413,17 +2413,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))
>               this.hideTab(tab);
>             else
>               this.showTab(tab);

How about:

  if (aTabs.contains(tab))
    this.showTab(tab);
  else
    this.hideTab(tab);

>--- a/browser/base/content/test/general/browser_contentAreaClick.js
>+++ b/browser/base/content/test/general/browser_contentAreaClick.js
>@@ -207,17 +207,17 @@ let gClickHandler = {
>     let isPanelClick = linkId == "panellink";
>     gTestWin.contentAreaClick(event, isPanelClick);
>     let prevent = event.defaultPrevented;
>     is(prevent, gCurrentTest.preventDefault,
>        gCurrentTest.desc + ": event.defaultPrevented is correct (" + prevent + ")")
> 
>     // Check that all required methods have been called.
>     gCurrentTest.expectedInvokedMethods.forEach(function(aExpectedMethodName) {
>-      isnot(gInvokedMethods.indexOf(aExpectedMethodName), -1,
>+      ok(gInvokedMethods.contains(aExpectedMethodName),
>             gCurrentTest.desc + ":" + aExpectedMethodName + " was invoked");

Please adjust the indentation in the last line (needs three spaces less).

Looks good otherwise. Thanks!
Attachment #8495897 - Flags: review?(dao) → review+
As I create a patch already. Now when i create a patch it's from the current patched version. I want it to be patched diffed from orignal. How do I remove the changes I made? without re-cloning the repo?
I didn't change any indentation. I think there are 5 spaces at each level.
Ohh.. For the this file browser/base/content/test/general/browser_contentAreaClick.js
Is there some rule like arguments to a function must come below each other if they are in different line? 
Is that why you are asking me for 3 less spaces?
Fixed indentation in multiple line statements and the if-else part.
Please review. :)
Attachment #8495897 - Attachment is obsolete: true
Attachment #8495958 - Flags: review?(dao)
Comment on attachment 8495958 [details] [diff] [review]
Indentation fixed.

Yes, this is what I meant :)
Thanks.
Attachment #8495958 - Flags: review?(dao) → review+
What does the above link mean? And generally how does the bug status get resolved? What's the process?
(In reply to Ayush Mishra from comment #18)
> What does the above link mean? And generally how does the bug status get
> resolved? What's the process?

I pushed your patch to the fx-team integration branch (https://wiki.mozilla.org/Tree_Rules#Integration_Branches). Since tests are green (https://tbpl.mozilla.org/?tree=Fx-Team&rev=cfbe5fc74ea2), it will automatically make its way to mozilla-central from where Firefox nightly builds are built. In a few weeks it will be merged to mozilla-aurora, six weeks later to mozilla-beta and finally mozilla-release.
And generally, a bug get resolved as fixed when the patch hits mozilla-central.
https://hg.mozilla.org/mozilla-central/rev/010c008febdb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Bug 1069063 is apparently Nightly-only, and may be backed out, so we should back this patch out. Not ready yet, unfortunately.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3c341e9e6639.

Sorry, Ayush, we'll not be able to use this patch for the time being. I'm marking this bug as blocked by bug 1070767. If and when that lands, we can take this one up again.
Mentor: dao
Status: RESOLVED → REOPENED
Depends on: 1070767
Resolution: FIXED → ---
Whiteboard: [good first bug][lang=js]
.includes is the new name
Summary: Replace array.indexOf(...) != -1 with array.contains(...) in browser/base/ → Replace array.indexOf(...) != -1 with array.includes(...) in browser/base/
Status: REOPENED → RESOLVED
Closed: 10 years ago → 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: