Bug 179600 – More is<> and downcast<>, less static_cast<>

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

More is<> and downcast<>, less static_cast<>

[Darin Adler]

Created attachment 326729 [details]
Patch

[Chris Dumez]

Seems to break GTK build.

[Darin Adler]

Created attachment 326733 [details]
Patch

[Darin Adler]

New one should compile in GTK. Ready for review, I think.

[Darin Adler]

Created attachment 326738 [details]
Patch

[Chris Dumez]

GTK bubble still red.

[Chris Dumez]

Comment on attachment 326738 [details]
Patch

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

r=me with comment. Please make sure GTK builds before landing.

> Source/WebCore/html/HTMLFormElement.cpp:671
> +    if (!event || !is<Element>(event->target()))

Doesn’t this change behavior? If the target is a Text node that is a descendant of an HTMLFormControlElement, then we will return nullptr instead of the control ancestor. Or I am missing something?

> Source/WebCore/html/HTMLFormElement.cpp:673
> +    return lineageOfType<HTMLFormControlElement>(downcast<Element>(*event->target())).first();

I discussed this Antti and he agreed with should have a lineageOfType overload that takes in a Node.

[Build Bot]

Comment on attachment 326738 [details]
Patch

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

New failing tests:
fast/canvas/webgl/webgl2-runtime-flag.html
fast/canvas/webgl/getBufferSubData-webgl1.html

[Build Bot]

Created attachment 326739 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

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

[Darin Adler]

Comment on attachment 326738 [details]
Patch

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

>> Source/WebCore/html/HTMLFormElement.cpp:671
>> +    if (!event || !is<Element>(event->target()))
> 
> Doesn’t this change behavior? If the target is a Text node that is a descendant of an HTMLFormControlElement, then we will return nullptr instead of the control ancestor. Or I am missing something?

Mostly event targets can’t be text nodes. They target the element, not the text node.

But I can’t verify this easily, and so I will rewrite to start with the node instead. Easy to do that with the parentElement function for now, while waiting for lineageOfType.

[Darin Adler]

OK, so four things to resolve before landing:

1) Crashing WebGL tests due to a mistake in HTMLCanvasElement::createContextWebGL. I fixed that.

2) Compiling on GTK failing because I used nil instead of nullptr. I fixed that.

3) Node vs. Element in findSubmitElement. Fixing that now.

4) Crash in svg/animations/svgtransform-animation-discrete.html test; seems unlikely to be due to my changes. Will test and see what I can learn.

[Darin Adler]

Created attachment 326741 [details]
Patch

[Darin Adler]

Uploaded a patch that probably fixes 1-3, testing locally for 4 soon.

[Darin Adler]

Created attachment 326742 [details]
Patch

[Darin Adler]

Committed r224740: <https://trac.webkit.org/changeset/224740>

[Radar WebKit Bug Importer]

<rdar://problem/35561978>