WebIDL bindings: add support for dictionary return values

RESOLVED FIXED in mozilla19

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bjacob, Assigned: bz)

Tracking

({dev-doc-complete})

unspecified
mozilla19
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

It would be nice to be able to return a dictionary.

For example, in WebGL, getContextAttributes() is supposed to return a dictionary.
Created attachment 668550 [details] [diff] [review]
part 1.  Add support for dictionary return values.
Attachment #668550 - Flags: review?(peterv)
Created attachment 668551 [details] [diff] [review]
part 2.  Switch WebGLContextAttributes to being a dictionary.
Attachment #668551 - Flags: review?(peterv)
Attachment #668551 - Flags: review?(bjacob)
Whiteboard: [need review]
(Reporter)

Comment 4

6 years ago
Comment on attachment 668551 [details] [diff] [review]
part 2.  Switch WebGLContextAttributes to being a dictionary.

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

::: content/canvas/src/WebGLContext.cpp
@@ +876,5 @@
>      if (!IsContextStable())
>      {
> +        // XXXbz spec says we should still return our attributes in
> +        // this case!  Should we store them all in mOptions?
> +        return;

Yes, probably; in my local patch implementing attributes as an interface, I was refactoring WebGLContextOptions into WebGLContextAttributes.
Attachment #668551 - Flags: review?(bjacob) → review+
Comment on attachment 668550 [details] [diff] [review]
part 1.  Add support for dictionary return values.

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

::: dom/bindings/Codegen.py
@@ +5405,5 @@
>                            m in d.members) + "\n"
> +                "};\n"
> +                "struct ${selfName}Initializer : public ${selfName} {\n"
> +                "  ${selfName}Initializer() {\n"
> +                "    // Safe to pass a null context if we pass a null value\n"

It is not safe right now, because we try to init the JS ids, but we don't actually need them. I'd make Init deal with a null JSContext by skipping the id initing, but only after asserting that the value is JS::NullValue().
Attachment #668550 - Flags: review?(peterv) → review+
Attachment #668551 - Flags: review?(peterv) → review+
Man, I suck.  I was pretty sure I'd run the webgl tests on this, but I had not.  The code that returns dictionaries never returns the new objects, causing test fails.

Patch coming up to fix that.
Created attachment 673941 [details] [diff] [review]
followup.  Actually return our newly-created object from dictionary ToObject.
Attachment #673941 - Flags: review?(peterv)
Comment on attachment 673941 [details] [diff] [review]
followup.  Actually return our newly-created object from dictionary ToObject.

As you mentioned, might be slightly better to do this in the "no parent" path.
Attachment #673941 - Flags: review?(peterv) → review+
Documented that we have this now.
Keywords: dev-doc-complete
You need to log in before you can comment on or make changes to this bug.