Closed Bug 975277 Opened 8 years ago Closed 8 years ago

Miscellaneous dependencies for Date object Xrays


(Core :: XPConnect, defect)

Not set





(Reporter: bholley, Assigned: bholley)




(6 files, 2 obsolete files)

I have a handful of patches for bug 975042 that can be reviewed by various people and land separately. Splitting them out into a separate bug.
This patch was originally written by peterv, but I made some changes to it. I
think efaust can review.
Attachment #8379491 - Flags: review?(efaustbmo)
The current logic ends up invoking BaseProxyHandler::set in various cases that
will cause it to invoke handler->getPropertyDescriptor, which is verboten for
mHasPrototype proxies.
Attachment #8379493 - Flags: review?(efaustbmo)
Attachment #8379494 - Flags: review?(gkrizsanits)
The current setup is kinda wrong, and doesn't work with HasPrototype Xrays.
This change requires us to manually munge the holder, but that's probably ok
for now.
Attachment #8379495 - Flags: review?(gkrizsanits)
Comment on attachment 8379496 [details] [diff] [review]
Part 6 - Introduce a mechanism to identify instances of standard classes. v1

Review of attachment 8379496 [details] [diff] [review]:

Unless you have planned used cases that actually depend on a dynamic value of IdentifyKind (I couldn't see any in this patch), I think it'd be cleaner to just have three different JSAPIs, one for each IdentifyKind.  This code duplication looks minimal and it'll make it easier to read.
Comment on attachment 8379493 [details] [diff] [review]
Part 3 - Rewrite Proxy::set logic. v1

Review of attachment 8379493 [details] [diff] [review]:

This is much cleaner. r=me

::: js/src/jsproxy.cpp
@@ +2550,5 @@
>      AutoEnterPolicy policy(cx, handler, proxy, id, BaseProxyHandler::SET, true);
>      if (!policy.allowed())
>          return policy.returnValue();
> +
> +    // If we don't have a prototype, we can sink to the |set| trap.

Can we change the wording of "don't have a prototype"? This flag is more or less unrelated to the value returned by JSObject::getProto() on the proxy, as I understand it.
Attachment #8379493 - Flags: review?(efaustbmo) → review+
Attachment #8379496 - Attachment is obsolete: true
Attachment #8379496 - Flags: review?(luke)
Attachment #8379890 - Flags: review?(luke)
Comment on attachment 8379890 [details] [diff] [review]
Part 6 - Introduce a mechanism to identify instances of standard classes. v2

Great, thanks.  Can you inject "StandardClass" into each of those names (e.g., IdentifyStandardClassInstance)?
Attachment #8379890 - Flags: review?(luke) → review+
Comment on attachment 8379491 [details] [diff] [review]
Part 1 - Add some machinery to allow Traits to specify whether they want to use hasPrototype or not. v3 r=bholley

Review of attachment 8379491 [details] [diff] [review]:

r=me. I hadn't seen EnableIf before. That's amazingly cool.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +1876,5 @@
>      RootedObject expando(cx, Traits::singleton.getExpandoObject(cx, target, wrapper));
> +    // We want to keep the Xray's prototype distinct from that of content, but
> +    // only if there's been a set. If there's not an expando, or the expando
> +    // slot is |undefined|, hand back content's proto, appropriately wrapped.

"hand back content's proto" is not quite true in the case where hasPrototype is set.
Attachment #8379491 - Flags: review?(efaustbmo) → review+
Comment on attachment 8379494 [details] [diff] [review]
Part 4 - Clean up the XPCWN XrayHolder a bit. v1

Looks like this doesn't compile on MSVC.
Attachment #8379494 - Flags: review?(gkrizsanits)
This should do it.
Attachment #8379494 - Attachment is obsolete: true
Attachment #8379929 - Flags: review?(gkrizsanits)
Attachment #8379929 - Flags: review?(gkrizsanits) → review+
Attachment #8379495 - Flags: review?(gkrizsanits) → review+
Thanks for the reviews everyone.

All-platform try push:
Followup bustage fix for the removal of shortid:
You need to log in before you can comment on or make changes to this bug.