2009-03-17 Darin Adler <darin@apple.com>

        Reviewed by David Hyatt.

        Bug 24517: REGRESSION (r41552): innerHTML does an updateLayout -- unneeded and can be slow
        https://bugs.webkit.org/show_bug.cgi?id=24517

        * editing/DeleteButtonController.cpp:
        (WebCore::DeleteButtonController::enable): Added a call to updateRendering, since
        determining whether to display the delete button involves style and updateRendering
        also updates style (should probably be named updateStyle, in fact). Not needed to fix
        this bug, but would have prevented the crash that led to this bug in the first place.

        * editing/EditCommand.cpp:
        (WebCore::EditCommand::EditCommand): Get rid of unneeded null check. All frames have
        delete button controllers.
        * editing/Editor.cpp:
        (WebCore::Editor::rangeForPoint): Ditto.

        * editing/markup.cpp:
        (WebCore::appendStartMarkup): Changed a "&" to a "&&" so that generating markup
        doesn't depend on renderers at all when the convertBlocksToInlines boolean is false.
        This allows us to omit the call to updateLayoutIgnorePendingStylesheets in the
        createMarkup function that's called by innerHTML.
        (WebCore::MarkupAccumulator::appendMarkup): Turned this into a class with a member
        function. Added a feature where the accumulator will skip a node. Moved arguments
        that don't change during recursion into an object. This function still is a bit
        inefficient, since it creates a new HashMap at every level as it recurses, but for now
        I did not tackle that. Also replaced the onlyIncludeChildren boolean with EChildrenOnly
        for consistency and clarity.
        (WebCore::createMarkup): Removed the call to updateLayoutIgnorePendingStylesheets.
        Instead of calling disable/enable on the delete button controller's container element,
        pass it in to the markup accumulator as a node to skip.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@41791 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index c1f1337..3e28624 100644 (file)
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,3 +1,37 @@
+2009-03-17  Darin Adler  <darin@apple.com>
+
+        Reviewed by David Hyatt.
+
+        Bug 24517: REGRESSION (r41552): innerHTML does an updateLayout -- unneeded and can be slow
+        https://bugs.webkit.org/show_bug.cgi?id=24517
+
+        * editing/DeleteButtonController.cpp:
+        (WebCore::DeleteButtonController::enable): Added a call to updateRendering, since
+        determining whether to display the delete button involves style and updateRendering
+        also updates style (should probably be named updateStyle, in fact). Not needed to fix
+        this bug, but would have prevented the crash that led to this bug in the first place.
+
+        * editing/EditCommand.cpp:
+        (WebCore::EditCommand::EditCommand): Get rid of unneeded null check. All frames have
+        delete button controllers.
+        * editing/Editor.cpp:
+        (WebCore::Editor::rangeForPoint): Ditto.
+
+        * editing/markup.cpp:
+        (WebCore::appendStartMarkup): Changed a "&" to a "&&" so that generating markup
+        doesn't depend on renderers at all when the convertBlocksToInlines boolean is false.
+        This allows us to omit the call to updateLayoutIgnorePendingStylesheets in the
+        createMarkup function that's called by innerHTML.
+        (WebCore::MarkupAccumulator::appendMarkup): Turned this into a class with a member
+        function. Added a feature where the accumulator will skip a node. Moved arguments
+        that don't change during recursion into an object. This function still is a bit
+        inefficient, since it creates a new HashMap at every level as it recurses, but for now
+        I did not tackle that. Also replaced the onlyIncludeChildren boolean with EChildrenOnly
+        for consistency and clarity.
+        (WebCore::createMarkup): Removed the call to updateLayoutIgnorePendingStylesheets.
+        Instead of calling disable/enable on the delete button controller's container element,
+        pass it in to the markup accumulator as a node to skip.
+

 2009-03-17  Scott Violet  <sky@google.com>
 
         Reviewed by Eric Seidel.
 2009-03-17  Scott Violet  <sky@google.com>
 
         Reviewed by Eric Seidel.

