[webkit-reviews] review denied: [Bug 18662] Extend keyboard navigation : [Attachment 22634] Add directional navigation to WebCore

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Marco Barisione
<marco.barisione at collabora.co.uk>'s request for review:
Bug 18662: Extend keyboard navigation
https://bugs.webkit.org/show_bug.cgi?id=18662

Attachment 22634: Add directional navigation to WebCore
https://bugs.webkit.org/attachment.cgi?id=22634&action=edit

------- Additional Comments from Sam Weinig <sam at webkit.org>
This is great patch.  I think for it to go in we really need a way platforms
too choose whether to use this or not.	Perhaps it can go in Settings.	Some
style issues follow.  r- for the platform choice issue for now.

	 if (event->keyIdentifier() == "U+0009")
	     defaultTabEventHandler(event);
+	  else if (event->keyIdentifier() == "Down" || event->keyIdentifier()
== "Up" ||
+		  event->keyIdentifier() == "Right" || event->keyIdentifier()
== "Left")
+	      defaultArrowEventHandler(event);

There are some whitespace issues here.

inline IntPoint realLocation(RenderObject* render)
inline int rectangleDistance(IntRect rect1, IntRect rect2)
inline int rectangleDistance(IntRect rect1, IntRect rect2)
inline bool isPointAboveAntiDiagonal(const IntPoint& rectPoint, const IntPoint&
point)
inline bool isRectAboveMainDiagonal(const IntPoint& rectPoint, const IntRect&
rect)
inline bool isRectBelowMainDiagonal(const IntPoint& rectPoint, const IntRect&
rect)
inline bool isRectAboveAntiDiagonal(const IntPoint& rectPoint, const IntRect&
rect)
inline bool isRectBelowAntiDiagonal(const IntPoint& rectPoint, const IntRect&
rect)

These should all be marked static to get internal linkage if they are not
inlined.

+	     ASSERT(false);

This can be written as ASSERT_NOT_REACHED();

+class BestCandidate

This should be a struct,  

+{

{ should go on the first line

+public:
+    BestCandidate() : node(0), distance(INT_MAX) {}

We like to style this as 

BestCandidate()
    : node(0)
    , distance(numeric_limits<int>::max())
{
}

+    for (Node* candidate = document->firstChild(); candidate; candidate =
candidate->traverseNextNode())
+	 if (candidate->isFrameOwnerElement()) {

The for loop should have braces.

+    Node* focusedNode = focusedDocument->focusedNode();
+    if (!focusedNode)
+	 // Just move to the first focusable node.
+	 // initialFocus is set to true so the chrome is not focused.
+	 return advanceFocusLinear(FocusDirectionForward, event, true);

This if should have braces.  Our rule is that if there is more than one line
(code or comments) we use braces. 

+    if (!node)
+      // Leave the focus on the same node.
+      return true;

Here too.

+    if (!node->isElementNode())
+	 // FIXME: May need a way to focus a document here.
+	 return false;

And here.