Last Comment Bug 788229 - String.indexOf should be replaced with String.contains in debugger-view.js
: String.indexOf should be replaced with String.contains in debugger-view.js
Status: RESOLVED FIXED
[good first bug][mentor=jaws][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 18
Assigned To: Matt Vorce
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-04 12:00 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-09-21 06:01 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Updated debugger-view.js file (101.55 KB, patch)
2012-09-07 18:52 PDT, Matt Vorce
no flags Details | Diff | Review
Patch for bug 788229 (3.20 KB, patch)
2012-09-10 10:00 PDT, Matt Vorce
jaws: review-
Details | Diff | Review
Updated debugger-view.js file (1.09 KB, patch)
2012-09-11 09:53 PDT, Matt Vorce
jaws: review+
dao+bmo: review-
Details | Diff | Review
Change String.indexOf to String.contains in debugger-view.js (1.11 KB, patch)
2012-09-12 07:30 PDT, Matt Vorce
jaws: review+
Details | Diff | Review

Comment 1 Matt Vorce 2012-09-07 18:52:51 PDT
Created attachment 659434 [details] [diff] [review]
Updated debugger-view.js file

Attached is the updated debugger-view.js file
Comment 2 Panos Astithas [:past] 2012-09-10 00:59:20 PDT
(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 2012-09-10 10:00:05 PDT
Created attachment 659755 [details] [diff] [review]
Patch for bug 788229

I believe I submitted the patch correctly this time.
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-09-11 01:37:00 PDT
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 2012-09-11 09:53:36 PDT
Created attachment 660133 [details] [diff] [review]
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) 2012-09-12 05:53:53 PDT
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.
Comment 7 Dão Gottwald [:dao] 2012-09-12 05:56:43 PDT
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) 2012-09-12 06:02:01 PDT
Doh! Sorry, thank you for catching that Dao.
Comment 9 Matt Vorce 2012-09-12 07:30:26 PDT
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.
Comment 10 Jared Wein [:jaws] (please needinfo? me) 2012-09-12 07:35:05 PDT
Matt, have you run the tests with this patch applied?
Comment 11 Matt Vorce 2012-09-12 07:48:56 PDT
(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 2012-09-13 09:13:41 PDT
I ran the mochitest-browser-chrome and their was no errors
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-09-13 19:01:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56ba10aea13
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-09-14 18:21:05 PDT
https://hg.mozilla.org/mozilla-central/rev/b56ba10aea13
Comment 15 Rob Campbell [:rc] (:robcee) 2012-09-21 06:01:09 PDT
while I'm ok with this bug, in the future please ask a devtools peer for review before landing code.

Note You need to log in before you can comment on or make changes to this bug.