Bug 120391

Summary: The code to look for an ancestor form element is duplicated in three different places
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, darin, kling, koivisto, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Cleanup
none
Addressed comments none

Description Ryosuke Niwa 2013-08-27 21:30:05 PDT
HTMLElement.cpp, HTMLImageElement.cpp, and TreeBuilder.cpp
Comment 1 Ryosuke Niwa 2013-08-27 21:33:02 PDT
Created attachment 209844 [details]
Cleanup
Comment 2 Sam Weinig 2013-08-27 21:48:26 PDT
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?
Comment 3 Ryosuke Niwa 2013-08-27 21:55:30 PDT
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.
Comment 4 Build Bot 2013-08-27 22:15:08 PDT
Comment on attachment 209844 [details]
Cleanup

Attachment 209844 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1618172
Comment 5 Darin Adler 2013-08-27 22:23:28 PDT
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?
Comment 6 Ryosuke Niwa 2013-08-28 11:19:22 PDT
Created attachment 209909 [details]
Addressed comments
Comment 7 Ryosuke Niwa 2013-08-28 11:20:40 PDT
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.
Comment 8 WebKit Commit Bot 2013-08-28 18:34:16 PDT
Comment on attachment 209909 [details]
Addressed comments

Clearing flags on attachment: 209909

Committed r154801: <http://trac.webkit.org/changeset/154801>
Comment 9 WebKit Commit Bot 2013-08-28 18:34:19 PDT
All reviewed patches have been landed.  Closing bug.