[webkit-reviews] review granted: [Bug 50666] Spatial Navigation: code clean up : [Attachment 77087] patch 6 - 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 77087: patch 6 - v1
https://bugs.webkit.org/attachment.cgi?id=77087&action=review

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

r=me.

> WebCore/ChangeLog:13
> +	   (WebCore::updatFocusCandidateIfNeeded): Assert if !renderer() or
!isElementNode()
> +	   since we are bailing bailing our earlir in FocusCandidate
constructor and
> +	   FocusController::findFocusCandidateInContainer respectively;

I believe you meant to write:

Assert renderer() and isElementNode() now that we are bailing out earlier in
both the FocusCandidate constructor and
FocusController::findFocusCandidateInContainer().

> WebCore/ChangeLog:17
> +	   i(WebCore::FocusController::findFocusCandidateInContainer):
> +	   (WebCore::FocusController::advanceFocusDirectionallyInContainer):
Adjusted callsites

Remove the leading 'i' before the "(WebCore::FocusController..." on line 16.

callsites => call sites

> WebCore/ChangeLog:19
> +	   (WebCore::FocusController::advanceFocusDirectionally): Adjusted
callsite of

callsite => call site

> WebCore/ChangeLog:23
> +	   (WebCore::FocusCandidate::FocusCandidate): Assert if node is not a
element node;
> +	   (WebCore::isScrollableNode): Renamed from isScrollableContainerNode;


element => Element (as per "The DOM Structure Model"
<http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1590626202>).

; => . (on both line 22 and 23)

> WebCore/ChangeLog:24
> +	   (WebCore::scrollInDirection): Adjusted callsite after function name
change;

callsite => call site

; => .

> WebCore/ChangeLog:26
> +	   a documentNode;

documentNode => "Document node" (as per "The DOM Structure Model"
<http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1590626202>).

; => .

> WebCore/page/FocusController.cpp:475
> +    for (; node; node = (node->isFrameOwnerElement() ||
canScrollInDirection(node, direction)) ? node->traverseNextSibling(container) :
node->traverseNextNode(container)) {

I still do not like this for-loop since it's long to follow and its step
function changes depending on the truth value of "node->isFrameOwnerElement()
|| canScrollInDirection(node, direction)".

Maybe use a while-loop that has the form:

Node* next = container->firstChild();
while (next) {
   ...
   next = next->isFrameOwnerElement() || canScrollInDirection(next, direction)
? next->traverseNextSibling(container) : next->traverseNextNode(container);
}

> WebCore/page/FocusController.cpp:585
> +	       startingRect = virtualRectForAreaElementAndDirection(area,
direction);

Yay! The name of the function now matches the argument order.

> WebCore/page/SpatialNavigation.cpp:431
>	   return (renderer->isBox() &&
toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()
> -		&& node->hasChildNodes() && !node->isDocumentNode());
> +		&& node->hasChildNodes());

I would remove the outer parentheses and write this return on one line now that
we have less conjuncts.