Last Comment Bug 758499 - Remove JSRESOLVE_WITH
: Remove JSRESOLVE_WITH
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-24 20:48 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-06 08:45 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (2.55 KB, patch)
2012-05-25 11:08 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-24 20:48:52 PDT
JSRESOLVE_WITH was added long ago in bug 463997 as a hackaround for some oddness with |with (window) var x = 17;|.  The details of exactly went wrong are never *quite* fully expressed in the bug with an exactly minimal browser testcase, alas, but that was the expressed gist of it.

With the changes in bug 632003 and bug 577325, however, I suspect the hackaround that JSRESOLVE_WITH implemented to be no longer necessary -- we define vars and names declared by function statements rather than set them.  Thus I expect it's no longer necessary to special-case window expandos when being set for vars inside |with|, and it's no longer necessary to special-case with-based lookups via the resolve flag mechanism.

And indeed, if I remove JSRESOLVE_WITH and run JS tests, the test mentioned in bug 463997 as failing does not fail!  Let's do this.

...after seeing if embedders (sigh) are using this.  Given the extreme obscurity of it and that it was added only to placate Gecko, I am cautiously optimistic.  Thread started:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.js-engine/DT1cqbDir0A
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 11:08:10 PDT
Created attachment 627295 [details] [diff] [review]
Patch
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 16:13:32 PDT
Comment on attachment 627295 [details] [diff] [review]
Patch

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

Feel free to wait however long you feel is right for embedder feedback (I only posted last night, so this is still a bit fresh).  But note that this flag was never even documented anywhere, so the odds anyone but us used it, and will comment in the newsgroup, are slim to none.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 16:15:50 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d7880a675681 is all green so far with a J result or two on every platform, fwiw.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 10:03:23 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b0df6ff60f \o/
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-05 10:13:33 PDT
I documented the presence of JSRESOLVE_WITH, and its obsoletion, here:

https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Reference/JSNewResolveOp

Also I added a note about its removal to the 1.8.8 notes:

https://developer.mozilla.org/en/SpiderMonkey/1.8.8
Comment 6 Ed Morley [:emorley] 2012-06-06 08:45:08 PDT
https://hg.mozilla.org/mozilla-central/rev/12b0df6ff60f

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