[webkit-reviews] review denied: [Bug 49382] Spatial Navigation: issues with the node selection algorithm. : [Attachment 73954] Patch - code only

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 49382: Spatial Navigation: issues with the node selection algorithm.
https://bugs.webkit.org/show_bug.cgi?id=49382

Attachment 73954: Patch - code only
https://bugs.webkit.org/attachment.cgi?id=73954&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=73954&action=review

Great! ... but we are still missing some stuff. One more batch needed.

> WebCore/page/FocusController.cpp:783
> +	       rect = nodeRectInAbsoluteCoordinates(focusedNode, true);

Add a comment here explaining the bool.

rect = nodeRectInAbsoluteCoordinates(focusedNode, true /* ignore border */);

> WebCore/page/FocusController.cpp:817
> +bool FocusController::navigateIn4WayDirection(FocusDirection direction,
KeyboardEvent* event)

I would really like a better name here. Why not keep the previous name?

> WebCore/page/FocusController.cpp:833
> +	   container = scrollableOrFrameParentForNodeAndDirection(direction,
focusedNode);
> +	   startingRect = nodeRectInAbsoluteCoordinates(focusedNode, true);

On IRC we talked about renamed FocusableCandidate to FocusableContent. It would
have an additional IntRect, and Node could be 0. The 'container' above would be
the FocusableContent::scrollableEnclosingNode. Does it make sense to implement,
so it all gets consistent?

ps: Add a comment explaining the bool here too.

> WebCore/page/FocusController.cpp:836
> +    bool consumed = false;

what is consumed?

> WebCore/page/FocusController.cpp:841
> +	   startingRect = nodeRectInAbsoluteCoordinates(container, true);

Comment for the bool.

> WebCore/page/FocusController.cpp:844
> +    } while (!consumed && container);
> +    return consumed;

Add a line between these two.

> WebCore/page/SpatialNavigation.cpp:64
> +    , rect(nodeRectInAbsoluteCoordinates(n, true))

Please add a "/* */" comment explaining the bool.

> WebCore/page/SpatialNavigation.cpp:301
> +{

Could you consider making use of isRectInDirection() all over this method,
instead of "curRect.x() < targetRect.right()" , "targetRect.x() <
curRect.right()"...

It made practical results better for me, back on time.

> WebCore/page/SpatialNavigation.cpp:494
> +    default:
> +	   break;
> +    }

Add an assert_not_reached here, and return :)

> WebCore/page/SpatialNavigation.cpp:528
> +	       ASSERT_NOT_REACHED();

add a 'return' here as well.

> WebCore/page/SpatialNavigation.cpp:564
> +	       ASSERT_NOT_REACHED();

'return' here too.

> WebCore/page/SpatialNavigation.cpp:569
> +
> +	   return true;

Remove this empty line.

> WebCore/page/SpatialNavigation.cpp:667
> +	   if (parent->isDocumentNode()) // This is true only if node is a
document node.

I would remove this comment.

> WebCore/page/SpatialNavigation.cpp:672
> +	       parent = parent->parentNode();
> +    } while (parent && !canScrollInDirection(direction, parent) &&
!parent->isDocumentNode());
> +    return parent;

Empty line between this two.

> WebCore/page/SpatialNavigation.cpp:705
> +    frame->view()->calculateScrollbarModesForLayout(horizontalMode,
verticalMode);

Maybe FrameView::calculateScrollbarModesForLayout does not consider the case
when Webkit side disabled the scrollbars? See ScrollView::scrollbarModes()...

> WebCore/page/SpatialNavigation.cpp:788
> +	   ASSERT_NOT_REACHED();

'return' here too.

> WebCore/page/SpatialNavigation.cpp:848
> +    switch (direction) {
> +    case FocusDirectionLeft:
> +	   if (nodeRect.right() > currentRect.x())
> +	       return;
> +	   break;
> +    case FocusDirectionUp:
> +	   if (nodeRect.bottom() > currentRect.y())
> +	       return;
> +	   break;
> +    case FocusDirectionRight:
> +	   if (nodeRect.x() < currentRect.right())
> +	       return;
> +	   break;
> +    case FocusDirectionDown:
> +	   if (nodeRect.y() < currentRect.bottom())
> +	       return;

are not you doing these checks in areRectsMoreThanFullScreenApart?

> WebCore/page/SpatialNavigation.cpp:851
> +	   ASSERT_NOT_REACHED();

return here.

> WebCore/page/SpatialNavigation.cpp:857
> +    int sameAxisDist = 0;
> +    int otherAxisDist = 0;

Lets spell out 'Dist'.

> WebCore/page/SpatialNavigation.cpp:898
> +    IntRect candidateRect = nodeRectInAbsoluteCoordinates(candidate.node);

do not need to do this. Just use candidate.rect ?