Closed
Bug 1081255
Opened 10 years ago
Closed 10 years ago
Better comments in jsproxy.h and proxy handler classes
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: jorendorff, Assigned: jorendorff)
Details
Attachments
(1 file, 1 obsolete file)
This patch does not change behavior at all. It rewrites the comments in jsproxy.h and changes the ordering of the BaseProxyHandler methods. It makes corresponding changes to the various proxy handler subclasses throughout the tree. The old distinction between "fundamental" and "derived" traps is completely dead AFAICT (outside ScriptedIndirectProxyHandler), so I threw that away and replaced "trap" with "method" in most places.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8503356 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8503356 [details] [diff] [review] Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior r?bz for the bits in XPConnect, in bholley's PTOness. All it does is reorder methods to match the base class, and tweak a few comments. ES6's MOP has been pretty stable for a while now; here's where it landed: * isExtensible, getPrototypeOf, setPrototypeOf, call, and construct are standard internal methods * getPropertyDescriptor, hasOwn, keys, and iterate are not The current draft specifies 14 standard internal methods. BaseProxyHandler has a method for each one of them. In the draft, all 14 methods are scriptable by scriptable proxies; we implement 12 (bug 888969 is adding the last 2).
Attachment #8503356 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8503415 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•10 years ago
|
Attachment #8503356 -
Attachment is obsolete: true
Attachment #8503356 -
Flags: review?(efaustbmo)
Attachment #8503356 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8503415 [details] [diff] [review] Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior new patch, now with more patchiness
Attachment #8503415 -
Flags: review?(bzbarsky)
Comment 5•10 years ago
|
||
Comment on attachment 8503415 [details] [diff] [review] Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior Review of attachment 8503415 [details] [diff] [review]: ----------------------------------------------------------------- Yes yes yes yes yes yes yes. Yes. Thank you. +\infty. r=me ::: js/src/jsproxy.h @@ +75,5 @@ > + * implementations scattered through the ICs and JITs. > + * > + * 2. Certain non-native objects have internal methods that are implemented as > + * magical js::ObjectOps hooks. We're trying to get rid of these. > + * nit, whitespace on blank comment line between 2 and 3 @@ +81,5 @@ > + * implemented in C++, as the virtual methods of a C++ object stored on the > + * proxy, known as its handler. > + * > + * This means that just about anything you do to a proxy will end up going > + * through a C++ virtual method call. Possibly several. There's no reason the Glad to see some notion of the cost of "just make it a proxy" is at least mentioned to would-be consumers. @@ +113,5 @@ > + * BaseProxyHandler. > + * > + * Gecko's security wrappers are examples of cross-compartment wrappers. > + * > + * ### Proxy prototype chains Thankyouthankyouthankyou. This desperately needed to be explained in comments. I should have written this block when I wrote the initial system.
Attachment #8503415 -
Flags: review?(efaustbmo) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8503415 [details] [diff] [review] Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior This is awesome. r=me.
Attachment #8503415 -
Flags: review?(bzbarsky) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8503415 [details] [diff] [review] Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior Review of attachment 8503415 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure this has enough r+.
Attachment #8503415 -
Flags: review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/075cf4911854
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/075cf4911854
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•