Better comments in jsproxy.h and proxy handler classes

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 8503356 [details] [diff] [review]
Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior
Attachment #8503356 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 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

4 years ago
Created attachment 8503415 [details] [diff] [review]
Rewrite comments in jsproxy.h; reclassify the methods a bit. No change in behavior
Attachment #8503415 - Flags: review?(efaustbmo)
(Assignee)

Updated

4 years ago
Attachment #8503356 - Attachment is obsolete: true
Attachment #8503356 - Flags: review?(efaustbmo)
Attachment #8503356 - Flags: review?(bobbyholley)
(Assignee)

Comment 4

4 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

4 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 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

4 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+
https://hg.mozilla.org/mozilla-central/rev/075cf4911854
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.