[webkit-reviews] review granted: [Bug 36463] Spatial Navigation: make it work with focusable elements in overflow content : [Attachment 58644] patch v5.4 - same as patch v5.4 but fixed style bug

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Antonio Gomes
(tonikitoo) <tonikitoo at webkit.org>'s request for review:
Bug 36463: Spatial Navigation: make it work with focusable elements in overflow
content
https://bugs.webkit.org/show_bug.cgi?id=36463

Attachment 58644: patch v5.4 - same as patch v5.4 but fixed style bug
https://bugs.webkit.org/attachment.cgi?id=58644&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>


> +    bool sameContainer;
> +    if (candidate.document() != closest.document())
> +	   sameContainer = false;
> +    else
> +	   sameContainer = candidate.enclosingScrollableBox ==
closest.enclosingScrollableBox;

This would be clearer as
  bool sameContainer == candidate.document() == closest.document() &&
candidate.enclosingScrollableBox == closest.enclosingScrollableBox;

> +    // ### !sameContainerAsCandidate && !sameContainerAsClosest

Odd comment style; please don't use ####

> +	       // 1) If candidateParent is not null, and it holds the distance
and alignment data of the
> +	       // parent container element itself.
> +	       if (!candidateParent.isNull()) {
> +		   currentFocusCandidate.parentAlignment =
candidateParent.alignment;
> +		   currentFocusCandidate.parentDistance =
candidateParent.distance;
> +		   currentFocusCandidate.enclosingScrollableBox =
candidateParent.node;
> +
> +	       // 2) Parent of outer is either:
> +	       // * <frame> or <iframe>;
> +	       // * or any other scrollable block element.
> +	       } else if (!isInRootDocument(outer)) {
> +		   if (Document* document =
static_cast<Document*>(outer->parent()))
> +		       currentFocusCandidate.enclosingScrollableBox =
static_cast<Node*>(document->ownerElement());
> +

Put the comments inside the relevant braces. This style is confusing.

r=me with those comments addressed.