Closed
Bug 1072889
Opened 10 years ago
Closed 6 years ago
Replace array.indexOf(...) != -1 with array.includes(...) in browser/base/
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
DUPLICATE
of bug 1339461
Firefox 35
People
(Reporter: dao, Assigned: ayushmishra.iit)
References
Details
Attachments
(1 file, 1 obsolete file)
|
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
|
Assignee | |
Comment 1•10 years ago
|
||
Can you give some more info about the bug? Which file need to be changed?
|
Reporter | |
Comment 2•10 years ago
|
||
(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.
|
|
Assignee | |
Comment 3•10 years ago
|
||
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?
|
|
Reporter | |
Comment 4•10 years ago
|
||
(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");
|
|
Assignee | |
Comment 5•10 years ago
|
||
And what about lines 215 and 214 in browser/base/content/test/general/browser_contentAreaClick.js ?
|
|
Reporter | |
Comment 6•10 years ago
|
||
Basically the same thing:
ok(gInvokedMethods.contains(aExpectedMethodName),
gCurrentTest.desc + ":" + aExpectedMethodName + " was invoked");
|
|
Assignee | |
Comment 7•10 years ago
|
||
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?
|
|
Reporter | |
Comment 8•10 years ago
|
||
Yes, exactly!
|
|
Assignee | |
Comment 9•10 years ago
|
||
Can you assign this bug to me?
|
|
Reporter | |
Updated•10 years ago
|
Assignee: nobody → ayushmishra.iit
|
|
Assignee | |
Comment 10•10 years ago
|
||
Created patch, please review it. :)
Attachment #8495897 -
Flags: review?(dao)
|
|
Reporter | |
Comment 11•10 years ago
|
||
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+
|
|
Assignee | |
Comment 12•10 years ago
|
||
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?
|
|
Assignee | |
Comment 13•10 years ago
|
||
I didn't change any indentation. I think there are 5 spaces at each level.
|
|
Assignee | |
Comment 14•10 years ago
|
||
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?
|
|
Assignee | |
Comment 15•10 years ago
|
||
Fixed indentation in multiple line statements and the if-else part.
Please review. :)
Attachment #8495897 -
Attachment is obsolete: true
Attachment #8495958 -
Flags: review?(dao)
|
|
Reporter | |
Comment 16•10 years ago
|
||
Comment on attachment 8495958 [details] [diff] [review]
Indentation fixed.
Yes, this is what I meant :)
Thanks.
Attachment #8495958 -
Flags: review?(dao) → review+
|
|
Reporter | |
Comment 17•10 years ago
|
||
|
|
Assignee | |
Comment 18•10 years ago
|
||
What does the above link mean? And generally how does the bug status get resolved? What's the process?
|
|
Reporter | |
Comment 19•10 years ago
|
||
(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.
|
|
Reporter | |
Comment 20•10 years ago
|
||
And generally, a bug get resolved as fixed when the patch hits mozilla-central.
|
||
Comment 21•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
|
||
Comment 22•10 years ago
|
||
Bug 1069063 is apparently Nightly-only, and may be backed out, so we should back this patch out. Not ready yet, unfortunately.
|
||
Comment 23•10 years ago
|
||
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]
|
||
Comment 24•10 years ago
|
||
.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/
|
||
Updated•6 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 6 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•