[webkit-reviews] review denied: [Bug 18662] Extend keyboard navigation to allow directional navigation : [Attachment 49074] Add directional navigation to WebCore (v14)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antonio Gomes
(tonikitoo) <tonikitoo at webkit.org>'s request for review:
Bug 18662: Extend keyboard navigation to allow directional navigation
https://bugs.webkit.org/show_bug.cgi?id=18662

Attachment 49074: Add directional navigation to WebCore (v14)
https://bugs.webkit.org/attachment.cgi?id=49074&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> +    FocusDirection focusDirectionForKey(const AtomicString&);

This method should be |const|



> +	   FocusDirection tabDirection = (FocusDirectionUp || direction ==
FocusDirectionLeft) ?
> +					  FocusDirectionForward :
FocusDirectionBackward;

Shouldn't this be:

  FocusDirection tabDirection = ( direction == FocusDirectionUp || direction ==
FocusDirectionLeft)
?

> +    // Move up in the chain of nested frames.
> +    frame = frame->tree()->top();

I don't undersand this. Why jump to the topmost frame?

> +	   if (!(isMovingToRootDocument(focusedNode, candidate)
> +	       && (closestFocusCandidate.node
> +	       && (focusedNode->document() ==
closestFocusCandidate.node->document())))) {

You don't need so many parens here.



>  
>  namespace WebCore {
>      enum FocusDirection {
>	   FocusDirectionForward = 0,
> -	   FocusDirectionBackward
> +	   FocusDirectionBackward,
> +	   FocusDirectionNone,
> +	   FocusDirectionUp,
> +	   FocusDirectionDown,
> +	   FocusDirectionLeft,
> +	   FocusDirectionRight
>      };

Would making FocusDirectionNone have value 0 break anything? It would be
preferable.


> +++ b/WebCore/page/SpatialNavigation.cpp


> +namespace WebCore {
> +
> +long long spatialDistance(FocusDirection, const IntRect&, const IntRect&);
> +IntRect renderRectRelativeToRootDocument(RenderObject*);
> +RectsAlignment alignmentForRects(FocusDirection, const IntRect&, const
IntRect&);
> +bool areRectsFullyAligned(FocusDirection, const IntRect&, const IntRect&);
> +bool areRectsPartiallyAligned(FocusDirection, const IntRect&, const
IntRect&);
> +bool isRectInDirection(FocusDirection, const IntRect&, const IntRect&);

I'm not sure if we normally declare file-local functions as 'static'. Take a
look at some other files to see.

> +// FIXME_tonikitoo: FocusCandidate distanceDataForNode(start, dest,
direction)

It's not clear what the action item is.

> +long long distanceInDirection(Node* start, Node* dest, FocusDirection
direction, FocusCandidate& candidate)
> +{
> +    long long& distance = candidate.distance;

You set distance to be a reference to candidate.distance here...

> +	   // Reset distance if we are now in higher precedence case.
> +	   if (candidate.alignment < alignment && candidate.parentAlignment <
alignment)
> +	       distance = cMaxDistance;

and then assign to it lower down. Why use the reference at all? This is hard to
follow.

> +	   // FIXME_tonikitoo: make this assignment in the caller method ?

Either do it, or remove the FIXME.


> +    // There are cases, mostly when |render|'s associated node is offscreen,

> +    // it is safer to get its position regardless the container scroll
offset.

This is a bit obscure. Why?

> +    if (useScrollOffset)
> +	   if (FrameView* frameView = render->node()->document()->view())
> +	       rect.move(-frameView->scrollOffset());

We would normally put braces at if (useScrollOffset) {

> +#define MIDDLE(direction, rect)	 \
> +    isHorizontalMove(direction) ?	 \
> +	   rect.y() + rect.height() / 2: \
> +	   rect.x() + rect.width()  / 2;
> +
> +#define END(direction, rect) 	 \
> +    isHorizontalMove(direction) ?	 \
> +	   rect.y() + rect.height() :	 \
> +	   rect.x() + rect.width();
> +
> +#define START(direction, rect)	 \
> +    isHorizontalMove(direction) ? rect.y() : rect.x();

Please convert these into inline functions.

> +// This method checks if rects |a| and |b| are fully aligned either vertical
or

vertically

> +// horizontally. Rects that match this criteria are preferable target nodes
in
> +// move focus changing operations.
> +// In essence, targets whose middle falls between the top or bottom of the
current rect.

This isn't a complete sentence.

> +    // About the logic below:

What about it?

> +    return ((bStart >= aStart && bStart <= aEnd)
> +	       || (bStart >= aStart && bStart <= aEnd)
> +	       || (bEnd >= aStart && bEnd <= aEnd)
> +	       || (bMiddle >= aStart && bMiddle <= aEnd)
> +	       || (bEnd >= aStart && bEnd <= aEnd));
> +}
> +
> +// * a = Current focused node.
> +// * b = Focus candidate node.

In all these methods, rather than commenting what a and b are, you should just
name them appropriately: curRect, targetRect, or something.


> +	   if (a.y() > b.y() + b.height()) {

use IntRect:bottom(), IntRect:right() here and in all these methods.

You could also define some inline methods like:

bool below(const IntRect& r, int pos)
{
  return pos > r.bottom();
}

to make this code more readable. You could also add IntPoint IntRect::center()
const if you like (FloatRect has one).

> +bool isMovingToRootDocument(Node* from, Node* to)
> +{
> +    if (!from || !to)
> +	   return false;
> +
> +    Document* rootDocument =
to->document()->page()->mainFrame()->document();
> +    return (to->document() == rootDocument && from->document() !=
to->document());
> +}

The 'moving to' terminology is confusing. I think this would be clearer with:

bool isInRootDocument(Node*)

and then put the other logic at the call site.

> +void deflateIfNeeded(IntRect& a, IntRect& b, int fudgeFactor)

If needed for what?

I think this needs one more round of changes.