Closed Bug 757562 Opened 8 years ago Closed 8 years ago
The SpiderMonkey property work would be greatly helped, and clarified, if we can get rid of more of the resolve flags, ideally all of them. JSRESOLVE_CLASSNAME is only very rarely used, and it seems feasible to get rid of it without much trouble. This try push says everything except Windows is green just removing the flag entirely: https://tbpl.mozilla.org/?tree=Try&rev=b8a69854c6f4 Windows hit some issues based on the qparent I was using, so I rebased against a newer tree and repushed to get results there: https://tbpl.mozilla.org/?tree=Try&rev=91294e26d74c If we hit anything, I'm pretty sure it can be fixed in relatively short order. If we don't hit anything, that suggests either that this removal is super-easy or that we need more tests for this stuff. If it is the case that we don't hit any failures in our tests from removing the flag, tribal knowledge of when this flag is "necessary" would be extremely helpful. (The history behind it might also be mildly interesting, but I doubt it's important to know -- just what if any behavior may need preserving is all that actually matters.)
CCing other DOM people for tribal knowledge about the necessity of this flag's semantics...
We use JSRESOLVE_CLASSNAME outside Spidermonkey in two places: 1) The global scope polluter. I'm not sure why we need it there; wouldn't classnames just end up resolved on the global before we ever get to the polluter (which is on the global's proto chain)? 2) In XBL code when looking up classnames. This was probably just copy/paste from the jseng code for doing that.
Comment on attachment 626123 [details] [diff] [review] Patch Re 1), that's what I was thinking when I read the code. (There might be a difference if you pulled out the polluter object manually and did property lookups on that, but that seems way way way in the weeds.) I hadn't formulated a hypothesis for 2, but that seems plausible and happy-making if that's the case. The Windows try results look green enough that I think we're good to go on this for review now.
Attachment #626123 - Flags: review?(dmandelin)
The odds any JSAPI users are using this meaningfully seems super-low, but I posted to the newsgroup just the same: https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.js-engine/WPxUzfK17Bs
Comment on attachment 626123 [details] [diff] [review] Patch Review of attachment 626123 [details] [diff] [review]: ----------------------------------------------------------------- This is a good thing.
Attachment #626123 - Flags: review?(dmandelin) → review+
No response in the newsgroups that this was being used -- landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a33088aeb47b
Target Milestone: --- → mozilla15
Documented on MDN: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference (marking it as obsolete) https://developer.mozilla.org/en/SpiderMonkey/1.8.8 (noting it as a dropped feature -- or should it have been listed in the API change section? dunno) https://developer.mozilla.org/En/SpiderMonkey/JSAPI_Reference/JSNewResolveOp (the flag's dead, Jim)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.