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)

x86_64
Linux
defect
Not set
normal

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: nobody → jorendorff
Status: NEW → ASSIGNED
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)
Attachment #8503356 - Attachment is obsolete: true
Attachment #8503356 - Flags: review?(efaustbmo)
Attachment #8503356 - Flags: review?(bobbyholley)
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 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 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 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+
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.