RenderObject::offsetParent should return Element*
Created attachment 196062 [details] Patch
Comment on attachment 196062 [details] Patch Attachment 196062 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17356414
Comment on attachment 196062 [details] Patch Attachment 196062 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17311944
Comment on attachment 196062 [details] Patch Attachment 196062 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17301500
Created attachment 196063 [details] Patch
Comment on attachment 196062 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196062&action=review > Source/WebCore/rendering/RenderObject.cpp:-3018 > - // FIXME: Stop the search at the flow thread boundary until we figure out the right > - // behavior for elements inside a flow thread. I assume you meant to change the behavior for flow threads? You should say so in the ChangeLog. Why change the flow thread behavior? It's not clear to me what the preferred behavior is, but seems strange to change it as part of this patch which is really about tightening the types. > Source/WebCore/rendering/RenderObject.h:669 > + Element* offsetParent() const; It's strange to me to have parent() return a RenderObject and offsetParent() return an Element. Should this maybe be called offsetParentElement? I guess that's kind of dumb since the signature clearly makes it an Element.
Comment on attachment 196063 [details] Patch Clearing flags on attachment: 196063 Committed r147395: <http://trac.webkit.org/changeset/147395>
All reviewed patches have been landed. Closing bug.
(In reply to comment #6) > (From update of attachment 196062 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=196062&action=review > Sorry this got submitted before responding to your comments, that was my bad. > > Source/WebCore/rendering/RenderObject.cpp:-3018 > > - // FIXME: Stop the search at the flow thread boundary until we figure out the right > > - // behavior for elements inside a flow thread. > > I assume you meant to change the behavior for flow threads? You should say so in the ChangeLog. > > Why change the flow thread behavior? It's not clear to me what the preferred behavior is, but seems strange to change it as part of this patch which is really about tightening the types. The behavior doesn't actually change, I just removed the special casing for flow threads and made it generically handle any case that would have resulted in a renderer for a non Element being returned. RenderNamedFlowThread's are always children of the RenderView so "stopping" doesn't matter anyway. The flow thread has Document* as the node() and so does the RenderView so both result in returning null. > > > Source/WebCore/rendering/RenderObject.h:669 > > + Element* offsetParent() const; > > It's strange to me to have parent() return a RenderObject and offsetParent() return an Element. Should this maybe be called offsetParentElement? I guess that's kind of dumb since the signature clearly makes it an Element. Yeah, but offsetParent is a sort of nonsensical DOM API name. It's nice to keep the same names as the DOM API this is implementing. I can change it in a follow up patch if you have a strong opinion though. :)
This patch caused a reproducible crash on http://www.hodinkee.com/blog/twixt-time-evaluate-the-accuracy-of-your-watch-right-on-your-iphone