[webkit-reviews] review denied: [Bug 49382] Spatial Navigation: issues with the node selection algorithm. : [Attachment 73729] Patch including code and no tests

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 14 07:30:15 PST 2010


Antonio Gomes <tonikitoo at webkit.org> has denied  review:
Bug 49382: Spatial Navigation: issues with the node selection algorithm.
https://bugs.webkit.org/show_bug.cgi?id=49382

Attachment 73729: Patch including code and no tests
https://bugs.webkit.org/attachment.cgi?id=73729&action=review

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

Nice work! 

It needs one more round, though. I would like to get some of my comments
addressed and questions answered.

> WebCore/page/SpatialNavigation.cpp:291
> +	   return curRect.x() < targetRect.right() || curRect.x() -
targetRect.right() > viewSize.width();

"curRect.x() < targetRect.right()". This should not be tested here OR the
method has to be renamed. iirc there were other methods that would catch these
cases.

areRectsTooFarApart essentially test the second conditions e.g. "curRect.x() -
targetRect.right() > viewSize.width();"

> WebCore/page/SpatialNavigation.cpp:293
> +	   return targetRect.x() < curRect.right() || targetRect.x() -
curRect.right() > viewSize.width();

ditto

> WebCore/page/SpatialNavigation.cpp:295
> +	   return curRect.y() < targetRect.bottom() || curRect.y() -
targetRect.bottom() > viewSize.height();

ditto

> WebCore/page/SpatialNavigation.cpp:297
> +	   return targetRect.y() < curRect.bottom() || targetRect.y() -
curRect.bottom() > viewSize.height();

ditto

> WebCore/page/SpatialNavigation.cpp:524
> +bool scrollInDirection(Node* container, FocusDirection direction, const
FocusCandidate&)

I wish, if possible you keep using the enclosingScrollableBox logic in
FocusCandidate.

That way you should just do

scrollInDirection(direction, focusCandidate);

focuscandidate would have a reference for the 'container' node.

> WebCore/page/SpatialNavigation.cpp:653
> +Node* scrollableOrFrameParentForNodeAndDirection(FocusDirection direction,
Node* node)

FocusCandidate POD has a enclosingScrollableBox. Why not using this?

> WebCore/page/SpatialNavigation.cpp:659
> +	   if (parent->isDocumentNode())
> +	       parent =
static_cast<Document*>(parent)->document()->frame()->ownerElement();

put a 'break' right below here, and remove the isDocumentNode check from the
loop condition.

> WebCore/page/SpatialNavigation.cpp:673
> +    if (!container->renderBox() ||
!container->renderBox()->canBeScrolledAndHasScrollableArea())
> +	   return false;

why not use isScrollableContainerNode() ?

> WebCore/page/SpatialNavigation.cpp:695
> +    if ((direction == FocusDirectionLeft || direction ==
FocusDirectionRight) && !frame->view()->horizontalScrollbar())
> +	   return false;

Will it work with scrollbar policy thing of the Qt API? It is toggled OFF for
WRT.

> WebCore/page/SpatialNavigation.cpp:717
> +IntRect nodeRectInAbsCoords(Node* node, bool ignoreBorder)

Abs => Absolute

> WebCore/page/SpatialNavigation.cpp:732
> +    for (Frame* frame = node->document()->frame(); frame; frame =
frame->tree()->parent()) {
> +	   if (Element* element = static_cast<Element*>(frame->ownerElement()))
{
> +	       do {
> +		   rect.move(element->offsetLeft(), element->offsetTop());
> +	       } while ((element = element->offsetParent()));
> +	       rect.move((-frame->view()->scrollOffset()));
> +	   }
> +    }

This steps are duplicated here and in frameRectInAbsCoords. Lets make it a
helper.

> WebCore/page/SpatialNavigation.cpp:734
> +    // For authors that use border indtead of outline in their CSS, we
compensate by ignoring the border when calculating

typo: inStead

> WebCore/page/SpatialNavigation.cpp:744
> +IntRect frameRectInAbsCoords(Frame* frame)

Abs -> Absolute.

> WebCore/page/SpatialNavigation.cpp:755
> +    for (; frame; frame = frame->tree()->parent()) {
> +	   if (Element* element = static_cast<Element*>(frame->ownerElement()))
{
> +	       do {
> +		   rect.move(element->offsetLeft(), element->offsetTop());
> +	       } while ((element = element->offsetParent()));
> +	       rect.move((-frame->view()->scrollOffset()));
> +	   }
> +    }

Duplicated code.

> WebCore/page/SpatialNavigation.cpp:816
> +void distanceDataForNode(FocusDirection direction, const IntRect&
startingRect, FocusCandidate& candidate)

In my opinion,the signature of this method is inconsistent: you should be
either pass the direction and both nodes OR the direction and both rects.

Maybe we need this first, where "startingNode" would be "FocusCandidate
focusedNode". If there is not current focused not, focusedNode.isNull() would
catch it and then you get your wanted starting rect from within this method.


More information about the webkit-reviews mailing list