[webkit-reviews] review granted: [Bug 50666] Spatial Navigation: code clean up : [Attachment 76498] patch 3 - v1

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 50666: Spatial Navigation: code clean up
https://bugs.webkit.org/show_bug.cgi?id=50666

Attachment 76498: patch 3 - v1
https://bugs.webkit.org/attachment.cgi?id=76498&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76498&action=review

> WebCore/ChangeLog:11
> +	   * page/SpatialNavigation.h: Added
FocusCandidate::isFrameOwnerElement and WebCore::toFrameOwnerElement helper
functions.
> +	   (WebCore::FocusCandidate::isFrameOwnerElement): Checks is the Node
pointer wrapped by FocusCandidate is a instead of HTMLFrameOwnerElement.

Nit: We tend to wrap change log lines at around 80 columns.

> WebCore/ChangeLog:13
> +	   (WebCore::toFrameOwnerElement): Static casts the FocusCandidate
given as parameter to HTMLFrameOwnerElement if appropriated.

Nit: This sentence does not read well. Maybe, write "Returns the
HTMLFrameOwnerElement associated with the FocusCandidate if appropriate"?

> WebCore/page/SpatialNavigation.cpp:704
> +HTMLFrameOwnerElement* toFrameOwnerElement(FocusCandidate& candidate)

The name of this function seems strange. In particular, the prefix "to" implies
that the FocusCandidate will be transformed into an HTMLFrameOwnerElement (like
toRenderTable casting a RenderObject* to a RenderTable*). However, this
function actually transforms the visibleNode of a FocusCandidate object from
type Node* to HTMLFrameOwnerElement*.

Maybe rename this function frameOwnerElement(FocusCandidate& candidate)?