Remove JSRESOLVE_WITH

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({dev-doc-complete})

unspecified
mozilla16
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

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
See Also: → bug 463997
Created attachment 627295 [details] [diff] [review]
Patch
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.
Attachment #627295 - Flags: review?(dmandelin)
https://tbpl.mozilla.org/?tree=Try&rev=d7880a675681 is all green so far with a J result or two on every platform, fwiw.
Attachment #627295 - Flags: review?(dmandelin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/12b0df6ff60f \o/
Target Milestone: --- → mozilla16
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
Keywords: dev-doc-complete
https://hg.mozilla.org/mozilla-central/rev/12b0df6ff60f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.