Implement PutForwards in the new DOM bindings

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla19
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 654879 [details] [diff] [review]
v1
(Assignee)

Updated

5 years ago
Attachment #654879 - Flags: review?(bzbarsky)
Comment on attachment 654879 [details] [diff] [review]
v1

Should the parser enforce lack of forwarding cycles, PutForwards only being used on readonly attributes, and PutForwards not being used on [Replaceable] attributes?  Seems like it would be nice.  Also the bits about it not being used for static attributes or on callback interfaces?

Also, might be good to enforce in the parser the bit about how there must be an attribute with the right name on the target interface.  And perhaps the parser should ensure that the type of the PutForwards attribute is an interface type, so you don't have to do the v.isObject() check in your definition_body.

>+++ b/dom/bindings/Codegen.py
>+class CGSpecializedForwardingSetter(CGSpecializedSetter):
>+        forwardToType = attr.type
>+        self.forwardToDescriptor = descriptor.getDescriptor(forwardToType.unroll().inner.identifier.name)
>+        self.forwardedTo = self.forwardToDescriptor.name
>+        self.forwardedToNative = self.forwardToDescriptor.nativeType

I don't think any of those are needed.  Just remove them.

>+    def definition_body(self):
>+        return CGIndenter(CGGeneric("""JS::Value v;

I think this should be:

  JS::RootedValue v(cx);

and then pass v.address() to things that want a Value*.  I think just using a RootedValue as a Value (in terms of calling toObject()) should work; if not, v.get().toObject() should.

>+if (!get_%s(cx, obj, self, &v)) {

I'm not sure this matches spec.  The spec seems to require that this be a JS_GetProperty(cx, obj, "%s", v.address()), possibly with an assert somewhere here that self.attr.identifier.name is ASCII.  That said, I'm not convinced the spec is the right way to go here, so we could keep this impl and file a spec bug if desired.  Either way.

>+if (!v.isObject()) {
>+  return ThrowErrorMessage(cx, MSG_NOT_OBJECT);

I don't see why this would be needed if we ensure the type is an interface type in the parser.

>+return JS_SetProperty(cx, &v.toObject(), "%s", argv);""" % (self.attr.identifier.name, self.forwardToAttrName))).define()

We should be asserting that self.forwardToAttrName is ASCII.

>+            if not self.member.readonly or not self.member.getExtendedAttribute("PutForwards") is None:

I'd prefer the "X is not None" pattern to the "not X is None" pattern here, especially given the length of X.

r=me with the above fixed.
Attachment #654879 - Flags: review?(bzbarsky) → review+
I'm not sure we should go through JSAPI at all, tbh. In the case of

var el = document.createElement("div");
Object.defineProperty(el, "itemType", {
  get: function() {
    w("getter");
    return { set value(v) { w("setter") } };
  }
})
el.itemType = "foo";

I don't see the point in calling either getter or setter.
Well, I assumed Cameron did it that way on purpose.  But maybe not?
It was deliberate, just to be less magical (which internally knowing what the forwarded-to setter is would be).
(Assignee)

Comment 5

5 years ago
(In reply to :Ms2ger from comment #2)
> I don't see the point in calling either getter or setter.

Why would you want |el.itemType = "foo"| be different from |el.itemType.value = "foo"|?
(Assignee)

Comment 6

5 years ago
Created attachment 676146 [details] [diff] [review]
v1.1

> Should the parser enforce lack of forwarding cycles, PutForwards only being
> used on readonly attributes, and PutForwards not being used on [Replaceable]
> attributes?  Seems like it would be nice.  Also the bits about it not being
> used for static attributes or on callback interfaces?

Done.

> Also, might be good to enforce in the parser the bit about how there must be
> an attribute with the right name on the target interface.  And perhaps the
> parser should ensure that the type of the PutForwards attribute is an
> interface type, so you don't have to do the v.isObject() check in your
> definition_body.

Done.

> I don't think any of those are needed.  Just remove them.

Done.

> I think this should be:
> 
>   JS::RootedValue v(cx);
> 
> and then pass v.address() to things that want a Value*.  I think just using
> a RootedValue as a Value (in terms of calling toObject()) should work; if
> not, v.get().toObject() should.

Done.

> I'm not sure this matches spec.  The spec seems to require that this be a
> JS_GetProperty(cx, obj, "%s", v.address()), possibly with an assert
> somewhere here that self.attr.identifier.name is ASCII.  That said, I'm not
> convinced the spec is the right way to go here, so we could keep this impl
> and file a spec bug if desired.  Either way.

I went with what the spec says, added assert to check for ASCII attribute name.

> >+if (!v.isObject()) {
> >+  return ThrowErrorMessage(cx, MSG_NOT_OBJECT);
> 
> I don't see why this would be needed if we ensure the type is an interface
> type in the parser.

I left this in, since I've now switched to a JS_GetProperty (and the getter could be redefined).

> We should be asserting that self.forwardToAttrName is ASCII.

Done.

> I'd prefer the "X is not None" pattern to the "not X is None" pattern here,
> especially given the length of X.

Done.
Attachment #654879 - Attachment is obsolete: true
Attachment #676146 - Flags: review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/461978f9a844
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/461978f9a844
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.