Binding generator should allow passing DOM object parameters as references

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[youenn fablet]

Currently, DOM objects parameters are always passed as pointers.
Not nullable parameters should be passed as references.

[youenn fablet]

Created attachment 275043 [details]
Patch

[youenn fablet]

Created attachment 275045 [details]
Disabling for encrypted media

[youenn fablet]

Created attachment 275048 [details]
Patch

[Build Bot]

Comment on attachment 275048 [details]
Patch

Attachment 275048 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1057962

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275054 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275048 [details]
Patch

Attachment 275048 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1057965

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275055 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275048 [details]
Patch

Attachment 275048 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1057963

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275056 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275048 [details]
Patch

Attachment 275048 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1057957

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 275057 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 275083 [details]
Fixing DOMWindow.idl

[Darin Adler]

Comment on attachment 275083 [details]
Fixing DOMWindow.idl

View in context: https://bugs.webkit.org/attachment.cgi?id=275083&action=review

I’m OK with this. A step in the right direction I think. I did review- only because I am uncertain about some of the DOMWindow.idl changes and what they might mean about the bindings generator.

I think that [Default=Undefined] might effective need to mean nullable and so either we should not have a new FIXME for these functions, or we should not have to explicitly add nullable. We should get this straight before beginning on this path.

Side note: Seems a bit of a shame to have to annotate the IDL for this. With some template meta-programming we could probably make the bindings adapt automatically but I suppose this is a more realistic and practical option.

How were you able to do this successfully without dealing with gobject and objc bindings?

I think the attribute should be named differently, perhaps something like UsePointersEvenForNonNullableObjectArguments? But we should probably just remove the keyword fairly quickly by applying nullable in a lot more places and then removing the nullable annotation more slowly.

> Source/WebCore/page/DOMWindow.idl:143
> +    // FIXME: Make element parameter not nullable.
> +    CSSStyleDeclaration getComputedStyle([Default=Undefined] optional Element? element, [Default=Undefined] optional DOMString? pseudoElement);

I don’t have a full understanding of this FIXME and of what’s right here. If the default for element is "undefined" then we have to have a way to pass that to the underlying C++ function, presumably with a null pointer. Given that, what does nullable even mean? Is it only about the behavior of a JavaScript null?

> Source/WebCore/page/DOMWindow.idl:148
> +    // FIXME: Check whether these parameters can be nullable.
> +    CSSRuleList getMatchedCSSRules([Default=Undefined] optional Element? element, [Default=Undefined] optional DOMString? pseudoElement);

Ditto.

> Source/WebCore/page/DOMWindow.idl:157
> +    // FIXME: Check whether these parameters can be nullable.
> +    WebKitPoint webkitConvertPointFromPageToNode([Default=Undefined] optional Node? node,
> +                                                 [Default=Undefined] optional WebKitPoint? p);
> +    WebKitPoint webkitConvertPointFromNodeToPage([Default=Undefined] optional Node? node,
> +                                                 [Default=Undefined] optional WebKitPoint? p);

Ditto.

[youenn fablet]

Created attachment 275114 [details]
Adding test expectation to area-coords.html

[youenn fablet]

(In reply to comment #13)
> Comment on attachment 275083 [details]
> Fixing DOMWindow.idl
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275083&action=review
> 
> I’m OK with this. A step in the right direction I think. I did review- only
> because I am uncertain about some of the DOMWindow.idl changes and what they
> might mean about the bindings generator.

I fixed that in the binding generator.

> I think that [Default=Undefined] might effective need to mean nullable and
> so either we should not have a new FIXME for these functions, or we should
> not have to explicitly add nullable. We should get this straight before
> beginning on this path.

The FIXME were there to not forget to fix that case.
I updated the binding generator in the current patch to pass parameter as pointer in the case of optional=undefined case.

> Side note: Seems a bit of a shame to have to annotate the IDL for this. With
> some template meta-programming we could probably make the bindings adapt
> automatically but I suppose this is a more realistic and practical option.

I thought of that but prefer the current approach.
New APIs will have no choice but to pass parameters by references which seems like an improvement.
Removing the UsePointersEvenForNonNullableObjectArguments parameter gives some incentive to fix the current APIs as well.

> How were you able to do this successfully without dealing with gobject and
> objc bindings?

I did not check gobject/objc bindings.
Maybe some issues will arise when removing UsePointersEvenForNonNullableObjectArguments.

> I think the attribute should be named differently, perhaps something like
> UsePointersEvenForNonNullableObjectArguments? But we should probably just
> remove the keyword fairly quickly by applying nullable in a lot more places
> and then removing the nullable annotation more slowly.

I changed the new parameter name.
Removing its use as quickly as possible is part of the plan.

> > Source/WebCore/page/DOMWindow.idl:143
> > +    // FIXME: Make element parameter not nullable.
> > +    CSSStyleDeclaration getComputedStyle([Default=Undefined] optional Element? element, [Default=Undefined] optional DOMString? pseudoElement);
> 
> I don’t have a full understanding of this FIXME and of what’s right here. If
> the default for element is "undefined" then we have to have a way to pass
> that to the underlying C++ function, presumably with a null pointer. Given
> that, what does nullable even mean? Is it only about the behavior of a
> JavaScript null?

The idea was to remove the nullable parameter change.
I fixed that in the new patch at the binding generator level.

[Alex Christensen]

Comment on attachment 275114 [details]
Adding test expectation to area-coords.html

This doesn't change the generated objc or gobject bindings. r=me

[WebKit Commit Bot]

Comment on attachment 275114 [details]
Adding test expectation to area-coords.html

Clearing flags on attachment: 275114

Committed r198833: <http://trac.webkit.org/changeset/198833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> Comment on attachment 275114 [details]
> Adding test expectation to area-coords.html
> 
> Clearing flags on attachment: 275114
> 
> Committed r198833: <http://trac.webkit.org/changeset/198833>

What is the rule for when to add "UsePointersEvenForNonNullableObjectArguments" to an IDL file?

Is it a matter of trial-and-error to see if the generated code compiles without it, or is there an easy rule that can be used (such as looking at the list of constructor arguments) to know when this needs to be added?

I ask because this change broke some internal IDL files, and I don't want to roll this change back since it seems like a step forward.

[youenn fablet]

> What is the rule for when to add
> "UsePointersEvenForNonNullableObjectArguments" to an IDL file?

UsePointersEvenForNonNullableObjectArguments is just a temporary keyword to update progressively all DOM classes to take references and not pointers.

In the IDL files I saw, sometimes the IDL file should be updated to accept nullable parameters, sometimes the DOM class needs to be updated to take references. That is why it seems good to do the migration progressively.

> Is it a matter of trial-and-error to see if the generated code compiles
> without it, or is there an easy rule that can be used (such as looking at
> the list of constructor arguments) to know when this needs to be added?

It might be easier to do trial-and-error.
Otherwise, one needs to look at the constructor and each method to check whether there is a non nullable DOM class parameter.

> I ask because this change broke some internal IDL files, and I don't want to
> roll this change back since it seems like a step forward.

I see.
Depending on how much this change is breaking internal IDL files, it might be easier to add UsePointersEvenForNonNullableObjectArguments to these broken IDL files, and later on update the DOM class to take reference parameters.

If there are very few broken IDL files, you might want to update the DOM classes directly.

[Darin Adler]

Rule of thumb: UsePointersEvenForNonNullableObjectArguments preserves the old behavior, so add it to any IDL file where there’s a problem. Then remove it later as we specifically pay attention to that file. We’ll remove support for UsePointersEvenForNonNullableObjectArguments entirely once no file needs to use it any more.