The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Developer Tools: Debugger
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jaws, Assigned: Matt Vorce)

Tracking

Trunk
Firefox 18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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.
(Assignee)

Comment 1

5 years ago
Created attachment 659434 [details] [diff] [review]
Updated debugger-view.js file

Attached is the updated debugger-view.js file
(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?
(Assignee)

Comment 3

5 years ago
Created attachment 659755 [details] [diff] [review]
Patch for bug 788229

I believe I submitted the patch correctly this time.
Attachment #659434 - Attachment is obsolete: true
Attachment #659755 - Flags: review?(jaws)
Attachment #659755 - Flags: checkin?(lucasr.at.mozilla)
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.
Attachment #659755 - Flags: review?(jaws)
Attachment #659755 - Flags: review-
Attachment #659755 - Flags: checkin?(lucasr.at.mozilla)
(Assignee)

Comment 5

5 years ago
Created attachment 660133 [details] [diff] [review]
Updated debugger-view.js file

This patch should only include changes to the requested debugger-view.js
Attachment #659755 - Attachment is obsolete: true
Attachment #660133 - Flags: review?(jaws)
Attachment #660133 - Flags: checkin?(lucasr.at.mozilla)
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 <address@example.com>

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.
Attachment #660133 - Flags: review?(jaws)
Attachment #660133 - Flags: review+
Attachment #660133 - Flags: checkin?(lucasr.at.mozilla)
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.
Attachment #660133 - Flags: review-
Doh! Sorry, thank you for catching that Dao.
(Assignee)

Comment 9

5 years ago
Created attachment 660429 [details] [diff] [review]
Change String.indexOf to String.contains in debugger-view.js

That was a silly mistake last time. Everything should be correct now.
Attachment #660133 - Attachment is obsolete: true
Attachment #660429 - Flags: review?(lucasr.at.mozilla)
Attachment #660429 - Flags: review?(jaws)
Matt, have you run the tests with this patch applied?
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
I ran the mochitest-browser-chrome and their was no errors
Attachment #660429 - Flags: review?(lucasr.at.mozilla)
Attachment #660429 - Flags: review?(jaws)
Attachment #660429 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56ba10aea13
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b56ba10aea13
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
while I'm ok with this bug, in the future please ask a devtools peer for review before landing code.
You need to log in before you can comment on or make changes to this bug.