Bug 113739 – RenderObject::offsetParent should return Element*

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

RenderObject::offsetParent should return Element*

[Elliott Sprehn]

Created attachment 196062 [details]
Patch

[Early Warning System Bot]

Comment on attachment 196062 [details]
Patch

Attachment 196062 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17356414

[Early Warning System Bot]

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

[EFL EWS Bot]

Comment on attachment 196062 [details]
Patch

Attachment 196062 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17301500

[Elliott Sprehn]

Created attachment 196063 [details]
Patch

[Ojan Vafai]

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.

[WebKit Review Bot]

Comment on attachment 196063 [details]
Patch

Clearing flags on attachment: 196063

Committed r147395: <http://trac.webkit.org/changeset/147395>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Elliott Sprehn]

(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. :)

[Beth Dakin]

This patch caused a reproducible crash on http://www.hodinkee.com/blog/twixt-time-evaluate-the-accuracy-of-your-watch-right-on-your-iphone