[webkit-reviews] review denied: [Bug 117265] Spatial Navigation should avoid unwanted calculation while deciding focus candidate. : [Attachment 206329] Updated patch-2

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Abhijeet Kandalkar
<kandalkar.abhijeet58 at gmail.com>'s request for review:
Bug 117265: Spatial Navigation should avoid unwanted calculation while deciding
focus candidate.
https://bugs.webkit.org/show_bug.cgi?id=117265

Attachment 206329: Updated patch-2
https://bugs.webkit.org/attachment.cgi?id=206329&action=review

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


Almost there... r- for now.

> LayoutTests/fast/spatial-navigation/snav-search-optimization.html:3
> +    This test ensures the optimization done in seaching logic to find best
candidate focusable node with minimum iterations.

seaRching

> LayoutTests/fast/spatial-navigation/snav-search-optimization.html:52
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "9"); //
Focused node is 2 and navigation direction is DOWN. Only 1,3,4,5,6,7,8,9,iframe
are considered as valid candidate.
> +	   eventSender.keyDown("downArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "6"); //
Focused node is 5 and navigation direction is DOWN. Only 4,6,7,8,9,iframe are
considered as valid candidate.
> +	   eventSender.keyDown("downArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "2"); //
Focused node is 8 and navigation direction is DOWN. Only 7,9 are considered as
valid candidate.
> +	   eventSender.keyDown("rightArrow");
> +	   eventSender.keyDown("upArrow");
> +	   eventSender.keyDown("leftArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "9"); //
Focused node is 6 and navigation direction is LEFT. Only 1,2,3,4,5,7,8,9,iframe
are considered as valid candidate.
> +	   eventSender.keyDown("leftArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "6"); //
Focused node is 5 and navigation direction is LEFT. Only 1,2,4,7,8,iframe are
considered as valid candidate.
> +	   eventSender.keyDown("leftArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "2"); //
Focused node is 4 and navigation direction is LEFT. Only 1,7 are considered as
valid candidate.
> +	   eventSender.keyDown("upArrow");
> +	   shouldBe("internals.lastSpatialNavigationCandidateCount()", "5"); //
Focused node is 4 and navigation direction is UP. Only 1,2,3,iframe,6 are
considered as valid candidate.

I am not sure if these comments are valid. I would omit them.

Additionally, why should the iframe itself be considered a focus candidate?

> Source/WebCore/page/FocusController.cpp:794
> +    if (focusedFrame() && focusedFrame()->document()) {
> +	   candidateCount +=
focusedFrame()->document()->page()->lastSpatialNavigationCandidateCount();
> +	  
focusedFrame()->document()->page()->setLastSpatialNavigationCandidateCount(cand
idateCount);
> +    }

could you explain this block? maybe a comment in the code..

> Source/WebCore/page/Page.h:547
> +    unsigned m_lastSpatialNavigationCandidatesCount;

please add a comment like

// NOTE: Only called from Internals for testing.

> Source/WebCore/page/SpatialNavigation.cpp:628
> +	   if (candidateRect.x() < currentRect.maxX())

return candidateRect.x() < currentRect.maxX();