diff --git a/WebCore/editing/DeleteButtonController.cpp b/WebCore/editing/DeleteButtonController.cpp
index f35ae79..c0775e3 100644 (file)
--- a/WebCore/editing/DeleteButtonController.cpp
+++ b/WebCore/editing/DeleteButtonController.cpp
@@ -1,5 +1,5 @@
 /*
 /*

- * Copyright (C) 2006, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2006, 2008, 2009 Apple Inc. All rights reserved.

  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions


@@ -284,8 +284,13 @@ void DeleteButtonController::enable()
     ASSERT(m_disableStack > 0);
     if (m_disableStack > 0)
         m_disableStack--;
     ASSERT(m_disableStack > 0);
     if (m_disableStack > 0)
         m_disableStack--;

-    if (enabled())
+    if (enabled()) {
+        // Determining if the element is deletable currently depends on style
+        // because whether something is editable depends on style, so we need
+        // to recalculate style before calling enclosingDeletableElement.
+        m_frame->document()->updateRendering();

         show(enclosingDeletableElement(m_frame->selection()->selection()));
         show(enclosingDeletableElement(m_frame->selection()->selection()));

+    }

 }
 
 void DeleteButtonController::disable()
 }
 
 void DeleteButtonController::disable()

diff --git a/WebCore/editing/EditCommand.cpp b/WebCore/editing/EditCommand.cpp
index 4507c5f..fefe658 100644 (file)
--- a/WebCore/editing/EditCommand.cpp
+++ b/WebCore/editing/EditCommand.cpp
@@ -47,8 +47,7 @@ EditCommand::EditCommand(Document* document)
 {
     ASSERT(m_document);
     ASSERT(m_document->frame());
 {
     ASSERT(m_document);
     ASSERT(m_document->frame());

-    DeleteButtonController* deleteButton = m_document->frame()->editor()->deleteButtonController();
-    setStartingSelection(avoidIntersectionWithNode(m_document->frame()->selection()->selection(), deleteButton ? deleteButton->containerElement() : 0));
+    setStartingSelection(avoidIntersectionWithNode(m_document->frame()->selection()->selection(), m_document->frame()->editor()->deleteButtonController()->containerElement()));

     setEndingSelection(m_startingSelection);
 }
 
     setEndingSelection(m_startingSelection);
 }
 

diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp
index ef29747..0d45592 100644 (file)
--- a/WebCore/editing/Editor.cpp
+++ b/WebCore/editing/Editor.cpp
@@ -2224,7 +2224,7 @@ PassRefPtr<Range> Editor::rangeForPoint(const IntPoint& windowPoint)
         return 0;
     IntPoint framePoint = frameView->windowToContents(windowPoint);
     VisibleSelection selection(frame->visiblePositionForPoint(framePoint));
         return 0;
     IntPoint framePoint = frameView->windowToContents(windowPoint);
     VisibleSelection selection(frame->visiblePositionForPoint(framePoint));

-    return avoidIntersectionWithNode(selection.toNormalizedRange().get(), deleteButtonController() ? deleteButtonController()->containerElement() : 0);
+    return avoidIntersectionWithNode(selection.toNormalizedRange().get(), m_deleteButtonController->containerElement());

 }
 
 void Editor::revealSelectionAfterEditingOperation()
 }
 
 void Editor::revealSelectionAfterEditingOperation()

diff --git a/WebCore/editing/markup.cpp b/WebCore/editing/markup.cpp
index 1137f8d..7c84e8f 100644 (file)
--- a/WebCore/editing/markup.cpp
+++ b/WebCore/editing/markup.cpp
@@ -1,5 +1,5 @@
 /*
 /*

- * Copyright (C) 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
+ * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions


@@ -438,7 +438,7 @@ static void appendStartMarkup(Vector<UChar>& result, const Node *node, const Ran
         case Node::ELEMENT_NODE: {
             result.append('<');
             const Element* el = static_cast<const Element*>(node);
         case Node::ELEMENT_NODE: {
             result.append('<');
             const Element* el = static_cast<const Element*>(node);

-            bool convert = convertBlocksToInlines & isBlock(const_cast<Node*>(node));
+            bool convert = convertBlocksToInlines && isBlock(const_cast<Node*>(node));

             append(result, el->nodeNamePreservingCase());
             NamedAttrMap *attrs = el->attributes();
             unsigned length = attrs->length();
             append(result, el->nodeNamePreservingCase());
             NamedAttrMap *attrs = el->attributes();
             unsigned length = attrs->length();


@@ -587,26 +587,50 @@ static String getEndMarkup(const Node *node)
     return String::adopt(result);
 }
 
     return String::adopt(result);
 }
 

-static void appendMarkup(Vector<UChar>& result, Node* startNode, bool onlyIncludeChildren, Vector<Node*>* nodes, const HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces = 0)
+class MarkupAccumulator {
+public:
+    MarkupAccumulator(Node* nodeToSkip, Vector<Node*>* nodes)
+        : m_nodeToSkip(nodeToSkip)
+        , m_nodes(nodes)
+    {
+    }
+
+    void appendMarkup(Node* startNode, EChildrenOnly, const HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces = 0);
+
+    String takeResult() { return String::adopt(m_result); }
+
+private:
+    Vector<UChar> m_result;
+    Node* m_nodeToSkip;
+    Vector<Node*>* m_nodes;
+};
+
+// FIXME: Would be nice to do this in a non-recursive way.
+void MarkupAccumulator::appendMarkup(Node* startNode, EChildrenOnly childrenOnly, const HashMap<AtomicStringImpl*, AtomicStringImpl*>* namespaces)

 {
 {

+    if (startNode == m_nodeToSkip)
+        return;
+

     HashMap<AtomicStringImpl*, AtomicStringImpl*> namespaceHash;
     if (namespaces)
         namespaceHash = *namespaces;
     HashMap<AtomicStringImpl*, AtomicStringImpl*> namespaceHash;
     if (namespaces)
         namespaceHash = *namespaces;

-    
-    if (!onlyIncludeChildren) {
-        if (nodes)
-            nodes->append(startNode);
-        
-        appendStartMarkup(result,startNode, 0, DoNotAnnotateForInterchange, false, &namespaceHash);
+
+    // start tag
+    if (!childrenOnly) {
+        if (m_nodes)
+            m_nodes->append(startNode);
+        appendStartMarkup(m_result, startNode, 0, DoNotAnnotateForInterchange, false, &namespaceHash);

     }
     }

-    // print children
-    if (!(startNode->document()->isHTMLDocument() && doesHTMLForbidEndTag(startNode)))
+
+    // children
+    if (!(startNode->document()->isHTMLDocument() && doesHTMLForbidEndTag(startNode))) {

         for (Node* current = startNode->firstChild(); current; current = current->nextSibling())
         for (Node* current = startNode->firstChild(); current; current = current->nextSibling())

-            appendMarkup(result, current, false, nodes, &namespaceHash);
-    
-    // Print my ending tag
-    if (!onlyIncludeChildren)
-        appendEndMarkup(result, startNode);
+            appendMarkup(current, IncludeNode, &namespaceHash);
+    }
+
+    // end tag
+    if (!childrenOnly)
+        appendEndMarkup(m_result, startNode);

 }
 
 static void completeURLs(Node* node, const String& baseURL)
 }
 
 static void completeURLs(Node* node, const String& baseURL)


@@ -1017,32 +1041,21 @@ PassRefPtr<DocumentFragment> createFragmentFromMarkup(Document* document, const
     return fragment.release();
 }
 
     return fragment.release();
 }
 

-String createMarkup(const Node* node, EChildrenOnly includeChildren, Vector<Node*>* nodes)
+String createMarkup(const Node* node, EChildrenOnly childrenOnly, Vector<Node*>* nodes)

 {
 {

-    Vector<UChar> result;
-

     if (!node)
         return "";
 
     if (!node)
         return "";
 

-    Document* document = node->document();
-    Frame* frame = document->frame();
-    DeleteButtonController* deleteButton = frame ? frame->editor()->deleteButtonController() : 0;
-
-    // disable the delete button so it's elements are not serialized into the markup
-    if (deleteButton) {
-        if (node->isDescendantOf(deleteButton->containerElement()))
+    HTMLElement* deleteButtonContainerElement = 0;
+    if (Frame* frame = node->document()->frame()) {
+        deleteButtonContainerElement = frame->editor()->deleteButtonController()->containerElement();
+        if (node->isDescendantOf(deleteButtonContainerElement))

             return "";
             return "";

-        deleteButton->disable();

     }
 
     }
 

-    document->updateLayoutIgnorePendingStylesheets();
-
-    appendMarkup(result, const_cast<Node*>(node), includeChildren, nodes);
-
-    if (deleteButton)
-        deleteButton->enable();
-
-    return String::adopt(result);
+    MarkupAccumulator accumulator(deleteButtonContainerElement, nodes);
+    accumulator.appendMarkup(const_cast<Node*>(node), childrenOnly);
+    return accumulator.takeResult();

 }
 
 static void fillContainerFromString(ContainerNode* paragraph, const String& string)
 }
 
 static void fillContainerFromString(ContainerNode* paragraph, const String& string)