790273 - Implement support for [LenientThis] in WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We'll need this for DOM nodes.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This should do the trick

This has a slight conflict with bug 742191 that I can deal with if that lands first.

Comment 2

[Peter Van der Beken [:peterv]]

Comment on attachment 660129 [details] [diff] [review]
This should do the trick

Review of attachment 660129 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +3475,5 @@
> +                "JS_SET_RVAL(cx, vp, JS::UndefinedValue());\n"
> +                "return true;")
> +        else:
> +            unwrapFailureCode = None
> +        name = "genericLenientGetter" if lenientThis else "genericGetter"

I'd just set name in the if/else above.

@@ +3527,5 @@
> +                "MOZ_ASSERT(!JS_IsExceptionPending(cx));\n"
> +                "return true;")
> +        else:
> +            unwrapFailureCode = None
> +        name = "genericLenientSetter" if lenientThis else "genericSetter"

Same here.

@@ +3528,5 @@
> +                "return true;")
> +        else:
> +            unwrapFailureCode = None
> +        name = "genericLenientSetter" if lenientThis else "genericSetter"
> +        CGAbstractBindingMethod.__init__(self, descriptor, name, args, unwrapFailureCode)

Long line? (You wrapped it above)

::: dom/bindings/parser/WebIDL.py
@@ +1997,5 @@
> +        elif name == "LenientThis":
> +            if self.isStatic():
> +                raise WebIDLError("[LenientThis] is only allowed on non-static "
> +                                  "attributes", [self.location])
> +            self.lenientThis = True

Do we need to check list to make sure no arguments where specified? (I guess we don't check anywhere else either :-/)

Comment 3

[Boris Zbarsky [:bzbarsky]]

That last comment above is about stuff like [LenientThis=foo], which is not allowed.  I'll add a check for it, and a test.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: With review comments addressed

I got a bit carried away with the changes to check for lack of arguments and have better error reportings for issues with extended attributes...  I'll attach an interdiff so you can just review that

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 660244 [details] [diff] [review]
With review comments addressed

Review of attachment 660244 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/parser/WebIDL.py
@@ +348,2 @@
>                      raise WebIDLError("[TreatNullAs] must take the identifier "
>                                        "EmptyString", [self.location])

Maybe make this |"'EmptyString', not '" + value + "'"| and drop the print?

@@ +661,5 @@
>              elif identifier == "NoInterfaceObject":
> +                if not attr.noArguments():
> +                    raise WebIDLError("[NoInterfaceObject] must take no arguments",
> +                                      [attr.location])
> +                                      

Trailing whitespace.

@@ +2513,5 @@
> +        self._tuple = tuple
> +
> +    def identifier(self):
> +        return self._tuple[0]
> +    

Trailing whitespace.

@@ +2518,5 @@
> +    def noArguments(self):
> +        return len(self._tuple) == 1
> +
> +    def hasValue(self):
> +        return len(self._tuple) == 2 and isinstance(self._tuple[1], str)

So i guess this means "has an identifier as the value", it seems like the named argument list would also be a value but this won't return it?

@@ +2529,5 @@
> +        return (len(self._tuple) == 2 and isinstance(self._tuple[1], list) or
> +                len(self._tuple) == 3)
> +
> +    def args(self):
> +        assert(self.hasArgs)

self.hasArgs()?

::: dom/bindings/parser/tests/test_extended_attributes.py
@@ +29,5 @@
> +
> +    results = parser.finish()
> +    harness.ok(results[0].members[0].hasLenientThis(),
> +               "Should have a lenient this")
> +    

Trailing whitespace.

@@ +59,5 @@
> +    harness.ok(results[0].members[0].signatures()[0][1][0].clamp,
> +               "Should be clamped")
> +    harness.ok(not results[0].members[1].signatures()[0][1][0].clamp,
> +               "Should not be clamped")
> +    

Trailing whitespace.

@@ +73,5 @@
> +    except:
> +        threw = True
> +
> +    harness.ok(threw, "[Clamp] must take no arguments")
> +    

Trailing whitespace.

@@ +103,5 @@
> +    except:
> +        threw = True
> +
> +    harness.ok(threw, "[EnforceRange] must take no arguments")
> +    

Trailing whitespace.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Maybe make this |"'EmptyString', not '" + value + "'"| and drop the print?

Fixed.  Good catch on the print.  That should not have been there.

> So i guess this means "has an identifier as the value"

It can be an identifier or a string (e.g. in the [Pref] case).

> it seems like the named argument list would also be a value but this won't return it?

That's correct.

> self.hasArgs()?

Yes.

I fixed the whitespace nits.

https://hg.mozilla.org/integration/mozilla-inbound/rev/83380c25f756

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83380c25f756