Bug 50666

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

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

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

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

(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 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 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 on attachment 75873 [details] patch v1 (committed r73627, r=dbates) Clearing flags on attachment: 75873 Committed r73627: <http://trac.webkit.org/changeset/73627>

Created attachment 76168 [details] patch 2 - v1 More clean ups

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

Comment on attachment 76168 [details] patch 2 - v1 r-, breaks mac.

(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 :(

(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);

Created attachment 76324 [details] patch 2 - v2 (committed with r73885, r=dbates) Fixed Mac build fix.

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.

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

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

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

(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 :)

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

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

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

Created attachment 76502 [details] patch 5 - v1 (committed r74006, r=dbates) (This one just need a quick rubber stamp)

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 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 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 on attachment 76502 [details] patch 5 - v1 (committed r74006, r=dbates) Clearing flags on attachment: 76502 Committed r74006: <http://trac.webkit.org/changeset/74006>

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

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 on attachment 77087 [details] patch 6 - v1 (committed r74723, r=dbates) Clearing flags on attachment: 77087 Committed r74723: <http://trac.webkit.org/changeset/74723>

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

Code is cleaner now. Lets close it.