1081520 - use String.includes() instead of String.indexOf() where appropriate

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Tomislav Jovanovic :zombie]

the web is the best.. the web is the worst.. the web is why we can't have nice things (bug 1075059).

but at least we have String.contains(), and it's much more readable than  | ~!string.indexOf(substr) |  or  | string.indexOf(substr) >= 0 |.

Comment 1

[yt]

I'd like to work on this bug.

Comment 2

[Tomislav Jovanovic :zombie]

ok, assigned.

so the issue here is that we often use string.indexOf() method to check cf a string contains some other substring. the problem is that indexOf() was meant to do more than that, it also returns the position of the substring, or -1 in case where it's not present.

the new string.contains() method just returns true/false, which makes for much more readable code where that's all that's needed.

suggested steps for fixing this are:

1) use a grep, or some similar tool, to find all source files where .indexOf() method is used
2) read the code around each use to verify .indexOf() is called on a string (and not an array)
3) also filter out uses where the .indexOf() is used for position info, and not just as "contains"
4) edit to use .contains() where appropriate, and run package test for each file as you finish them

let me know if any of that was not clear, or you have other questions..

Comment 3

[yt]

Attachment: submitted PR 1678

Comment 4

[yt]

I'm not finished of course, but I wanted to submit a PR just to make sure I'm doing it right before continuing. Tests passed though. Should be fine.

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8505836 [details] [review]
submitted PR 1678

i'm afraid this isn't correct.

for this bug, the simple find and replace approach is not appropriate. the .indexOf() and .contains() methods are not equivalent. you can check out the documentation for both on mdn:

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/indexOf
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/String/contains

but in short, the .indexOf() is a more general function that returns the position of a substring in a big string, or -1 if it's not found. 

testing if that return value is greater then or equal to zero (or the special trick with ~) is equivalent to calling the .contains() method.


the first change in "httpd.js" is violating the point 3) from my comment above. it's using the index of the substring for something more than just deciding if it's "contained" in the bigger string or not.

(and btw, that whole file "httpd.js" was replaced, and is not in the current tree. you should pull upstream changes to your own fork of addon-sdk to avoid working on already changed files)


the second change in "loader.js" is using the special "~" syntax which is a bit hard to explain, but basically converts the -1 result of .indexOf() to false, and every other result to true, so basically to what we want to use the .contains() method for.

to see how your current change doesn't behave like the old code, check the examples here:

http://jsbin.com/zisewepagafa/2/edit?js,console (use the "run" button to see the results)

feel free to play with that example code on jsbin to figure out how to actually get the same result in the changed code (hint, the "~" is not needed).

...

uh, that was a bit wordier than i expected, but i hope it's at least somewhat clear..

Comment 6

[yt]

Ah, ok. Thanks for the input. That's what I was afraid of. I'll see if I can fix it.

Comment 7

[yt]

Is string.contains still necessary in light of String.prototype.includes()?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/includes

Comment 8

[Tooru Fujisawa [:arai]]

String.prototype.contains was renamed to String.prototype.includes (bug 1102219),
and String.prototype.contains will be removed shortly (bug 1103588).

Comment 9

[Andy McKay]

Because of the difficulty finding mentors and the expected life span of the SDK, we are removing [good first bug] from remaining SDK bugs.

Comment 10

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562