Last Comment Bug 757562 - Remove JSRESOLVE_CLASSNAME
: Remove JSRESOLVE_CLASSNAME
Status: RESOLVED FIXED
[js:t]
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-22 12:02 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-05-25 08:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.10 KB, patch)
2012-05-22 12:02 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dmandelin: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-22 12:02:11 PDT
Created attachment 626123 [details] [diff] [review]
Patch

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.)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-22 12:06:31 PDT
CCing other DOM people for tribal knowledge about the necessity of this flag's semantics...
Comment 2 Boris Zbarsky [:bz] 2012-05-22 12:28:23 PDT
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 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-22 14:35:12 PDT
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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-22 14:52:43 PDT
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 5 David Mandelin [:dmandelin] 2012-05-22 18:44:15 PDT
Comment on attachment 626123 [details] [diff] [review]
Patch

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

This is a good thing.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 00:19:39 PDT
No response in the newsgroups that this was being used -- landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a33088aeb47b
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-25 00:24:04 PDT
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)
Comment 8 Ed Morley [:emorley] 2012-05-25 08:21:41 PDT
https://hg.mozilla.org/mozilla-central/rev/a33088aeb47b

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