HTMLElement.cpp, HTMLImageElement.cpp, and TreeBuilder.cpp
Created attachment 209844 [details] Cleanup
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 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 on attachment 209844 [details] Cleanup Attachment 209844 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1618172
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?
Created attachment 209909 [details] Addressed comments
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 on attachment 209909 [details] Addressed comments Clearing flags on attachment: 209909 Committed r154801: <http://trac.webkit.org/changeset/154801>
All reviewed patches have been landed. Closing bug.