Bug 155953

Created attachment 275043 [details] Patch

Created attachment 275045 [details] Disabling for encrypted media

Created attachment 275048 [details] Patch

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.

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 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.

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 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.

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 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.

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

Created attachment 275083 [details] Fixing DOMWindow.idl

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.

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

(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 on attachment 275114 [details] Adding test expectation to area-coords.html This doesn't change the generated objc or gobject bindings. r=me

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>

All reviewed patches have been landed. Closing bug.

(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.

> 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.

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.