895603 - Removed named function expressions from script actors

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

We used to do this because it gave us useful stack traces. Spidermonkey infers this stuff now. Lets remove it.

Comment 1

[Jonathan [:elefont]]

Nick,

I wasn't exactly sure what you meant by "script actors". I removed the named function expressions from script.js (/server/actors/script.js). I ran the following tests & they both passed. 

 $ ./mach xpcshell-test toolkit/devtools/server
 $ ./mach mochitest-browser browser/devtools/debugger

Did I tackle the correct file? If so, I can upload a patch for it. If not, could you please specify which files I should look at? 

Thanks

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jonathan [:elefont] from comment #1)
> Nick,
> 
> I wasn't exactly sure what you meant by "script actors". I removed the named
> function expressions from script.js (/server/actors/script.js). I ran the
> following tests & they both passed. 
> 
>  $ ./mach xpcshell-test toolkit/devtools/server
>  $ ./mach mochitest-browser browser/devtools/debugger
> 
> Did I tackle the correct file? If so, I can upload a patch for it. If not,
> could you please specify which files I should look at? 
> 
> Thanks

Yup, that is indeed the exact correct file! ;)

Please rebase (just to be sure no one snuck any new NFEs in) and upload your patch and flag me for review :)

Comment 3

[Jonathan [:elefont]]

Attachment: bug_895603_remove_named_function_expressions.patch

Nick,

I think I messed up rebasing somehow (I don't know Mercurial or really distributed VC that well). I ended up doing the following:

1. Deleted my entire local repo & cloned it again
2. Rebuilt fx-team again & ran the two test suites I mentioned in my previous comment (all passed).
3. Remove the named function expressions from script.js, ran the same two tests (all passed), then made a patch.
4. Enabled the rebase extension in hgrc, ran 'hg rebase' & got 'nothing to rebase' as output. Based on that output, I believe everything was run against the latest tip of the remote fx-team repo.

And now here's the patch.

Comment 4

[Jonathan [:elefont]]

I should add: What I mean by "I think I messed up" is that while trying to rebase, I believe I did something I shouldn't have since those tests began to fail, 'hg status' was not returning what I expected it to return (it would return some random unversioned file instead of the modified file), as well as a few other strange errors. I think I got around all that though by following the steps described above.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 819538 [details] [diff] [review]
bug_895603_remove_named_function_expressions.patch

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

Looks good to me :)

Just fix the indentation nits below and add the "checkin-needed" keyword once you upload the new patch.

