Bug 50666 – Spatial Navigation: code clean up

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Antonio Gomes]

Although the spatial navigation is in a great shape, feature complete wise, its code can be improved in some patch.

Patch(es) coming ...

[Antonio Gomes]

(In reply to comment #0)
> ... its code can be improved in some patch.

"parts"

[Antonio Gomes]

Created attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

Patch unifies the two non-default FocusCandidate constructors, making caller sites simpler. Now the special handling given to HTMLAreaElement elements is done within
the non default constructor (i.e. FocusCanditate(Node*, FocusDirection)).

[Yael]

Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=75873&action=review

> WebCore/page/FocusController.cpp:-492
> -        if (!node->renderer())

Removing the check for !node->Renderer() will cause a crash if you have an iframe of size (0 x 0). This is not uncommon thing to do.
I am about to add a layout test to make sure we don't break this in the future.

[Antonio Gomes]

Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

It is not removed it moved to inside FocusCandidate ctor:

+if (!n->renderer())
+  82 return;

[Yael]

(In reply to comment #4)
> (From update of attachment 75873 [details])
> It is not removed it moved to inside FocusCandidate ctor:
> 
> +if (!n->renderer())
> +  82 return;

My bad.

[Antonio Gomes]

> ...  cause a crash if you have an iframe of size (0 x 0). This is not uncommon thing to do.
> I am about to add a layout test to make sure we don't break this in the future.

that would be nice anyways.!

A layout test with 0x0 sized frame and anchor whose display property is set to none should get caught here.

[Yael]

(In reply to comment #6)
> > ...  cause a crash if you have an iframe of size (0 x 0). This is not uncommon thing to do.
> > I am about to add a layout test to make sure we don't break this in the future.
> 
> that would be nice anyways.!
> 
> A layout test with 0x0 sized frame and anchor whose display property is set to none should get caught here.

https://bugs.webkit.org/show_bug.cgi?id=50730
https://bugs.webkit.org/show_bug.cgi?id=50728

[Daniel Bates]

Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=75873&action=review

> WebCore/ChangeLog:9
> +        simpler. Now the special handling HTMLAreaElement gets is done within

The start of the second sentence does not read well. I believe you mean:

"Now the special handling for the HTMLAreaElement gets done within ..."

> WebCore/page/SpatialNavigation.cpp:88
> +    if (n->hasTagName(HTMLNames::areaTag)) {
> +        HTMLAreaElement* area = static_cast<HTMLAreaElement*>(n);
> +        HTMLImageElement* image = area->imageElement();
> +        if (!image)
> +            return;
> +
> +        focusableNode = area;
> +        visibleNode = image;
> +        rect = virtualRectForAreaElementAndDirection(direction, area);
> +        isOffscreen = hasOffscreenRect(image);
> +        isOffscreenAfterScrolling = hasOffscreenRect(image, direction);
> +    } else {
> +        if (!n->renderer())
> +            return;
> +
> +        visibleNode = focusableNode = n;
> +        rect = nodeRectInAbsoluteCoordinates(n, true /* ignore border */);
> +        isOffscreen = hasOffscreenRect(n);
> +        isOffscreenAfterScrolling = hasOffscreenRect(n, direction);
> +    }

It looks like we can move the variable initialization of focusableNode, isOffscreen, and isOffscreenAfterScrolling outside the if-else clauses since they occur in both. Maybe something like:

if (n->hasTagName(HTMLNames::areaTag)) {
	HTMLAreaElement* area = static_cast<HTMLAreaElement*>(n);
	HTMLImageElement* image = area->imageElement();
	if (!image || !image->renderer())
		return;
	visibleNode = image;
	rect = virtualRectForAreaElementAndDirection(direction, area);
} else {
	if (!n->renderer())
		return;
	visibleNode = n;
	rect = nodeRectInAbsoluteCoordinates(n, true /* ignore border */);
}
focusableNode = n;
isOffscreen = hasOffscreenRect(visibleNode);
isOffscreenAfterScrolling = hasOffscreenRect(visibleNode, direction);

[Daniel Bates]

Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=75873&action=review

> WebCore/page/SpatialNavigation.cpp:85
> +        rect = nodeRectInAbsoluteCoordinates(n, true /* ignore border */);

Nit: I don't really like the boolean second argument of nodeRectInAbsoluteCoordinates(). We should consider making the second argument take enum values so that we an remove the comment /* ignore border */. This can be done in a follow up bug.

[Antonio Gomes]

Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

Clearing flags on attachment: 75873

Committed r73627: <http://trac.webkit.org/changeset/73627>

[Antonio Gomes]

Created attachment 76168 [details]
patch 2 - v1

More clean ups

[Eric Seidel (no email)]

Attachment 76168 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6905025

[Eric Seidel (no email)]

Comment on attachment 76168 [details]
patch 2 - v1

r-, breaks mac.

[Antonio Gomes]

(In reply to comment #13)
> (From update of attachment 76168 [details])
> r-, breaks mac.

Could someone help me to see what is the Mac build error about? The logs did not help me :(

[Yael]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 76168 [details] [details])
> > r-, breaks mac.
> 
> Could someone help me to see what is the Mac build error about? The logs did not help me :(

I think you should bring back this declaration:

static void updateFocusCandidateIfNeeded(FocusDirection direction, const IntRect& startingRect, FocusCandidate& candidate, FocusCandidate& closest);

[Antonio Gomes]

Created attachment 76324 [details]
patch 2 - v2 (committed with r73885, r=dbates)

Fixed Mac build fix.

[Daniel Bates]

Comment on attachment 76324 [details]
patch 2 - v2 (committed with r73885, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=76324&action=review

> WebCore/ChangeLog:18
> +        Summary:
> +        - Removed static declaration of updateFocusCandidateIfNeeded() from the top of FocusController.cpp.
> +        It is unneeded;
> +        - In FocusCandidate constructor, renamed 'n' to 'node', and added an assert to it;
> +        - In virtualRectForAreaElementAndDirection, I added an assert to 'node' as well;
> +        - In SpatialNavigation.h:
> +        1) I reordered the declaration of some methods in order to group
> +           related ones;
> +        2) removed isScrollableContainerNode() function declaration since it is not used outside
> +        SpatialNavigation.cpp;
> +        3) and removed the declaration of isNodeDeepDescendantOfDocument() since it does not exist anymore.

It is unusual to give such a detailed summary for a change.

If you feel that the name of the bug is insufficient to describe this change then I suggest  you write a general description of the change and/or highlight the major change in this patch. Then I would move your Summary remarks to after the ':' to the right of the respective file/function in the list of files and functions (below).

For an example of this, see <http://trac.webkit.org/changeset/73829>. Notice, a description is omitted in the change log entry as the bug title is sufficiently detailed.

> WebCore/page/SpatialNavigation.cpp:54
>  static IntRect rectToAbsoluteCoordinates(Frame* initialFrame, const IntRect& rect);

Nit: Although outside of this patch, the name of the second argument "rect" should be removed/renamed as it doesn't add any value to this function prototype.

> WebCore/page/SpatialNavigation.cpp:56
> +static bool isScrollableContainerNode(const WebCore::Node*);

Do we need the "WebCore::" prefix here? Looking at the code, this declaration is within the WebCore namespace.

> WebCore/page/SpatialNavigation.cpp:695
>  IntRect virtualRectForAreaElementAndDirection(FocusDirection direction, HTMLAreaElement* area)

Nit: The name of this function disagrees with the ordering of its arguments. In particular, its name implies that the argument order should be HTMLAreaElement*, then FocusDirection.

> WebCore/page/SpatialNavigation.cpp:697
> +    ASSERT(area);

I suggest that we also ASSERT(area->imageElement()).

> WebCore/page/SpatialNavigation.h:143
> +Node* scrollableEnclosingBoxOrParentFrameForNodeInDirection(FocusDirection, Node* node);

Please remove the name of the second argument "node" as it doesn't add any value to this prototype declaration.

[Antonio Gomes]

> It is unusual to give such a detailed summary for a change.
> 
> ... Then I would move your Summary remarks to after the ':' to the right of the respective file/function in the list of files and functions (below).

Done.

> > WebCore/page/SpatialNavigation.cpp:56
> > +static bool isScrollableContainerNode(const WebCore::Node*);
> 
> Do we need the "WebCore::" prefix here? Looking at the code, this declaration is within the WebCore namespace.

It is not needed. Removed.
 
> > WebCore/page/SpatialNavigation.cpp:697
> > +    ASSERT(area);
> 
> I suggest that we also ASSERT(area->imageElement()).

Done.

> > WebCore/page/SpatialNavigation.h:143
> > +Node* scrollableEnclosingBoxOrParentFrameForNodeInDirection(FocusDirection, Node* node);
> 
> Please remove the name of the second argument "node" as it doesn't add any value to this prototype declaration.

Done.

[Antonio Gomes]

Comment on attachment 76324 [details]
patch 2 - v2 (committed with r73885, r=dbates)

Clearing flags on attachment: 76324

Committed r73885: <http://trac.webkit.org/changeset/73885>

[Antonio Gomes]

Wont close it, since there is more stuff coming ...

[Eric Seidel (no email)]

It's generally preferred to use master bugs, and do one-bug-per change, but multi-patch bugs are certainly not "forbidden". :)

[Antonio Gomes]

(In reply to comment #21)
> It's generally preferred to use master bugs, and do one-bug-per change, but multi-patch bugs are certainly not "forbidden". :)

I know :( I promiss it won't happen again othen than here :)

[Antonio Gomes]

Created attachment 76498 [details]
patch 3 - v1 (committed r74003, r=dbates)

Some more clean ups

[Antonio Gomes]

Created attachment 76499 [details]
patch 4 - v1 (committed r74004, r=dbates)

[Daniel Bates]

Comment on attachment 76498 [details]
patch 3 - v1 (committed r74003, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=76498&action=review

> WebCore/ChangeLog:11
> +        * page/SpatialNavigation.h: Added FocusCandidate::isFrameOwnerElement and WebCore::toFrameOwnerElement helper functions.
> +        (WebCore::FocusCandidate::isFrameOwnerElement): Checks is the Node pointer wrapped by FocusCandidate is a instead of HTMLFrameOwnerElement.

Nit: We tend to wrap change log lines at around 80 columns.

> WebCore/ChangeLog:13
> +        (WebCore::toFrameOwnerElement): Static casts the FocusCandidate given as parameter to HTMLFrameOwnerElement if appropriated.

Nit: This sentence does not read well. Maybe, write "Returns the HTMLFrameOwnerElement associated with the FocusCandidate if appropriate"?

> WebCore/page/SpatialNavigation.cpp:704
> +HTMLFrameOwnerElement* toFrameOwnerElement(FocusCandidate& candidate)

The name of this function seems strange. In particular, the prefix "to" implies that the FocusCandidate will be transformed into an HTMLFrameOwnerElement (like toRenderTable casting a RenderObject* to a RenderTable*). However, this function actually transforms the visibleNode of a FocusCandidate object from type Node* to HTMLFrameOwnerElement*.

Maybe rename this function frameOwnerElement(FocusCandidate& candidate)?

[Daniel Bates]

Comment on attachment 76499 [details]
patch 4 - v1 (committed r74004, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=76499&action=review

Looks good to me.
r=me.

> WebCore/page/FocusController.cpp:505
> +        // spatial navigation algorithm to continue, skipping this container.

Nit: I think it may be a bit clearer to write this line as "spatial navigation algorithm will skip this container."

[Antonio Gomes]

Created attachment 76502 [details]
patch 5 - v1 (committed r74006, r=dbates)

(This one just need a quick rubber stamp)

[Antonio Gomes]

Comment on attachment 76498 [details]
patch 3 - v1 (committed r74003, r=dbates)

Clearing flags on attachment: 76498

Committed r74003: <http://trac.webkit.org/changeset/74003>

[Antonio Gomes]

Comment on attachment 76499 [details]
patch 4 - v1 (committed r74004, r=dbates)

Clearing flags on attachment: 76499

Committed r74004: <http://trac.webkit.org/changeset/74004>

[Antonio Gomes]

Comment on attachment 76502 [details]
patch 5 - v1 (committed r74006, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=76502&action=review

> WebCore/ChangeLog:12
> +        (WebCore::distanceDataForNode): Made 'FocusCandidate current' const since it is not supposed
> +        changed within this function.

typo 'to be changed' fixed locally.

[Antonio Gomes]

Comment on attachment 76502 [details]
patch 5 - v1 (committed r74006, r=dbates)

Clearing flags on attachment: 76502

Committed r74006: <http://trac.webkit.org/changeset/74006>

[Antonio Gomes]

Created attachment 77087 [details]
patch 6 - v1 (committed r74723, r=dbates)

[Daniel Bates]

Comment on attachment 77087 [details]
patch 6 - v1 (committed r74723, r=dbates)

View in context: https://bugs.webkit.org/attachment.cgi?id=77087&action=review

r=me.

> WebCore/ChangeLog:13
> +        (WebCore::updatFocusCandidateIfNeeded): Assert if !renderer() or !isElementNode()
> +        since we are bailing bailing our earlir in FocusCandidate constructor and
> +        FocusController::findFocusCandidateInContainer respectively;

I believe you meant to write:

Assert renderer() and isElementNode() now that we are bailing out earlier in both the FocusCandidate constructor and FocusController::findFocusCandidateInContainer().

> WebCore/ChangeLog:17
> +        i(WebCore::FocusController::findFocusCandidateInContainer):
> +        (WebCore::FocusController::advanceFocusDirectionallyInContainer): Adjusted callsites

Remove the leading 'i' before the "(WebCore::FocusController..." on line 16.

callsites => call sites

> WebCore/ChangeLog:19
> +        (WebCore::FocusController::advanceFocusDirectionally): Adjusted callsite of

callsite => call site

> WebCore/ChangeLog:23
> +        (WebCore::FocusCandidate::FocusCandidate): Assert if node is not a element node;
> +        (WebCore::isScrollableNode): Renamed from isScrollableContainerNode;

element => Element (as per "The DOM Structure Model" <http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1590626202>).

; => . (on both line 22 and 23)

> WebCore/ChangeLog:24
> +        (WebCore::scrollInDirection): Adjusted callsite after function name change;

callsite => call site

; => .

> WebCore/ChangeLog:26
> +        a documentNode;

documentNode => "Document node" (as per "The DOM Structure Model" <http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-1590626202>).

; => .

> WebCore/page/FocusController.cpp:475
> +    for (; node; node = (node->isFrameOwnerElement() || canScrollInDirection(node, direction)) ? node->traverseNextSibling(container) : node->traverseNextNode(container)) {

I still do not like this for-loop since it's long to follow and its step function changes depending on the truth value of "node->isFrameOwnerElement() || canScrollInDirection(node, direction)".

Maybe use a while-loop that has the form:

Node* next = container->firstChild();
while (next) {
   ...
   next = next->isFrameOwnerElement() || canScrollInDirection(next, direction) ? next->traverseNextSibling(container) : next->traverseNextNode(container);
}

> WebCore/page/FocusController.cpp:585
> +            startingRect = virtualRectForAreaElementAndDirection(area, direction);

Yay! The name of the function now matches the argument order.

> WebCore/page/SpatialNavigation.cpp:431
>          return (renderer->isBox() && toRenderBox(renderer)->canBeScrolledAndHasScrollableArea()
> -             && node->hasChildNodes() && !node->isDocumentNode());
> +             && node->hasChildNodes());

I would remove the outer parentheses and write this return on one line now that we have less conjuncts.

[Antonio Gomes]

Comment on attachment 77087 [details]
patch 6 - v1 (committed r74723, r=dbates)

Clearing flags on attachment: 77087

Committed r74723: <http://trac.webkit.org/changeset/74723>

[WebKit Review Bot]

http://trac.webkit.org/changeset/74723 might have broken Leopard Intel Debug (Tests)

[Antonio Gomes]

Code is cleaner now. Lets close it.