Bug 155953 - Binding generator should allow passing DOM object parameters as references
Summary: Binding generator should allow passing DOM object parameters as references
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 155886
  Show dependency treegraph
 
Reported: 2016-03-28 13:08 PDT by youenn fablet
Modified: 2016-03-30 08:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (57.48 KB, patch)
2016-03-28 13:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Disabling for encrypted media (59.32 KB, patch)
2016-03-28 13:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (59.90 KB, patch)
2016-03-28 13:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (309.32 KB, application/zip)
2016-03-28 14:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (323.23 KB, application/zip)
2016-03-28 14:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (214.75 KB, application/zip)
2016-03-28 14:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-yosemite (300.68 KB, application/zip)
2016-03-28 14:47 PDT, Build Bot
no flags Details
Fixing DOMWindow.idl (60.35 KB, patch)
2016-03-29 01:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding test expectation to area-coords.html (59.71 KB, patch)
2016-03-29 11:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-03-28 13:08:21 PDT
Currently, DOM objects parameters are always passed as pointers.
Not nullable parameters should be passed as references.
Comment 1 youenn fablet 2016-03-28 13:34:20 PDT
Created attachment 275043 [details]
Patch
Comment 2 youenn fablet 2016-03-28 13:54:45 PDT
Created attachment 275045 [details]
Disabling for encrypted media
Comment 3 youenn fablet 2016-03-28 13:58:34 PDT
Created attachment 275048 [details]
Patch
Comment 4 Build Bot 2016-03-28 14:35:11 PDT
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.
Comment 5 Build Bot 2016-03-28 14:35:14 PDT
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
Comment 6 Build Bot 2016-03-28 14:37:26 PDT
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.
Comment 7 Build Bot 2016-03-28 14:37:29 PDT
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
Comment 8 Build Bot 2016-03-28 14:40:36 PDT
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.
Comment 9 Build Bot 2016-03-28 14:40:39 PDT
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
Comment 10 Build Bot 2016-03-28 14:47:51 PDT
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.
Comment 11 Build Bot 2016-03-28 14:47:55 PDT
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
Comment 12 youenn fablet 2016-03-29 01:03:02 PDT
Created attachment 275083 [details]
Fixing DOMWindow.idl
Comment 13 Darin Adler 2016-03-29 09:22:49 PDT
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.
Comment 14 youenn fablet 2016-03-29 11:58:21 PDT
Created attachment 275114 [details]
Adding test expectation to area-coords.html
Comment 15 youenn fablet 2016-03-29 12:06:47 PDT
(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.
Comment 16 Alex Christensen 2016-03-29 15:57:09 PDT
Comment on attachment 275114 [details]
Adding test expectation to area-coords.html

This doesn't change the generated objc or gobject bindings. r=me
Comment 17 WebKit Commit Bot 2016-03-30 01:18:20 PDT
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>
Comment 18 WebKit Commit Bot 2016-03-30 01:18:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 David Kilzer (:ddkilzer) 2016-03-30 02:24:43 PDT
(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.
Comment 20 youenn fablet 2016-03-30 02:34:42 PDT
> 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.
Comment 21 Darin Adler 2016-03-30 08:39:08 PDT
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.