::: toolkit/devtools/server/actors/script.js
@@ +742,1 @@
>                                                   onPacket=function (k) { return k; }) {

Fix the indent, please.

@@ +834,1 @@
>                                           startLocation }) {

I think startLocation can be moved onto the above line now. If moving it up makes the line over 80 chars long, then just make it match the indent with "thread".

@@ +1512,2 @@
>                                                                       aScript,
>                                                                       aScriptsAndOffsetMappings) {

Fix indent please.

@@ +3745,1 @@
>                                 contentType }) {

Fix indent please.

@@ +3883,1 @@
>                                                     aScriptURL) {

Fix indent please.

Comment 6

[Jonathan [:elefont]]

Attachment: bug_895603_fixed_indents.patch

Nick,

Here is a revised patch. Thanks!

--Jonathan

Comment 7

[Ed Morley [:emorley]]

Comment on attachment 823165 [details] [diff] [review]
bug_895603_fixed_indents.patch

Please can you rebase the patch against latest trunk - and then add the checkin-needed keyword to the bug? (The per-patch "checkin?" flag is normally only used when there are multiple patches and only a subset need checking in; by using the keyword instead it makes it easier to update the bug in one submit, rather than two).

[/c/src-gecko/fx-team]$ qimp 895603
Fetching... done
Parsing...Unknown flag checkin
 done
adding 895603 to series file
renamed 895603 -> bug_895603_fixed_indents.patch
applying bug_895603_fixed_indents.patch
patching file toolkit/devtools/server/actors/script.js
Hunk #43 FAILED at 2371
Hunk #44 FAILED at 2410
Hunk #45 FAILED at 2451
Hunk #47 FAILED at 2562
Hunk #71 FAILED at 3825
Hunk #74 FAILED at 3955
6 out of 74 hunks FAILED -- saving rejects to file toolkit/devtools/server/actors/script.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug_895603_fixed_indents.patch

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 823165 [details] [diff] [review]
bug_895603_fixed_indents.patch

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

LGTM!

As Ryan said, the "checkin-needed" keyword is how we track bugs with patches that are ready for check in. No biggie, now you know.

Please rebase and add the "checkin-needed" keyword so that we can land it quickly before anyone else lands any code here as well and breaks your patch and we have to do another rebase! :P

Comment 9

[Jonathan [:elefont]]

Attachment: bug895603-removeNamedFunctionExpressions.patch

Nick,

I ended up doing the following:

1. Cloning out a fresh copy & building fx-team again.
2. Running the following tests suites (all passed)

./mach mochitest-browser browser/devtools/debugger
./mach xpcshell-test toolkit/devtools/server

3. Removed the named function expressions from script.js
4. Saved & created my patch
5. Ran 'hg rebase' and got back 'nothing to rebase' as output.
6. Ran the same test suites (all passed)

Hopefully I did everything correctly this time.

Comment 10

[Jonathan [:elefont]]

I also added the checkin-need keyword to the ticket.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 828481 [details] [diff] [review]
bug895603-removeNamedFunctionExpressions.patch

If it's just a minor rebase, you don't need to re-request review. (Also, you normally wouldn't request checkin until after you have r+ :)...)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7af10a7f0321

Thanks for the patch, Jonathan!

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #11)
> Comment on attachment 828481 [details] [diff] [review]
> bug895603-removeNamedFunctionExpressions.patch
> 
> If it's just a minor rebase, you don't need to re-request review. (Also, you
> normally wouldn't request checkin until after you have r+ :)...)

What RyanVM said :)

Thanks Jonathan! Feel free to ping me for mentorship/guidance/tips/etc if you find another bug you want to work on!

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7af10a7f0321

Comment 15

[Jonathan [:elefont]]

Of course! I like contributing to dev tools :)

However, I just want to be sure that I understand the correct workflow. After reading the comments here & reading through the following, I think I know it: https://wiki.mozilla.org/Bugzilla:Review

1. I clone a clean working copy, and I make my changes. I rebase to make sure I'm running my patch against the latest code (I think this is equivalent to updating your working copy in Subversion, right?). I run some local test(s), and if they pass, I can post the patch to the ticket.

2. When I upload my patch, I can request a review. Requesting a review just involves setting the "review ?" flag on the ticket and entering in the review's name.

3. Once the reviewer approves of my patch, they change it to "review +". At this point, I suppose either I or the review (or perhaps someone else entirely) can add the "checkin-needed" keyword to the ticket. The review will push it to the try server, where tests are run. If successful, my patch is shepherded into Nightly, and eventually into more stable channels. 

Does that seem right?

And Nick, I think I have another ticket assigned to me that'll require some mentoring. I'll touch base with you tomorrow so I can start this weekend. Thanks guys!

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jonathan [:elefont] from comment #15)
> Of course! I like contributing to dev tools :)

:D

> However, I just want to be sure that I understand the correct workflow.
> After reading the comments here & reading through the following, I think I
> know it: https://wiki.mozilla.org/Bugzilla:Review
> 
> 1. I clone a clean working copy, and I make my changes. I rebase to make
> sure I'm running my patch against the latest code (I think this is
> equivalent to updating your working copy in Subversion, right?). I run some
> local test(s), and if they pass, I can post the patch to the ticket.

Yes. The way we rebase with hg and mq is like this:

$ hg qpop
$ hg pull -u
$ hg qpush

This assumes you have only one patch in your queue, if you have multiple patches you can pop and push them all by add "-a" after qpop and qpull.

> 2. When I upload my patch, I can request a review. Requesting a review just
> involves setting the "review ?" flag on the ticket and entering in the
> review's name.

Yup!

> 3. Once the reviewer approves of my patch, they change it to "review +". At
> this point, I suppose either I or the review (or perhaps someone else
> entirely) can add the "checkin-needed" keyword to the ticket. The review
> will push it to the try server, where tests are run. If successful, my patch
> is shepherded into Nightly, and eventually into more stable channels. 

Generally the checkin-needed keyword is added only after it is ready to be checked in immediately. If you are still waiting on try results, you should hold off on adding checkin-needed until the results come back.

Comment 17

[Jonathan [:elefont]]

Ah ok, so 'hg rebase' was not the correct command, it seems. Based on my understanding of the rebase command in Mercurial (I just read this: http://mercurial.selenic.com/wiki/RebaseExtension), we would not want to shelve any changes by putting them into the queue. We just pull in the latest & merge that with our changes in one fell swoop. 

Whereas here, we always want to isolate our changes by creating a patch & popping it off the queue. Then update our local repo and working copy, and then go back & apply the patch. 

It seems like the result is the same, but the second method sounds a bit simpler (and perhaps less error prone).

Thanks for the explanation, Nick. :D