Miscellaneous dependencies for Date object Xrays

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla30
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
This patch was originally written by peterv, but I made some changes to it. I
think efaust can review.
Attachment #8379491 - Flags: review?(efaustbmo)
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
Attachment #8379494 - Flags: review?(gkrizsanits)
(Assignee)

Comment 5

5 years ago
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+
(Assignee)

Comment 9

5 years ago
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+
(Assignee)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
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+
(Assignee)

Comment 14

5 years ago
Thanks for the reviews everyone.

All-platform try push:
https://tbpl.mozilla.org/?tree=Try&rev=27d44fcc0ff9
(Assignee)

Comment 16

5 years ago
Followup bustage fix for the removal of shortid:

https://hg.mozilla.org/integration/mozilla-inbound/rev/924690f9d81b
You need to log in before you can comment on or make changes to this bug.