788229 - String.indexOf should be replaced with String.contains in debugger-view.js

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

We should replace the two instances of string.indexOf in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js with string.contains.

The usages are at:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#384
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger-view.js#398

See http://wiki.ecmascript.org/doku.php?id=harmony:string_extras for documentation on the new String.contains method.

Comment 1

[Matt Vorce]

Attachment: Updated debugger-view.js file

Attached is the updated debugger-view.js file

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Matt Vorce from comment #1)
> Created attachment 659434 [details] [diff] [review]
> Updated debugger-view.js file
> 
> Attached is the updated debugger-view.js file

Can you attach the diff of your changes instead of the updated file please?

Comment 3

[Matt Vorce]

Attachment: Patch for bug 788229

I believe I submitted the patch correctly this time.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 659755 [details] [diff] [review]
Patch for bug 788229

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

The changes to debugger-view.js look fine, but this patch removes 4 tests from the source tree that shouldn't be removed.

Tip: If you are using a Mercurial Queues patch file, you can apply the patch (hg qpush --move patchName), then refresh only the file that you want to keep (hg qref /path/to/debugger-view.js). This will now keep only the debugger-view.js changes in the patch. The other files will still have their changes but won't be part of the patch file. You can now revert the changes to the test files (hg revert --all). After running this last command, your tree will be back to a healthy state and your patch file will only contain the changes to debugger-view.js.

Comment 5

[Matt Vorce]

Attachment: Updated debugger-view.js file

This patch should only include changes to the requested debugger-view.js

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 660133 [details] [diff] [review]
Updated debugger-view.js file

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

This looks good to me.

Please update your Mercurial [ui] username field to include your email address. When you have done it successfully, it should look like:
username = Abraa <[e-mail]>

Tip: Please don't set the checkin flag when requesting review. This flag is only used for multipart patches that have already been granted review but need to land at separate times.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 660133 [details] [diff] [review]
Updated debugger-view.js file

This is backwards. indexOf(...) == -1 means the string does /not/ contain the sub string.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Doh! Sorry, thank you for catching that Dao.

Comment 9

[Matt Vorce]

Attachment: Change String.indexOf to String.contains in debugger-view.js

That was a silly mistake last time. Everything should be correct now.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Matt, have you run the tests with this patch applied?

Comment 11

[Matt Vorce]

(In reply to Jared Wein [:jaws] from comment #10)
> Matt, have you run the tests with this patch applied?

No I have not yet. I will look into that.

Comment 12

[Matt Vorce]

I ran the mochitest-browser-chrome and their was no errors

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b56ba10aea13

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b56ba10aea13

Comment 15

[Rob Campbell [:rc] (:robcee)]

while I'm ok with this bug, in the future please ask a devtools peer for review before landing code.