[webkit-reviews] review granted: [Bug 50666] Spatial Navigation: code clean up : [Attachment 76324] patch 2 - v2

[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 76324: patch 2 - v2
https://bugs.webkit.org/attachment.cgi?id=76324&action=review

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

> WebCore/ChangeLog:18
> +	   Summary:
> +	   - Removed static declaration of updateFocusCandidateIfNeeded() from
the top of FocusController.cpp.
> +	   It is unneeded;
> +	   - In FocusCandidate constructor, renamed 'n' to 'node', and added an
assert to it;
> +	   - In virtualRectForAreaElementAndDirection, I added an assert to
'node' as well;
> +	   - In SpatialNavigation.h:
> +	   1) I reordered the declaration of some methods in order to group
> +	      related ones;
> +	   2) removed isScrollableContainerNode() function declaration since it
is not used outside
> +	   SpatialNavigation.cpp;
> +	   3) and removed the declaration of isNodeDeepDescendantOfDocument()
since it does not exist anymore.

It is unusual to give such a detailed summary for a change.

If you feel that the name of the bug is insufficient to describe this change
then I suggest	you write a general description of the change and/or highlight
the major change in this patch. Then I would move your Summary remarks to after
the ':' to the right of the respective file/function in the list of files and
functions (below).

For an example of this, see <http://trac.webkit.org/changeset/73829>. Notice, a
description is omitted in the change log entry as the bug title is sufficiently
detailed.

> WebCore/page/SpatialNavigation.cpp:54
>  static IntRect rectToAbsoluteCoordinates(Frame* initialFrame, const IntRect&
rect);

Nit: Although outside of this patch, the name of the second argument "rect"
should be removed/renamed as it doesn't add any value to this function
prototype.

> WebCore/page/SpatialNavigation.cpp:56
> +static bool isScrollableContainerNode(const WebCore::Node*);

Do we need the "WebCore::" prefix here? Looking at the code, this declaration
is within the WebCore namespace.

> WebCore/page/SpatialNavigation.cpp:695
>  IntRect virtualRectForAreaElementAndDirection(FocusDirection direction,
HTMLAreaElement* area)

Nit: The name of this function disagrees with the ordering of its arguments. In
particular, its name implies that the argument order should be
HTMLAreaElement*, then FocusDirection.

> WebCore/page/SpatialNavigation.cpp:697
> +    ASSERT(area);

I suggest that we also ASSERT(area->imageElement()).

> WebCore/page/SpatialNavigation.h:143
> +Node* scrollableEnclosingBoxOrParentFrameForNodeInDirection(FocusDirection,
Node* node);

Please remove the name of the second argument "node" as it doesn't add any
value to this prototype declaration.