Bug 50666 - Spatial Navigation: code clean up
Summary: Spatial Navigation: code clean up
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 61606
Blocks: 46905
  Show dependency treegraph
 
Reported: 2010-12-07 22:20 PST by Antonio Gomes
Modified: 2011-05-30 09:47 PDT (History)
7 users (show)

See Also:


Attachments
patch v1 (committed r73627, r=dbates) (5.47 KB, patch)
2010-12-07 22:32 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 2 - v1 (7.48 KB, patch)
2010-12-09 23:44 PST, Antonio Gomes
eric: review-
Details | Formatted Diff | Diff
patch 2 - v2 (committed with r73885, r=dbates) (8.09 KB, patch)
2010-12-12 06:13 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 3 - v1 (committed r74003, r=dbates) (6.07 KB, patch)
2010-12-13 22:12 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 4 - v1 (committed r74004, r=dbates) (4.25 KB, patch)
2010-12-13 22:13 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 5 - v1 (committed r74006, r=dbates) (2.92 KB, patch)
2010-12-13 22:43 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 6 - v1 (committed r74723, r=dbates) (12.70 KB, patch)
2010-12-20 23:37 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-12-07 22:20:43 PST
Although the spatial navigation is in a great shape, feature complete wise, its code can be improved in some patch.

Patch(es) coming ...
Comment 1 Antonio Gomes 2010-12-07 22:21:52 PST
(In reply to comment #0)
> ... its code can be improved in some patch.

"parts"
Comment 2 Antonio Gomes 2010-12-07 22:32:55 PST
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)).
Comment 3 Yael 2010-12-08 09:15:37 PST
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.
Comment 4 Antonio Gomes 2010-12-08 09:21:21 PST
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;
Comment 5 Yael 2010-12-08 09:57:43 PST
(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.
Comment 6 Antonio Gomes 2010-12-08 10:01:16 PST
> ...  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.
Comment 7 Yael 2010-12-09 05:31:50 PST
(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
Comment 8 Daniel Bates 2010-12-09 09:57:39 PST
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);
Comment 9 Daniel Bates 2010-12-09 10:00:17 PST
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.
Comment 10 Antonio Gomes 2010-12-09 11:04:44 PST
Comment on attachment 75873 [details]
patch v1 (committed r73627, r=dbates)

Clearing flags on attachment: 75873

Committed r73627: <http://trac.webkit.org/changeset/73627>
Comment 11 Antonio Gomes 2010-12-09 23:44:50 PST
Created attachment 76168 [details]
patch 2 - v1

More clean ups
Comment 12 Eric Seidel (no email) 2010-12-10 00:06:52 PST
Attachment 76168 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6905025
Comment 13 Eric Seidel (no email) 2010-12-10 02:41:44 PST
Comment on attachment 76168 [details]
patch 2 - v1

r-, breaks mac.
Comment 14 Antonio Gomes 2010-12-10 12:03:18 PST
(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 :(
Comment 15 Yael 2010-12-10 12:24:42 PST
(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);
Comment 16 Antonio Gomes 2010-12-12 06:13:37 PST
Created attachment 76324 [details]
patch 2 - v2 (committed with r73885, r=dbates)

Fixed Mac build fix.
Comment 17 Daniel Bates 2010-12-12 12:41:41 PST
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.
Comment 18 Antonio Gomes 2010-12-12 21:44:33 PST
> 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.
Comment 19 Antonio Gomes 2010-12-12 21:53:13 PST
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>
Comment 20 Antonio Gomes 2010-12-12 22:54:24 PST
Wont close it, since there is more stuff coming ...
Comment 21 Eric Seidel (no email) 2010-12-12 23:29:02 PST
It's generally preferred to use master bugs, and do one-bug-per change, but multi-patch bugs are certainly not "forbidden". :)
Comment 22 Antonio Gomes 2010-12-13 10:07:01 PST
(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 :)
Comment 23 Antonio Gomes 2010-12-13 22:12:31 PST
Created attachment 76498 [details]
patch 3 - v1 (committed r74003, r=dbates)

Some more clean ups
Comment 24 Antonio Gomes 2010-12-13 22:13:07 PST
Created attachment 76499 [details]
patch 4 - v1 (committed r74004, r=dbates)
Comment 25 Daniel Bates 2010-12-13 22:30:05 PST
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)?
Comment 26 Daniel Bates 2010-12-13 22:37:22 PST
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."
Comment 27 Antonio Gomes 2010-12-13 22:43:27 PST
Created attachment 76502 [details]
patch 5 - v1 (committed r74006, r=dbates)

(This one just need a quick rubber stamp)
Comment 28 Antonio Gomes 2010-12-13 22:53:31 PST
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>
Comment 29 Antonio Gomes 2010-12-13 22:54:29 PST
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>
Comment 30 Antonio Gomes 2010-12-13 22:56:26 PST
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.
Comment 31 Antonio Gomes 2010-12-13 23:23:04 PST
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>
Comment 32 Antonio Gomes 2010-12-20 23:37:42 PST
Created attachment 77087 [details]
patch 6 - v1 (committed r74723, r=dbates)
Comment 33 Daniel Bates 2010-12-22 21:38:56 PST
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.
Comment 34 Antonio Gomes 2010-12-28 13:58:17 PST
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>
Comment 35 WebKit Review Bot 2010-12-28 15:02:57 PST
http://trac.webkit.org/changeset/74723 might have broken Leopard Intel Debug (Tests)
Comment 36 Antonio Gomes 2011-05-30 09:47:04 PDT
Code is cleaner now. Lets close it.