[webkit-reviews] review granted: [Bug 120862] MouseEnter and MouseLeave may be emitted on Document nodes : [Attachment 210761] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Allan Sandfeld Jensen
<allan.jensen at digia.com>'s request for review:
Bug 120862: MouseEnter and MouseLeave may be emitted on Document nodes
https://bugs.webkit.org/show_bug.cgi?id=120862

Attachment 210761: Patch
https://bugs.webkit.org/attachment.cgi?id=210761&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210761&action=review


> Source/WebCore/dom/Document.cpp:5868
>	       for (Node* node = oldHoveredElement.get(); node; node =
node->parentNode()) {
> -		   if (!mustBeInActiveChain || (node->isElementNode() &&
toElement(node)->inActiveChain()))
> -		       nodesToRemoveFromChain.append(node);
> +		   if (!node->isElementNode())
> +		       continue;
> +		   Element* element = toElement(node);
> +		   if (!mustBeInActiveChain || element->inActiveChain())
> +		       elementsToRemoveFromChain.append(element);
>	       }

Should just write:

    for (Element* element = oldHoveredElement.get(); element; element =
element->parentElement()) {

That would eliminate three lines of code below.

Also, mustBeInActiveChain should be checked outside the loop, not inside it.

> Source/WebCore/dom/Document.cpp:5878
>	   for (RenderObject* curr = oldHoverObj; curr && curr != ancestor;
curr = curr->hoverAncestor()) {
> -	       if (!curr->node() || curr->isText())
> +	       if (!curr->node() || !curr->node()->isElementNode())
>		   continue;
> -	       if (!mustBeInActiveChain || (curr->node()->isElementNode() &&
toElement(curr->node())->inActiveChain()))
> -		   nodesToRemoveFromChain.append(curr->node());
> +	       Element* element = toElement(curr->node());
> +	       if (!mustBeInActiveChain || element->inActiveChain())
> +		   elementsToRemoveFromChain.append(element);
>	   }

!mustBeInActiveChain should be checked outside the loop

> Source/WebCore/dom/Document.cpp:5893
>      for (RenderObject* curr = newHoverObj; curr; curr =
curr->hoverAncestor()) {
> -	   if (!curr->node() || curr->isText())
> +	   if (!curr->node() || !curr->node()->isElementNode())
>	       continue;
> -	   if (!mustBeInActiveChain || (curr->node()->isElementNode() &&
toElement(curr->node())->inActiveChain()))
> -	       nodesToAddToChain.append(curr->node());
> +	   Element* element = toElement(curr->node());
> +	   if (!mustBeInActiveChain || element->inActiveChain())
> +	       elementsToAddToChain.append(element);
>      }

!mustBeInActiveChain should be checked outside the loop

> Source/WebCore/dom/Document.cpp:5896
> -    size_t removeCount = nodesToRemoveFromChain.size();
> +    size_t removeCount = elementsToRemoveFromChain.size();
>      for (size_t i = 0; i < removeCount; ++i) {

Our modern style for this is:

    for (size_t i = 0, size = elementsToRemoveFromChain.size(); i < size; ++i)
{

Please consider using that style.