From: darin@apple.com Date: Tue, 17 Mar 2009 23:34:54 +0000 (+0000) Subject: 2009-03-17 Darin Adler X-Git-Url: http://git.webkit.org/?p=WebKit-https.git;a=commitdiff_plain;h=8a1b0dd9dc5d230574200aba2754e8ca88787be3;ds=sidebyside 2009-03-17 Darin Adler 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 --- a/WebCore/ChangeLog +++ b/WebCore/ChangeLog @@ -1,3 +1,37 @@ +2009-03-17 Darin Adler + + 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 Reviewed by Eric Seidel. diff --git a/WebCore/editing/DeleteButtonController.cpp b/WebCore/editing/DeleteButtonController.cpp index f35ae79..c0775e3 100644 --- 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 @@ -284,8 +284,13 @@ void DeleteButtonController::enable() 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())); + } } void DeleteButtonController::disable() diff --git a/WebCore/editing/EditCommand.cpp b/WebCore/editing/EditCommand.cpp index 4507c5f..fefe658 100644 --- 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()); - 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); } diff --git a/WebCore/editing/Editor.cpp b/WebCore/editing/Editor.cpp index ef29747..0d45592 100644 --- a/WebCore/editing/Editor.cpp +++ b/WebCore/editing/Editor.cpp @@ -2224,7 +2224,7 @@ PassRefPtr Editor::rangeForPoint(const IntPoint& windowPoint) 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() diff --git a/WebCore/editing/markup.cpp b/WebCore/editing/markup.cpp index 1137f8d..7c84e8f 100644 --- 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 @@ -438,7 +438,7 @@ static void appendStartMarkup(Vector& result, const Node *node, const Ran case Node::ELEMENT_NODE: { result.append('<'); const Element* el = static_cast(node); - bool convert = convertBlocksToInlines & isBlock(const_cast(node)); + bool convert = convertBlocksToInlines && isBlock(const_cast(node)); 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); } -static void appendMarkup(Vector& result, Node* startNode, bool onlyIncludeChildren, Vector* nodes, const HashMap* namespaces = 0) +class MarkupAccumulator { +public: + MarkupAccumulator(Node* nodeToSkip, Vector* nodes) + : m_nodeToSkip(nodeToSkip) + , m_nodes(nodes) + { + } + + void appendMarkup(Node* startNode, EChildrenOnly, const HashMap* namespaces = 0); + + String takeResult() { return String::adopt(m_result); } + +private: + Vector m_result; + Node* m_nodeToSkip; + Vector* m_nodes; +}; + +// FIXME: Would be nice to do this in a non-recursive way. +void MarkupAccumulator::appendMarkup(Node* startNode, EChildrenOnly childrenOnly, const HashMap* namespaces) { + if (startNode == m_nodeToSkip) + return; + HashMap 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()) - 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) @@ -1017,32 +1041,21 @@ PassRefPtr createFragmentFromMarkup(Document* document, const return fragment.release(); } -String createMarkup(const Node* node, EChildrenOnly includeChildren, Vector* nodes) +String createMarkup(const Node* node, EChildrenOnly childrenOnly, Vector* nodes) { - Vector result; - 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 ""; - deleteButton->disable(); } - document->updateLayoutIgnorePendingStylesheets(); - - appendMarkup(result, const_cast(node), includeChildren, nodes); - - if (deleteButton) - deleteButton->enable(); - - return String::adopt(result); + MarkupAccumulator accumulator(deleteButtonContainerElement, nodes); + accumulator.appendMarkup(const_cast(node), childrenOnly); + return accumulator.takeResult(); } static void fillContainerFromString(ContainerNode* paragraph, const String& string)