[Webkit-unassigned] [Bug 49382] Spatial Navigation: issues with the node selection algorithm.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=49382





--- Comment #35 from Daniel Bates <dbates at webkit.org>  2010-11-21 17:06:22 PST ---
(From update of attachment 74412)
View in context: https://bugs.webkit.org/attachment.cgi?id=74412&action=review

> WebCore/page/FocusController.cpp:606
> +void updateFocusCandidateIfNeeded(FocusDirection direction, const IntRect& startingRect, FocusCandidate& candidate, FocusCandidate& closest)

As far as I can tell, this function does not modify candidate. Can we make this const?

> WebCore/page/FocusController.cpp:609
> +    if (!candidate.node->isElementNode() ||!candidate.node->renderer())
> +        return;

Nit: There should be a space between '||' and "!candidate.node->renderer()". The style bot didn't seem to catch this, we should consider updating our style-checker code.

> WebCore/page/FocusController.cpp:668
> +    for (Node* node = container->firstChild(); node; node = (node->isFrameOwnerElement() || canScrollInDirection(direction, node)) ? node->traverseNextSibling(container) : node->traverseNextNode(container)) {
> +        if ((!focusedFrame() || !focusedFrame()->document() || node != focusedFrame()->document()->focusedNode()) && (node->isKeyboardFocusable(event) || node->isFrameOwnerElement() || canScrollInDirection(direction, node))) {
> +            if (!node->renderer())
> +                continue;
> +            FocusCandidate candidate(node);
> +            candidate.enclosingScrollableBox = container;
> +            updateFocusCandidateIfNeeded(direction, startingRect, candidate, closest);
> +        }
> +    }

It is a common convention to write code using early returns/continues/breaks so as to make the code easier to read and follow its flow control.

Additionally, it is unnecessary to re-compute the focused node (i.e. focusedFrame()->document()->focusedNode()) for each iteration. Instead, we should compute it once outside of the loop-body. Then I suggest we use an early continue for when "node == focusedFrame()->document()->focusedNode()". We can also further break down the if-condition into additional early continues.

Moreover, we should hoist the early-continue:

if (!node->renderer())
    continue;

out of the body for the if statement.

On another note, I wish we could simplify and/or shorten the step condition of the for-loop construct. Maybe, we should write this loop using the while-loop construct?

> WebCore/page/FocusController.cpp:713
> +        HTMLFrameOwnerElement* frameElement = static_cast<HTMLFrameOwnerElement*>(focusCandidate.node);
> +        // If we have an iframe without the src attribute, it will not have a contentFrame().
> +        // We should never consider such an iframe as a candidate.
> +        ASSERT(frameElement->contentFrame());
> +
> +        if (hasOffscreenRect(focusCandidate.node, direction)) {
> +            scrollInDirection(focusCandidate.node->document(), direction);
> +            return true;
> +        }
> +        // Navigate into a new frame.
> +        IntRect rect;
> +        Node* focusedNode = focusedOrMainFrame()->document()->focusedNode();
> +        if (focusedNode && !hasOffscreenRect(focusedNode))
> +            rect = nodeRectInAbsoluteCoordinates(focusedNode, true /* ignore border */);
> +        frameElement->contentFrame()->document()->updateLayoutIgnorePendingStylesheets();
> +        if (!navigateInContainer(frameElement->contentFrame()->document(), rect, direction, event)) {
> +            // The new frame had nothing interesting, need to find another candidate.
> +            return navigateInContainer(container, nodeRectInAbsoluteCoordinates(focusCandidate.node, true), direction, event);
> +        }

I am unclear from the ASSERT(frameElement->contentFrame()) and the comment above it whether it is possible for frameElement->contentFrame() to be null. In particular, the second sentence of the comment seems to imply it can happen in practice. So, it seems insufficient to only ASSERT here (which is only useful in a debug build) and subsequently dereference frameElement->contentFrame(), which could be a null pointer. Can frameElement->contentFrame() be null given the context?

> WebCore/page/FocusController.cpp:736
> +    // We found a new focus node, navigate to it.
> +    Element* element = static_cast<Element*>(focusCandidate.node);
> +    ASSERT(element);

I suggest we use toElement() (see Element.h) instead of explicitly performing the static_cast and null check here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.