<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[200690] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/200690">200690</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2016-05-11 09:25:53 -0700 (Wed, 11 May 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Optimize DataDetection's searchForLinkRemovingExistingDDLinks()
https://bugs.webkit.org/show_bug.cgi?id=157561
Reviewed by Ryosuke Niwa.
Optimize DataDetection's searchForLinkRemovingExistingDDLinks():
1. The first loop was using Node::childNodes() to iterate over the child
elements. Because of the recursive call, we may end up prepending
children as we iterate over them. This is an issue because the
childCount was cached before the loop and vector it would blow away
the cache inside the NodeList. Switch to using ElementTraversal which
should be both safer and more efficient. I don't believe we can use
our nice ElementChildIterator here unfortunately as we would hit
assertions due the the DOM mutations while iterating.
2. The second loop was using again Node::childNodes() and kept calling
item(0) to get the first child and move it to make it the previous
sibling of its old parent. Again, using childNodes() here is super
inefficient because we keep modifying the children and childNodes()
returns a live NodeList. The call to NodeList::length() would cache
all the children in a Vector, only to blow that cache away after
removing the first child. Switch to using ContainerNode::firstChild().
3. Drop the parentElement parameter as we can just get it from the child.
4. Use tighter typing so we don't end up calling the implementations of
insertBefore() / removeChild() that are on Node, thus unnecessarily
doing a is<ContainerNode>() check every time.
5. Pass element by reference instead of pointer as it cannot be null.
* editing/cocoa/DataDetection.mm:
(WebCore::removeResultLinksFromAnchor):
(WebCore::searchForLinkRemovingExistingDDLinks):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreeditingcocoaDataDetectionmm">trunk/Source/WebCore/editing/cocoa/DataDetection.mm</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (200689 => 200690)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2016-05-11 16:24:31 UTC (rev 200689)
+++ trunk/Source/WebCore/ChangeLog 2016-05-11 16:25:53 UTC (rev 200690)
</span><span class="lines">@@ -1,3 +1,36 @@
</span><ins>+2016-05-11 Chris Dumez <cdumez@apple.com>
+
+ Optimize DataDetection's searchForLinkRemovingExistingDDLinks()
+ https://bugs.webkit.org/show_bug.cgi?id=157561
+
+ Reviewed by Ryosuke Niwa.
+
+ Optimize DataDetection's searchForLinkRemovingExistingDDLinks():
+ 1. The first loop was using Node::childNodes() to iterate over the child
+ elements. Because of the recursive call, we may end up prepending
+ children as we iterate over them. This is an issue because the
+ childCount was cached before the loop and vector it would blow away
+ the cache inside the NodeList. Switch to using ElementTraversal which
+ should be both safer and more efficient. I don't believe we can use
+ our nice ElementChildIterator here unfortunately as we would hit
+ assertions due the the DOM mutations while iterating.
+ 2. The second loop was using again Node::childNodes() and kept calling
+ item(0) to get the first child and move it to make it the previous
+ sibling of its old parent. Again, using childNodes() here is super
+ inefficient because we keep modifying the children and childNodes()
+ returns a live NodeList. The call to NodeList::length() would cache
+ all the children in a Vector, only to blow that cache away after
+ removing the first child. Switch to using ContainerNode::firstChild().
+ 3. Drop the parentElement parameter as we can just get it from the child.
+ 4. Use tighter typing so we don't end up calling the implementations of
+ insertBefore() / removeChild() that are on Node, thus unnecessarily
+ doing a is<ContainerNode>() check every time.
+ 5. Pass element by reference instead of pointer as it cannot be null.
+
+ * editing/cocoa/DataDetection.mm:
+ (WebCore::removeResultLinksFromAnchor):
+ (WebCore::searchForLinkRemovingExistingDDLinks):
+
</ins><span class="cx"> 2016-05-11 Rawinder Singh <rawinder.singh-webkit@cisra.canon.com.au>
</span><span class="cx">
</span><span class="cx"> preprocess-idls.pl not ignoring comments during processing
</span></span></pre></div>
<a id="trunkSourceWebCoreeditingcocoaDataDetectionmm"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/editing/cocoa/DataDetection.mm (200689 => 200690)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/editing/cocoa/DataDetection.mm 2016-05-11 16:24:31 UTC (rev 200689)
+++ trunk/Source/WebCore/editing/cocoa/DataDetection.mm 2016-05-11 16:25:53 UTC (rev 200690)
</span><span class="lines">@@ -29,6 +29,7 @@
</span><span class="cx"> #import "Attr.h"
</span><span class="cx"> #import "CSSStyleDeclaration.h"
</span><span class="cx"> #import "DataDetectorsSPI.h"
</span><ins>+#import "ElementTraversal.h"
</ins><span class="cx"> #import "FrameView.h"
</span><span class="cx"> #import "HTMLAnchorElement.h"
</span><span class="cx"> #import "HTMLNames.h"
</span><span class="lines">@@ -248,37 +249,28 @@
</span><span class="cx"> return nil;
</span><span class="cx"> }
</span><span class="cx">
</span><del>-static void removeResultLinksFromAnchor(Node* node, Node* nodeParent)
</del><ins>+static void removeResultLinksFromAnchor(Element& element)
</ins><span class="cx"> {
</span><span class="cx"> // Perform a depth-first search for anchor nodes, which have the DDURLScheme attribute set to true,
</span><del>- // take their children and insert them before the anchor,
- // and then remove the anchor.
-
- if (!node)
</del><ins>+ // take their children and insert them before the anchor, and then remove the anchor.
+
+ // Note that this is not using ElementChildIterator because we potentially prepend children as we iterate over them.
+ for (auto* child = ElementTraversal::firstChild(element); child; child = ElementTraversal::nextSibling(*child))
+ removeResultLinksFromAnchor(*child);
+
+ auto* elementParent = element.parentElement();
+ if (!elementParent)
</ins><span class="cx"> return;
</span><span class="cx">
</span><del>- BOOL nodeIsDDAnchor = is<HTMLAnchorElement>(*node) && equalIgnoringASCIICase(downcast<Element>(*node).fastGetAttribute(x_apple_data_detectorsAttr), "true");
-
- RefPtr<NodeList> children = node->childNodes();
- unsigned childCount = children->length();
- for (size_t i = 0; i < childCount; i++) {
- Node* child = children->item(i);
- if (is<Element>(*child))
- removeResultLinksFromAnchor(child, node);
- }
-
- if (nodeIsDDAnchor && nodeParent) {
- children = node->childNodes();
- childCount = children->length();
-
- // Iterate over the children and move them all onto the same level as this anchor.
- // Remove the anchor afterwards.
- for (size_t i = 0; i < childCount; i++) {
- Node* child = children->item(0);
- nodeParent->insertBefore(child, node, ASSERT_NO_EXCEPTION);
- }
- nodeParent->removeChild(node, ASSERT_NO_EXCEPTION);
- }
</del><ins>+ bool elementIsDDAnchor = is<HTMLAnchorElement>(element) && equalIgnoringASCIICase(element.fastGetAttribute(x_apple_data_detectorsAttr), "true");
+ if (!elementIsDDAnchor)
+ return;
+
+ // Iterate over the children and move them all onto the same level as this anchor. Remove the anchor afterwards.
+ while (auto* child = element.firstChild())
+ elementParent->insertBefore(*child, &element);
+
+ elementParent->removeChild(element);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> static bool searchForLinkRemovingExistingDDLinks(Node& startNode, Node& endNode, bool& didModifyDOM)
</span><span class="lines">@@ -287,9 +279,10 @@
</span><span class="cx"> Node* node = &startNode;
</span><span class="cx"> while (node) {
</span><span class="cx"> if (is<HTMLAnchorElement>(*node)) {
</span><del>- if (!equalIgnoringASCIICase(downcast<Element>(*node).fastGetAttribute(x_apple_data_detectorsAttr), "true"))
</del><ins>+ auto& anchor = downcast<HTMLAnchorElement>(*node);
+ if (!equalIgnoringASCIICase(anchor.fastGetAttribute(x_apple_data_detectorsAttr), "true"))
</ins><span class="cx"> return true;
</span><del>- removeResultLinksFromAnchor(node, node->parentElement());
</del><ins>+ removeResultLinksFromAnchor(anchor);
</ins><span class="cx"> didModifyDOM = true;
</span><span class="cx"> }
</span><span class="cx">
</span><span class="lines">@@ -299,9 +292,10 @@
</span><span class="cx"> node = startNode.parentNode();
</span><span class="cx"> while (node) {
</span><span class="cx"> if (is<HTMLAnchorElement>(*node)) {
</span><del>- if (!equalIgnoringASCIICase(downcast<Element>(*node).fastGetAttribute(x_apple_data_detectorsAttr), "true"))
</del><ins>+ auto& anchor = downcast<HTMLAnchorElement>(*node);
+ if (!equalIgnoringASCIICase(anchor.fastGetAttribute(x_apple_data_detectorsAttr), "true"))
</ins><span class="cx"> return true;
</span><del>- removeResultLinksFromAnchor(node, node->parentElement());
</del><ins>+ removeResultLinksFromAnchor(anchor);
</ins><span class="cx"> didModifyDOM = true;
</span><span class="cx"> }
</span><span class="cx"> node = node->parentNode();
</span></span></pre>
</div>
</div>
</body>
</html>