Bug 104685

Summary: Add Element-specific traversal functions
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, apinheiro, cfleizach, cmarcelo, darin, dglazkov, dmazzoni, d-r, eric, fmalita, jdiggs, kling, macpherson, menard, mifenton, ojan.autocc, pdr, rniwa, schenney, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
rniwa: review+, webkit.review.bot: commit-queue-
patch2 none

Description Antti Koivisto 2012-12-11 10:41:27 PST
In many cases where we use NodeTraversal we really only want Elements. By having Element specific iteration functions we can tighten the code and make it faster too.
Comment 1 Antti Koivisto 2012-12-11 14:44:07 PST
Created attachment 178882 [details]
patch
Comment 2 Ryosuke Niwa 2012-12-11 15:02:32 PST
Comment on attachment 178882 [details]
patch

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

> Source/WebCore/ChangeLog:1529
> +        Another build fix.
> +
> +        * html/HTMLPropertiesCollection.cpp:
> +        (WebCore::nextNodeWithProperty):

Please revert these changes.

> Source/WebCore/dom/ContainerNodeAlgorithms.h:319
> +    Element* element = (shouldIncludeRoot == IncludeRoot && root->isElementNode()) ? static_cast<Element*>(root) : ElementTraversal::firstWithin(root);

I don't have toElement?

> Source/WebCore/dom/NodeTraversal.h:102
> +template <class NodeType>
> +inline Element* traverseNextElementTemplate(NodeType* current, const Node* stayWithin)
> +{
> +    Node* node = NodeTraversal::next(current, stayWithin);
> +    while (node && !node->isElementNode())
> +        node = NodeTraversal::nextSkippingChildren(node, stayWithin);
> +    return static_cast<Element*>(node);
> +}

I'm sad that we have to duplicate the template like this. Can we had a boolean template argument and share the code between two?

> Source/WebCore/dom/NodeTraversal.h:126
> +template <class NodeType>
> +inline Element* traverseNextElementSkippingChildrenTemplate(NodeType* current, const Node* stayWithin)
> +{
> +    Node* node = NodeTraversal::nextSkippingChildren(current, stayWithin);
> +    while (node && !node->isElementNode())
> +        node = NodeTraversal::nextSkippingChildren(node, stayWithin);
> +    return static_cast<Element*>(node);
> +}

Ditto.
Comment 3 WebKit Review Bot 2012-12-11 15:10:51 PST
Comment on attachment 178882 [details]
patch

Attachment 178882 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15277319
Comment 4 Antti Koivisto 2012-12-11 15:23:27 PST
Created attachment 178892 [details]
patch2
Comment 5 Antti Koivisto 2012-12-11 15:24:56 PST
(In reply to comment #2)
> I'm sad that we have to duplicate the template like this. Can we had a boolean template argument and share the code between two?

I didn't do this as the templates get pretty messy and hard to understand if we also want optimal code.
Comment 6 Antti Koivisto 2012-12-11 19:15:06 PST
http://trac.webkit.org/changeset/137406