Closed Bug 1074804 Opened 5 years ago Closed 4 years ago

Replace arrays .indexOf with .includes in Places

Categories

(Toolkit :: Places, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mak, Assigned: hassenbtn, Mentored)

References

Details

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

Attachments

(1 file, 21 obsolete files)

47.53 KB, patch
Details | Diff | Splinter Review
We can replace instances of .indexOf() with .contains like:

array.indexOf(something) == -1 => !array.contains(something)
array.indexOf(something) != -1 => array.contains(something)

This is the query pointing at the code points that can be changed:
http://mxr.mozilla.org/mozilla-central/search?string=indexOf\(%23\)+%23%3D+\-\1&regexp=1&find=places&findi=&filter=^[^\0]*%24

Would be nice to have 1 patch per subfolder, so one patch for /browser/components/places/content/, one for /browser/components/places/tests/ and so on.
before doing this we must be sure bug 1069063 will stick into the tree.
Hey. i would like to work on this bug. First time here so how do i submit patches ??
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> before doing this we must be sure bug 1069063 will stick into the tree.

Apparently, that's not the case (see Till's recent message to firefox-dev).
yeah sorry, we cannot do this bug for now, we must wait for .contains to be available again.
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][cannot be fixed until bug 1070767 is fixed]
ok i found out how to submit patches. I'll get back to this bug when .contains is available again
Can i work on this bug now? Has bug 1070767 been fixed?
(In reply to anirudh.gp from comment #6)
> Can i work on this bug now? Has bug 1070767 been fixed?

Not yet. I have added you in the list of persons watching bug 1070767, so that you can be informed once it is fixed.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #7)
> (In reply to anirudh.gp from comment #6)
> > Can i work on this bug now? Has bug 1070767 been fixed?
> 
> Not yet. I have added you in the list of persons watching bug 1070767, so
> that you can be informed once it is fixed.

Ok thanks!
Can you assign this to me? I'l get on with it as soon as bug 1070767 is fixed
I'm sorry but there's no point in booking bugs, when it will be the time, patches will be welcome, until then there's no reason to assign this to anyone.

I'm sure you can find much interesting bugs that are immediately actionable in the list of mentored bugs: http://www.joshmatthews.net/bugsahoy/?js=1&unowned=1
Summary: Replace indexOf with contains in Places → Replace arrays .indexOf with .includes in Places
Hey. Bug 1070767 has been fixed. Can I be assigned to this bug so that I can work on it ?
most likely yes, I'm trying to figure how stable is the current proposal.

It looks like experimental, for ES7, moving to stage 2 of the process.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

Looks like we are also changing string.contains to string.includes in bug 1102219, I'd not expect it to change another time, so I guess we can proceed.
Assignee: nobody → anirudh.gp
OK, we can't do this for now cause the feature is Nightly only. We must again wait on Bug 1070767.
Priority: -- → P4
Whiteboard: [good first bug][lang=js][cannot be fixed until bug 1070767 is fixed] → [good first bug][lang=js]
anirudh.gp, if you didn't start working on this yet, i'm available from today to work on this, i hope you allow me this [good first bug] :)
from the Try run looks like something is wrong...
by looking at the patches, looks like in some cases you replaced a.indexOf(b) == -1 with a.includes(b), but it should instead be !a.includes(b)
Flags: needinfo?(anirudh.gp)
i ll work on that, i somehow missed that, weird ...
ignore that, i thought it would edit the patch so i dont have to resubmit
heh no, you need to edit the patches locally, and submit them again, deprecating the previous version.
With mozReview the process is a little bit more automatized, but then you must also learn how to use it (http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview-user.html) and you have to move the patches there.
For now I'd suggest to just replace single parts with fixed one.

Note, if you wish to merge all the patches into one, I won't complain in this case, cause the change is mostly automatic.
Assignee: anirudh.gp → hassenbtn
Flags: needinfo?(anirudh.gp)
thanks for the tips, that's what i was gonna do, i ll see how to merge them into one patch
Attachment #8669452 - Attachment is obsolete: true
Attachment #8669453 - Attachment is obsolete: true
Attachment #8670268 - Attachment is obsolete: true
Attachment #8669454 - Attachment is obsolete: true
Attachment #8669455 - Attachment is obsolete: true
Attachment #8669459 - Attachment is obsolete: true
Attachment #8669460 - Attachment is obsolete: true
Attachment #8669462 - Attachment is obsolete: true
Changed the patches in question.
Not sure how to merge all patches into one without polluting my repo, is it a simple file concatenation?
if you are using mercurial queues, you can use the hg qfold command
Attachment #8669456 - Attachment is obsolete: true
Attachment #8669457 - Attachment is obsolete: true
Attachment #8669458 - Attachment is obsolete: true
Attachment #8669461 - Attachment is obsolete: true
Attachment #8670267 - Attachment is obsolete: true
Attachment #8670270 - Attachment is obsolete: true
Attachment #8670271 - Attachment is obsolete: true
Attachment #8670272 - Attachment is obsolete: true
Attachment #8670273 - Attachment is obsolete: true
Attachment #8670275 - Attachment is obsolete: true
Attachment #8670276 - Attachment is obsolete: true
can you push to tryserver for me, i'm not allowed since i'm new.
Comment on attachment 8670397 [details] [diff] [review]
Changes for Bug 1074804 - Replace arrays .indexOf with .includes in Places

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

Very nice job!

r=me with the following fixes:

Please edit the commit message so that it just reads as:
Bug 1074804 - Replace arrays .indexOf with .includes in Places. r=mak

::: toolkit/components/places/PlacesTransactions.jsm
@@ +1040,5 @@
>      }
>      if (annos.length > 0) {
>        if (!aRestoring && aExcludingAnnotations.length > 0) {
>          annos = [for(a of annos)
> +                 if (!aExcludingAnnotations.indcludes(a.name)) a];

There is a typo here "indcludes"
Attachment #8670397 - Flags: review+
please, when updating the patch, also ensure it applies on top of a current and updated tree, otherwise our sheriffs won't be able to land it.
thanks, i ll do that and upload again! thanks for the tips!
Not really sure if i did what you asked in the last comment, so what i did is update my local repo, then apply the patch.
Attachment #8670397 - Attachment is obsolete: true
sorry wrong previous file
Attachment #8671507 - Attachment is obsolete: true
(In reply to Hassen ben tanfous from comment #50)
> Created attachment 8671507 [details] [diff] [review]
> Bug 1074804 - Replace arrays .indexOf with .includes in Places. r=mak
> 
> Not really sure if i did what you asked in the last comment, so what i did
> is update my local repo, then apply the patch.

Yep, it's fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ed9915862b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2a6d58e63d30
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.