Closed
Bug 975277
Opened 10 years ago
Closed 10 years ago
Miscellaneous dependencies for Date object Xrays
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(6 files, 2 obsolete files)
6.42 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
1.82 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.98 KB,
patch
|
efaust
:
review+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
8.26 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
6.14 KB,
patch
|
gkrizsanits
:
review+
|
Details | Diff | Splinter Review |
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•10 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 2•10 years ago
|
||
Attachment #8379492 -
Flags: review+
Assignee | ||
Comment 3•10 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•10 years ago
|
||
Attachment #8379494 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•10 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)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8379496 -
Flags: review?(luke)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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•10 years ago
|
||
Attachment #8379496 -
Attachment is obsolete: true
Attachment #8379496 -
Flags: review?(luke)
Attachment #8379890 -
Flags: review?(luke)
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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•10 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•10 years ago
|
||
This should do it.
Attachment #8379494 -
Attachment is obsolete: true
Attachment #8379929 -
Flags: review?(gkrizsanits)
Updated•10 years ago
|
Attachment #8379929 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8379495 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Thanks for the reviews everyone. All-platform try push: https://tbpl.mozilla.org/?tree=Try&rev=27d44fcc0ff9
Assignee | ||
Comment 15•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/89a7246d2bdd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/65192746bdee remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5db44a9eece2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd04a8b8ccc4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/02dffb9d2748 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/84904662e2d5
Assignee | ||
Comment 16•10 years ago
|
||
Followup bustage fix for the removal of shortid: https://hg.mozilla.org/integration/mozilla-inbound/rev/924690f9d81b
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/89a7246d2bdd https://hg.mozilla.org/mozilla-central/rev/65192746bdee https://hg.mozilla.org/mozilla-central/rev/5db44a9eece2 https://hg.mozilla.org/mozilla-central/rev/fd04a8b8ccc4 https://hg.mozilla.org/mozilla-central/rev/02dffb9d2748 https://hg.mozilla.org/mozilla-central/rev/84904662e2d5 https://hg.mozilla.org/mozilla-central/rev/924690f9d81b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•