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.

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

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.

OK, we can't do this for now cause the feature is Nightly only. We must again wait on Bug 1070767.

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] :)

test results, https://treeherder.mozilla.org/#/jobs?repo=try&revision=948d0056d9c2

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)

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.

thanks for the tips, that's what i was gonna do, i ll see how to merge them into one patch

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

can you push to tryserver for me, i'm not allowed since i'm new.

test here, https://treeherder.mozilla.org/#/jobs?repo=try&revision=293dc7c0b3ae

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"

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.

sorry wrong previous file

(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