Bug 120391 – The code to look for an ancestor form element is duplicated in three different places

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

HTMLElement.cpp, HTMLImageElement.cpp, and TreeBuilder.cpp

[Ryosuke Niwa]

Created attachment 209844 [details]
Cleanup

[Sam Weinig]

Comment on attachment 209844 [details]
Cleanup

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

> Source/WebCore/html/HTMLFormElement.cpp:686
> +HTMLFormElement* HTMLFormElement::findClosestFormAncestor(Element* element)

Seems like this could take a const Element* so you don't need the const_casts.

> Source/WebCore/html/HTMLFormElement.cpp:689
> +    if (!element)
> +        return 0;

A bunch of the callers never pass rull.  Can we yank out this null check and pass an element by reference instead?

[Ryosuke Niwa]

Comment on attachment 209844 [details]
Cleanup

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

>> Source/WebCore/html/HTMLFormElement.cpp:686
>> +HTMLFormElement* HTMLFormElement::findClosestFormAncestor(Element* element)
> 
> Seems like this could take a const Element* so you don't need the const_casts.

I guess but then it'll be weird for it return non-const HTMLFormElement*.

>> Source/WebCore/html/HTMLFormElement.cpp:689
>> +        return 0;
> 
> A bunch of the callers never pass rull.  Can we yank out this null check and pass an element by reference instead?

I can try that.

[Build Bot]

Comment on attachment 209844 [details]
Cleanup

Attachment 209844 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1618172

[Darin Adler]

Comment on attachment 209844 [details]
Cleanup

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

>>> Source/WebCore/html/HTMLFormElement.cpp:686
>>> +HTMLFormElement* HTMLFormElement::findClosestFormAncestor(Element* element)
>> 
>> Seems like this could take a const Element* so you don't need the const_casts.
> 
> I guess but then it'll be weird for it return non-const HTMLFormElement*.

I think you should make it take const; I don’t think it’s so strange to return a non-const form element pointer even in that case.

> Source/WebCore/html/HTMLFormElement.cpp:693
> +    while (1) {
> +        element = element->parentElement();
> +        if (!element)
> +            return 0;

I suggest either writing it like this:

    while ((element = element->parentElement())

Or as a for loop.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-141
> -    ASSERT(isMainThread());

We lost this assertion. Should we add it back?

[Ryosuke Niwa]

Created attachment 209909 [details]
Addressed comments

[Ryosuke Niwa]

Comment on attachment 209844 [details]
Cleanup

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

>> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:-141
>> -    ASSERT(isMainThread());
> 
> We lost this assertion. Should we add it back?

I think this was there for the threaded parsing work.  I don't think it we need it since this function is no longer in HTMLTreeBuilder.cpp and HTMLTreeBuilder::HTMLTreeBuilder asserts that we're in the main thread already.

[WebKit Commit Bot]

Comment on attachment 209909 [details]
Addressed comments

Clearing flags on attachment: 209909

Committed r154801: <http://trac.webkit.org/changeset/154801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.