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

[bugzilla-daemon at webkit.org]

Antonio Gomes (tonikitoo) <tonikitoo at webkit.org> has asked  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 Antonio Gomes (tonikitoo)
<tonikitoo at webkit.org>
Addressed Eric's comments.


(In reply to comment #57)
> (From update of attachment 48141 [details])
> There are huge sections of copy/paste code here.  Couldn't those be turned
into
> template functions which used -1/+1 to denote up/down/left/right?  I feel
like
> that would make the code cleaner, because although it would be a little
awkward
> to use the functions, you're always 100% certain that tehre are no copy paste

> errors with them.

Done. I introduced the following macros: START, MIDDLE, END, which tak
|direction| and
|rect| as parameter. It makes 'if's in areRectsFullyAligned and
areRectsPartlyAligned
not copy and paste anymore.

> It would be possible to land this in smaller, more easily reviewed steps,
> including just the layout tests first, followed by the code change.

Done. This patch does not include the LayoutTests.


> 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	   }

It does have. Using it now. The qouted code above has gone in favor of ir.

> 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.

Done. I put this in a new method call "deepFindFocusableNodeInDirection()".

> Yeah, probably one for each half of the if.

Also added a static helper: "updateFocusCandidateIfCloser()".

> 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) {

Done.

> 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.

Done - renamed.

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

The microb code is node needed anymore, so we do not need to care about
licenses.
I drop the method spatialDistanceForAlignedRects. only "spatialDistance" is
being used.

> 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.

I hope we are in a better shape now.