757562 - Remove JSRESOLVE_CLASSNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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]]

CCing other DOM people for tribal knowledge about the necessity of this flag's semantics...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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]]

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]]

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]]

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]]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/a33088aeb47b