Last Comment Bug 777589 - jstest: Remove a 2 line workaround to a 2 line test fix
: jstest: Remove a 2 line workaround to a 2 line test fix
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla17
Assigned To: Terrence Cole [:terrence]
: general
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 745230
  Show dependency treegraph
Reported: 2012-07-25 17:38 PDT by Terrence Cole [:terrence]
Modified: 2012-08-02 19:08 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v0 (2.45 KB, patch)
2012-07-25 17:38 PDT, Terrence Cole [:terrence]
sphink: review+
Details | Diff | Splinter Review
vFinal: fixed nits (3.43 KB, patch)
2012-07-26 11:29 PDT, Terrence Cole [:terrence]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description User image Terrence Cole [:terrence] 2012-07-25 17:38:57 PDT
Created attachment 645968 [details] [diff] [review]

Oddness in the extreme.
Comment 1 User image Steve Fink [:sfink] [:s:] 2012-07-25 21:39:28 PDT
Comment on attachment 645968 [details] [diff] [review]

Review of attachment 645968 [details] [diff] [review]:

::: js/src/tests/js1_5/extensions/regress-50447-1.js
@@ +102,5 @@
>    var expectedLine = 114;
>    var expectedFileName = 'js1_5/extensions/regress-50447-1.js';
>    if (typeof document == "undefined")
>    {
> +    expectedFileName = expectedFileName;

I'm not going to look at this patch very closely, since it does seem rather odd and if everything works after it, I'm happy.

But why not go ahead and finish simplifying this?

  if (typeof document != "undefined") { ...href.replace... }

The "expectedFileName = expectedFileName" just makes me suspicious of bizarro magic that isn't happening.
Comment 2 User image Terrence Cole [:terrence] 2012-07-26 10:19:24 PDT
Well, I only looked at code that I know runs during shell tests so that it could get checked in with DONTBUILD.  I'll see if there is more in that section that I can clean up before I commit it.
Comment 3 User image Terrence Cole [:terrence] 2012-07-26 11:14:05 PDT
Oh, I see what you mean.  I know I was feeling rather tired yesterday, but still, wow.
Comment 4 User image Terrence Cole [:terrence] 2012-07-26 11:29:01 PDT
Created attachment 646227 [details] [diff] [review]
vFinal: fixed nits

Oh yes, there was a reason that I kept the code that way: the test is checking that the exceptions it raises have the right line number.  I fixed the line numbers for the shell, going to give it a try in the browser to make sure I didn't throw those off.
Comment 5 User image Ed Morley [:emorley] 2012-07-31 09:44:58 PDT
Push backed out for bustage:
Comment 6 User image Terrence Cole [:terrence] 2012-08-02 10:15:44 PDT
Green try run at:
Checked in at:
Comment 7 User image Ryan VanderMeulen [:RyanVM] 2012-08-02 19:08:44 PDT

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