[webkit-reviews] review denied: [Bug 49382] Spatial Navigation: issues with the node selection algorithm. : [Attachment 73993] 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 73993: Patch - code only
https://bugs.webkit.org/attachment.cgi?id=73993&action=review

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

> WebCore/page/FocusController.cpp:638
> +	   HitTestResult result =
candidate.node->document()->page()->mainFrame()->eventHandler()->hitTestResultA
tPoint(IntPoint(x, y), false, true);

page()->eventHandler()

> WebCore/page/FocusController.cpp:700
> +    // The starting rect is the rect of the focused node, in document
coordinates.
> +    // Compose a virtual starting rect if there is no focused node or if it
is off screen.
> +    // The virtual rect is the edge of the container or frame. We select
which
> +    // edge depending on the direction of the navigation.
> +    IntRect newStartingRect = startingRect;
> +
> +    if (startingRect.isEmpty()) {
> +	   newStartingRect = nodeRectInAbsoluteCoordinates(container);
> +	   switch (direction) {
> +	   case FocusDirectionLeft:
> +	       newStartingRect.setX(newStartingRect.right());
> +	       newStartingRect.setWidth(0);
> +	       break;
> +	   case FocusDirectionUp:
> +	       newStartingRect.setY(newStartingRect.bottom());
> +	       newStartingRect.setHeight(0);
> +	       break;
> +	   case FocusDirectionRight:
> +	       newStartingRect.setWidth(0);
> +	       break;
> +	   case FocusDirectionDown:
> +	       newStartingRect.setHeight(0);
> +	       break;
> +	   default:
> +	       ASSERT_NOT_REACHED();
> +	   }
> +    }

Make it a helper.

> WebCore/page/FocusController.cpp:709
> +    if (focusCandidate.isNull()) {
> +	   if (canScrollInDirection(direction, container)) {
> +	       // Nothing to focus, scroll if possible.
> +	       scrollInDirection(container, direction);

Lets change the logic here as following:

if (focusCandidate.isNull()) {

  // Nothing to focus, scroll if possible.
  if (scrollInDirection)
  ...
}

>From within scrollInDirection, you check canScrollInDirection, bailing out
earlier if it can not. Adjust the following 'return' calls accordingly.

> WebCore/page/FocusController.cpp:719
> +	   if (hasOffscreenRect(focusCandidate.node, direction)) {
> +	       Frame* frame = focusCandidate.node->document()->view()->frame();

> +	       scrollInDirection(frame->document(), direction);
> +	       return true;

You have two scrollInDirection methods. Here you should just use the one that
takes Frame*

Frame* frame = focusCandidate.node->document()->view()->frame();
scrollInDirection(frame, direction);
return true;

or make it:

Document* document = focusCandidate.node->document();
scrollInDirection(document, direction);
return true;

> WebCore/page/FocusController.cpp:725
> +	   IntRect rect;
> +	   Node* focusedNode = focusedOrMainFrame()->document()->focusedNode();

> +	   if (focusedNode && !hasOffscreenRect(focusedNode))
> +	       rect = nodeRectInAbsoluteCoordinates(focusedNode, true /* ignore
border */);

what happens here if you have no focused node, so 'rect' is empty. Also 'rect'
is a too generic name in this context. Lets give it a more descriptive name.

> WebCore/page/FocusController.cpp:726
> +	  
ASSERT(static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame()
);

Move this assert to outside the outer if here. If FrameOwnerElement is not a
contentFrame it just be no-op.

it should be 

if (focusCandidate.node->isFrameOwnerElement() &&
static_cast<HTMLFrameOwnerElement*>(focusableCandidate.node)->contentFrame())

> WebCore/page/FocusController.cpp:727
> +	  
static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)->contentFrame()->docum
ent()->updateLayoutIgnorePendingStylesheets();

Why not just do 

focusCandidate.node->document()?

> WebCore/page/FocusController.cpp:728
> +	   if
(!navigateInContainer(static_cast<HTMLFrameOwnerElement*>(focusCandidate.node)-
>contentFrame()->document(), rect, direction, event))

Same here.

Also, let same "Document* focusedDocument = focusCandidate.node->document()" in
a auxiliar variable to avoid make this cast so many times.

> WebCore/page/FocusController.cpp:729
> +	       // The new frame had nothing interesting, skip past it and try
again.

"skip past it" sounds strange.

Also put {} around the 'if' here.

> WebCore/page/FocusController.cpp:747
> +	   scrollInDirection(container, direction);

you are ignoring the return value here. Should you?

> WebCore/page/FrameView.h:238
> +    void calculateScrollbarModesForLayout(ScrollbarMode& hMode,
ScrollbarMode& vMode);

Your changelog should explain this change. Is this method designed to be
public?

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

It is not clear to me why you do not assert, and bail out. Could you explain?

> WebCore/page/SpatialNavigation.cpp:507
> +    ASSERT(frame && canScrollInDirection(direction, frame->document()));

you should be assert "canScrollInDirection" here. it should actually early
return here.

> WebCore/page/SpatialNavigation.cpp:560
> +	   switch (direction) {
> +	   case FocusDirectionLeft:
> +	       dx = - min(Scrollbar::pixelsPerLineStep(),
container->renderBox()->scrollLeft());
> +	       break;
> +	   case FocusDirectionRight:
> +	       ASSERT(container->renderBox()->scrollWidth() >
(container->renderBox()->scrollLeft() +
container->renderBox()->clientWidth()));
> +	       dx = min(Scrollbar::pixelsPerLineStep(),
container->renderBox()->scrollWidth() - (container->renderBox()->scrollLeft() +
container->renderBox()->clientWidth()));
> +	       break;
> +	   case FocusDirectionUp:
> +	       dy = - min(Scrollbar::pixelsPerLineStep(),
container->renderBox()->scrollTop());
> +	       break;
> +	   case FocusDirectionDown:
> +	       ASSERT(container->renderBox()->scrollHeight() -
(container->renderBox()->scrollTop() +
container->renderBox()->clientHeight()));
> +	       dy = min(Scrollbar::pixelsPerLineStep(),
container->renderBox()->scrollHeight() - (container->renderBox()->scrollTop() +
container->renderBox()->clientHeight()));

why do we need the minimal between the scrollable offset available and
Scrollbar::pixelPerLineStep ?

Just do ScrollGranularity line, and the rest will be handled for you.

> WebCore/page/SpatialNavigation.cpp:567
> +	   container->renderBox()->enclosingLayer()->scrollByRecursively(dx,
dy);

Why not do page()->eventHandler()->scrollRecursively( ... , ..., node), where
'node' is the 'startingNode'. It does that enclosing layer does and more.

> WebCore/page/SpatialNavigation.cpp:628
> +bool isScrollableContainerNode(const Node* node)

why does it need to be const here and not in the other method we are passing a
Node*?

> WebCore/page/SpatialNavigation.cpp:767
> +// Exit point is the closest point in the startingRect, depending on the
direction of the navigation.
> +// Entry point is the closest point in the candidate node, depending on the
direction of the navigation.

Lets rephrase this comment.

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

Lets name it either scrollableEnclosingBoxOrParentFrameForNodeInDirection or
scrollableOrParentFrameForNodeInDirection