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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> 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 48141: Add directional navigation to WebCore (v13+)
https://bugs.webkit.org/attachment.cgi?id=48141&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Doesn't frametree have a top() method?
 300	 // Move up in the chain of nested frames.
 301	 while (true) {
 302	     Frame* parentFrame = frame->tree()->parent();
 303	     if (!parentFrame || !parentFrame->document())
 304		 break;
 305 
 306	     frame = parentFrame;
 307	 }

Seems slightly dangerous having this so far away from the isElement check:
 343	 static_cast<Element*>(node)->focus(false);

I think I owuld haev made this whole block:
 473		     // Check if the current {i}frame element itself is a good
candidate
its own small function.
Yeah, probably one for each half of the if.

Another way to make part of this more readable (to my eye) woudl have been to
use early return for things like:
 492		 if (distance < closestFocusCandidate.distance) {

We generally avoid files called "utils" or "utilities" as they become sewers
very quickly.  In this case, I woudl just have called this file
"SpatialNavigation.h" and probably still left most of the content here.

Why is this OK license wise?
 160 // Based on the MICROB SNav logic (MPL/GLP/LGPG), excepted by the Down
logic.

I don't think it's OK to add code w/o citing the full license, but I'm not a
lawyer.

The ifs in areRectsFullyAligned are copy paste!
So is spatialDistanceForAlignedRects.

This is too hard to review at this size, and this amount of copy/paste code.

I would split the code out from the layout tests.  Get the layout tests
approved and possibly landed (skipped) and then work on landing the code in
parts, ideally with as little copy paste code as possible.

WebKit wants this functionality.  It's just impossible for a reviewer to do
anything more than rubber-stamp this code at this size.  Given all the copy
paste code I don't know how I could be sure that you haven't missed a minus
sign or forgotten to fix one half of a copy/paste if.  Again these things make
it difficult to r+.

r-  Mostly for the copy paste, partially for the license confusion.