Drop support for custom [object XrayWrapper [object ClassName]] stringification

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla33
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Right now, XrayWrapper::getPropertyDescriptor resolves a custom toString method that does all kinds of crazy stuff to produce the familiar "[object XrayWrapper [object HTMLDocument]]" stringification.

This code is problematic in a bunch of ways. In particular, it doesn't work when HasPrototype=1. This has two consequences:
* We don't get the [object XrayWrapper [...]] annotation for JSXrayTraits (and for the OpaqueXrayTraits I'm adding).
* It makes peter's life more difficult in bug 787070.

jimb recently did some refactoring of the proxy toString stuff. obj_toString is now gone, and has been replaced by a much simpler className() trap. The default obj_toString that we'll inherit off the Xrayed Object.prototype will invoke this, so I think we can leverage it to avoid defining any custom properties at all. I'll attach a patch.
(Assignee)

Comment 1

5 years ago
Hm, it looks like the className trap can't allocate and can only return static strings. That makes doing this a pretty huge PITA, so I'm inclined to try dropping support for the special ToString behavior and just use the underlying className.

The addons-mxr story looks much better than I thought. The only two addons that are checking for this (either via /XrayWrapper/ or .indexOf('XrayWrapper')) are Firebug and AsYouWish, and we know how to get in touch with them (I've CCed them in this bug).

I'm going to try dropping the [object XrayWrapper[...]] prefixing for WebIDL objects in preparation for bug 787070 and see if anything breaks.
(Assignee)

Updated

5 years ago
Summary: Clean up crazy custom XrayWrapper ToString → Drop support for custom [object XrayWrapper [object ClassName]] stringification

Comment 3

5 years ago
Sorry my refactoring in bug 862531 ended up getting in your way. At the time, I justified returning a static string by saying:

"For now, all our use cases would be returning constant strings, so a reasonable first implementation could specify that the callee owns the returned string. If we need dynamically-generated class names later, we can add the necessary complications."
(Assignee)

Comment 4

5 years ago
(In reply to Jim Blandy :jimb from comment #3)
> Sorry my refactoring in bug 862531 ended up getting in your way. At the
> time, I justified returning a static string by saying:
> 
> "For now, all our use cases would be returning constant strings, so a
> reasonable first implementation could specify that the callee owns the
> returned string. If we need dynamically-generated class names later, we can
> add the necessary complications."

I think that's reasonable. And if we can get away without the extra |object XrayWrapper| goop, I think we're probably better off in the long run.

Historically it's been useful for developers poking around in the web console and such to recognize that they're dealing with an XrayWrapper. But now that we have more advanced pretty printing, I think that (if we want that) we can just move that indicator logic to devtools and base it off of Cu.isXrayWrapper.

Comment 5

5 years ago
Can I ask whether this is expected to affect the SDK? I'm merely using wrap(), but on any conceivable SDK object...
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 6

5 years ago
(In reply to Brett Zamir from comment #5)
> Can I ask whether this is expected to affect the SDK? I'm merely using
> wrap(), but on any conceivable SDK object...

Addons MXR shows that at least one version of AsYouWish was bundling an old version of SpecialPowers, which included a /XrayWrapper/ test for XrayWrappers. The current version of SpecialPowers uses Cu.isXrayWrapper, which should work just fine.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 7

5 years ago
One orange test, which I've fixed. Uploading and flagging for review.
Attachment #8450682 - Flags: superreview?(mrbkap)
Attachment #8450682 - Flags: review?(peterv)
(In reply to Bobby Holley (:bholley) from comment #1)
> The addons-mxr story looks much better than I thought. The only two addons
> that are checking for this (either via /XrayWrapper/ or
> .indexOf('XrayWrapper')) are Firebug and AsYouWish, and we know how to get
> in touch with them (I've CCed them in this bug).
Thanks for the heads up.
(the change is just fine for Firebug)

Honza
Comment on attachment 8450682 [details] [diff] [review]
Drop support for custom [object XrayWrapper [object ClassName]] stringification. v1

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

r=me, I'd already done something like this in my patches for bug 787070.
Attachment #8450682 - Flags: review?(peterv) → review+
(In reply to Bobby Holley (:bholley) from comment #4)
> Historically it's been useful for developers poking around in the web
> console and such to recognize that they're dealing with an XrayWrapper. But
> now that we have more advanced pretty printing, I think that (if we want
> that) we can just move that indicator logic to devtools and base it off of
> Cu.isXrayWrapper.

What sort of more advanced pretty printing do we have now? If I'm developing in JS, how do I find out that the object I have is an Xray wrapper?
Comment on attachment 8450682 [details] [diff] [review]
Drop support for custom [object XrayWrapper [object ClassName]] stringification. v1

I guess I'll sr this given that our devtools should be better now.
Attachment #8450682 - Flags: superreview?(mrbkap) → superreview+
(Assignee)

Comment 13

5 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #4)
> > Historically it's been useful for developers poking around in the web
> > console and such to recognize that they're dealing with an XrayWrapper. But
> > now that we have more advanced pretty printing, I think that (if we want
> > that) we can just move that indicator logic to devtools and base it off of
> > Cu.isXrayWrapper.
> 
> What sort of more advanced pretty printing do we have now? If I'm developing
> in JS, how do I find out that the object I have is an Xray wrapper?

For code, Cu.isXrayWrapper. The pretty printing I'm referring to is from the Web Console and Browser Scratchpad, which no longer gives you the original .toString() anyway. My assumption is that that the toString() override was mostly useful for people poking around at things interactively, or using console.log().

I guess people might be doing alert(someObj). But it's not clear to me that this is an important enough feature to justify this behavior. In general, people shouldn't need to know that they have an XrayWrapper at all.
https://hg.mozilla.org/mozilla-central/rev/74c612bb0147
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.