Bug 38231 – crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

Happens as far back as r31256 (oldest chromium archive). The attached test case demonstrates the crash.

See http://crbug.com/42667 for more details (was hit in gmail).

[Tony Chang]

Can you try attaching the test case again?  Thanks.

[Ojan Vafai]

Sorry, don't know how that didn't attach. Anyways, it's attached in the crbug.com link.

[Tony Chang]

Created attachment 54526 [details]
test case

[Tony Chang]

Created attachment 54532 [details]
Patch

[Tony Chang]

Here's a test case with a closed pre.  I've also posted a patch, but it's wrong.  It's just showing some of the things I tried.

There are two problems.  First is that CompositeEditCommand::cloneParagraphUnderNewElement expects paragraphs to map to nodes.  However, with <pre>, paragraphs can be strings of text.  For example, in the test case, we have:
<pre>a
b

</pre>

a and b are both separate paragraphs.  However, when we try to clone the paragraphs, we clone the whole pre node.  I tried modifying cloneParagraphUnderNewElement to handle the case where we're only cloning part of a node (see patch).

The second problem is in IndentOutdentCommand::indentRegion.  It has similar logic that keeps track of the next paragraph to indent.  Unfortunately, after the first indent, |endOfNextParagraph| is pointing to the wrong place since we just removed text from the <pre> node causing the node offset to be wrong.  I tried to fix that in the patch as well, but it doesn't properly update |endAfterSelection|.

This seems very fragile, so maybe someone has some better ideas.  For example, maybe rather than trying to indent each paragraph, we would try to do a single indent if everything is in a single node.  Or maybe we can split the pre into multiple pre tags before we start moving paragraphs.  Thoughts?

[Ojan Vafai]

Comment on attachment 54532 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index a91efb68da9a3f83287d760013e02df18a64a864..4a0842fed9d643c7183cb1aaa31b179f52965110 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> +        No new tests. (OOPS!)

Add test?

> +++ b/WebCore/editing/CompositeEditCommand.cpp
> @@ -754,7 +754,15 @@ void CompositeEditCommand::cloneParagraphUnderNewElement(Position& start, Positi
>          Vector<RefPtr<Node> > ancestors;
>          
>          // Insert each node from innerNode to outerNode (excluded) in a list.
> -        for (Node* n = start.node(); n && n != outerNode; n = n->parentNode())
> +        Node* n = start.node();
> +        if (start.node() == end.node()) {
> +            ASSERT(start.node()->isTextNode());
> +            String text = plainText(Range::create(document(), start, end).get());
> +            ancestors.append(document()->createTextNode(text));
> +            n = n->parentNode();
> +        }

I don't really get this. What does creating a new text node fix? Maybe this deserves a comment?

> +++ b/WebCore/editing/IndentOutdentCommand.cpp
> @@ -154,11 +154,17 @@ void IndentOutdentCommand::indentRegion(const VisiblePosition& startOfSelection,

Maybe this deserves a comment about why this case needs to recompute positions?

> +        if (recomputePositions)
> +            endOfNextParagraph = endOfParagraph(startOfCurrentParagraph);

Does startOfCurrentParagraph exist?

[Ojan Vafai]

(In reply to comment #6)
> I don't really get this. What does creating a new text node fix? Maybe this
> deserves a comment?

nm. I hadn't seen comment #5 before looking at the code. I get it now.

[Ryosuke Niwa]

(In reply to comment #5)
> This seems very fragile, so maybe someone has some better ideas.  For example,
> maybe rather than trying to indent each paragraph, we would try to do a single
> indent if everything is in a single node.  Or maybe we can split the pre into
> multiple pre tags before we start moving paragraphs.  Thoughts?

In my broken fix for indentation, I did that.  In particular extendRangeToWrappingNodes in htmlediting.cpp extends the range as much as possible.  See http://trac.webkit.org/changeset/46142 for usage.

> -        for (Node* n = start.node(); n && n != outerNode; n = n->parentNode())
> +        Node* n = start.node();
> +        if (start.node() == end.node()) {
> +            ASSERT(start.node()->isTextNode());
> +            String text = plainText(Range::create(document(), start, end).get());
> +            ancestors.append(document()->createTextNode(text));
> +            n = n->parentNode();
> +        }
> +
> +        for (; n && n != outerNode; n = n->parentNode())

I'm not fully convinced that when start.node() == end.node(), it should be a text node. And also I'm not a fun to making cloneParagraphUnderNewElement smarter unless cloneParagraph is doing something unexpected.

[Tony Chang]

Created attachment 55814 [details]
Patch

[Tony Chang]

(In reply to comment #9)
> Created an attachment (id=55814) [details]
> Patch

Here's another attempt at fixing the crash.  The patch splits text nodes along paragraph boundaries before trying to indent.  This prevents the crash, although there are a few cases (e.g., when tables are involved) that we do the wrong thing.

[Tony Chang]

(In reply to comment #8)
> In my broken fix for indentation, I did that.  In particular extendRangeToWrappingNodes in htmlediting.cpp extends the range as much as possible.  See http://trac.webkit.org/changeset/46142 for usage.

Oh, so I tried this, but I wasn't able to use extendRangeToWrappingNodes because there are cases where we would still be within a single text node.  For example:

<pre>
one
two
three
</pre>

If you want to indent only "two", extendRangeToWrappingNodes would end up including "one" and "three" as well.

[Adam Barth]

Comment on attachment 55814 [details]
Patch

@Ojan: This patch is calling out for your review!

[Enrica Casucci]

Comment on attachment 55814 [details]
Patch

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 4dc9e8e05791fadb3e720b13f0e8e89db4975aa3..00771643fc6e56ee7c894d53a5a71e543806c352 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre
> +        https://bugs.webkit.org/show_bug.cgi?id=38231
> +
> +        * editing/execCommand/indent-pre-expected.txt: Added.
> +        * editing/execCommand/indent-pre.html: Added.
> +
>  2010-05-11  Tony Chang  <[e-mail]>
>  
>          Reviewed by Darin Fisher.
> diff --git a/LayoutTests/editing/execCommand/indent-pre-expected.txt b/LayoutTests/editing/execCommand/indent-pre-expected.txt
> new file mode 100644
> index 0000000000000000000000000000000000000000..883a704e8bd4e99bba9300fe30813c566363a586
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/indent-pre-expected.txt
> @@ -0,0 +1,179 @@
> +
> +<HTML>
> +<HEAD>
> +</HEAD>
> +<BODY>
> +<DIV contentEditable="">
> +<#text>
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-basic">
> +<#text>line one
> +</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +<PRE id="pre-basic">
> +<#text>line two
> +</#text>
> +</PRE>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-basic">
> +<#text>line three
> +</#text>
> +</PRE>
> +<PRE id="pre-basic">
> +<#text>line four</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +<#text>
> +
> +</#text>
> +<UL>
> +<LI>
> +<PRE id="pre-list">
> +<#text>list one
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<#text>list two
> +</#text>
> +<#text>list three
> +</#text>
> +</BLOCKQUOTE>
> +<#text>list four
> +</#text>
> +</PRE>
> +</LI>
> +</UL>
> +<#text>
> +
> +</#text>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<TABLE>
> +<#text>
> +</#text>
> +<TBODY>
> +<TR>
> +<TD>
> +<PRE id="pre-table">
> +<#text>table one
> +</#text>
> +<#text><selection-anchor>table two
> +</#text>
> +</PRE>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<PRE id="pre-table">
> +<#text>table three<selection-focus></#text>
> +</PRE>
> +<PRE id="pre-table">
> +<#text>table three</#text>
> +</PRE>
> +</BLOCKQUOTE>
> +</TD>
> +<TD>
> +<BLOCKQUOTE style="margin: 0 0 0 40px; border: none; padding: 0px;" class="webkit-indent-blockquote">
> +<#text>right cell</#text>
> +</BLOCKQUOTE>
> +</TD>
> +</TR>
> +</TBODY>
> +</TABLE>
> +<DIV id="results">
> +<#text>PASSED</#text>
> +</DIV>
> +</BLOCKQUOTE>
> +<#text>
> +
> +</#text>
> +<#text>
> +</#text>
> +</DIV>
> +<#text>
> +
> +</#text>
> +<A href="javascript:document.execCommand('indent')">
> +<#text>indent</#text>
> +</A>
> +<#text>
> +</#text>
> +<A href="javascript:document.execCommand('outdent')">
> +<#text>outdent</#text>
> +</A>
> +<#text>
> +</#text>
> +<SCRIPT src="../../resources/dump-as-markup.js"></SCRIPT>
> +<#text>
> +</#text>
> +<SCRIPT src="../editing.js"></SCRIPT>
> +<#text>
> +</#text>
> +<SCRIPT>
> +function setSelection(node, startOffset, endOffset)
> +{
> +var textNode = node.firstChild;
> +if (textNode.nodeType != Node.TEXT_NODE)
> +throw "Wrong node type: " + textNode;
> +execSetSelectionCommand(textNode, startOffset, endOffset);
> +}
> +
> +function verifyTextSelection(startNode, startOffset, endNode, endOffset)
> +{
> +if (startNode.nodeType != Node.TEXT_NODE)
> +console.log("Wrong start node type: " + startNode);
> +if (endNode.nodeType != Node.TEXT_NODE)
> +console.log("Wrong end node type: " + endNode);
> +var sel = window.getSelection();
> +if (sel.anchorNode != startNode || sel.focusNode != endNode)
> +console.log("Wrong node selected.");
> +if (sel.anchorOffset != startOffset)
> +console.log("Wrong anchor offset: " + sel.anchorOffset + " instead of " + startOffset);
> +if (sel.focusOffset != endOffset)
> +console.log("Wrong focus offset: " + sel.focusOffset + " instead of " + endOffset);
> +}
> +
> +// Indent a single line in a pre and make sure the selection is correctly preserved.
> +var pre = document.getElementById("pre-basic");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByCharacterCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("pre")[0].firstChild, 1,
> +document.getElementsByTagName("pre")[0].firstChild, 4);
> +
> +// Indent 2 lines.
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +if (document.getElementsByTagName("pre").length > 3) {
> +// FIXME: The selection for the anchorNode is wrong. It should be (pre[2], 0).
> +verifyTextSelection(document.getElementsByTagName("pre")[1].firstChild, 8,
> +document.getElementsByTagName("pre")[3].firstChild, 4);
> +} else {
> +console.log("Wrong number of pre nodes.");
> +}
> +// Indent <pre> lines in a list.
> +pre = document.getElementById("pre-list");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("blockquote")[2].firstChild, 0,
> +document.getElementsByTagName("blockquote")[2].firstChild.nextSibling, 10);
> +
> +// Indenting <pre> lines in a table.
> +pre = document.getElementById("pre-table");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +// FIXME: This is wrong.
> +document.execCommand("indent");
> +
> +document.getElementById("results").innerText = "PASSED";
> +</SCRIPT>
> +<#text>
> +</#text>
> +</BODY>
> +</HTML>
> diff --git a/LayoutTests/editing/execCommand/indent-pre.html b/LayoutTests/editing/execCommand/indent-pre.html
> new file mode 100644
> index 0000000000000000000000000000000000000000..0641c844de9fdd8538d60461b5c1525798ae1f90
> --- /dev/null
> +++ b/LayoutTests/editing/execCommand/indent-pre.html
> @@ -0,0 +1,91 @@
> +<div contentEditable>
> +<pre id="pre-basic">line one
> +line two
> +line three
> +line four</pre>
> +
> +<ul><li><pre id="pre-list">list one
> +list two
> +list three
> +list four
> +</pre></li></ul>
> +
> +<table>
> +<tr><td><pre id="pre-table">table one
> +table two
> +table three</pre></td><td>right cell</td></tr></table>
> +
> +<div id="results">FAILED</div>
> +</div>
> +
> +<a href="javascript:document.execCommand('indent')">indent</a>
> +<a href="javascript:document.execCommand('outdent')">outdent</a>
> +<script src="../../resources/dump-as-markup.js"></script>
> +<script src="../editing.js"></script>
> +<script>
> +function setSelection(node, startOffset, endOffset)
> +{
> +    var textNode = node.firstChild;
> +    if (textNode.nodeType != Node.TEXT_NODE)
> +        throw "Wrong node type: " + textNode;
> +    execSetSelectionCommand(textNode, startOffset, endOffset);
> +}
> +
> +function verifyTextSelection(startNode, startOffset, endNode, endOffset)
> +{
> +    if (startNode.nodeType != Node.TEXT_NODE)
> +        console.log("Wrong start node type: " + startNode);
> +    if (endNode.nodeType != Node.TEXT_NODE)
> +        console.log("Wrong end node type: " + endNode);
> +    var sel = window.getSelection();
> +    if (sel.anchorNode != startNode || sel.focusNode != endNode)
> +        console.log("Wrong node selected.");
> +    if (sel.anchorOffset != startOffset)
> +        console.log("Wrong anchor offset: " + sel.anchorOffset + " instead of " + startOffset);
> +    if (sel.focusOffset != endOffset)
> +        console.log("Wrong focus offset: " + sel.focusOffset + " instead of " + endOffset);
> +}
> +
> +// Indent a single line in a pre and make sure the selection is correctly preserved.
> +var pre = document.getElementById("pre-basic");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByCharacterCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("pre")[0].firstChild, 1,
> +                    document.getElementsByTagName("pre")[0].firstChild, 4);
> +
> +// Indent 2 lines.
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByWordCommand();
> +document.execCommand("indent");
> +if (document.getElementsByTagName("pre").length > 3) {
> +    // FIXME: The selection for the anchorNode is wrong.  It should be (pre[2], 0).
> +    verifyTextSelection(document.getElementsByTagName("pre")[1].firstChild, 8,
> +                        document.getElementsByTagName("pre")[3].firstChild, 4);
> +} else {
> +    console.log("Wrong number of pre nodes.");
> +}
> +// Indent <pre> lines in a list.
> +pre = document.getElementById("pre-list");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +document.execCommand("indent");
> +verifyTextSelection(document.getElementsByTagName("blockquote")[2].firstChild, 0,
> +                    document.getElementsByTagName("blockquote")[2].firstChild.nextSibling, 10);
> +
> +// Indenting <pre> lines in a table.
> +pre = document.getElementById("pre-table");
> +setSelection(pre, 0, 0);
> +execMoveSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +execExtendSelectionForwardByLineCommand();
> +// FIXME: This is wrong.
> +document.execCommand("indent");
> +
> +document.getElementById("results").innerText = "PASSED";
> +</script>
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 482ee438dd8109f3a9b4b4105770931a21c640fc..266ea14b48c21c1f7d97320797651fc5f3f033bf 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,19 @@
> [phone]  Tony Chang  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre
> +        https://bugs.webkit.org/show_bug.cgi?id=38231
> +
> +        Test: editing/execCommand/indent-pre.html
> +
> +        * editing/IndentOutdentCommand.cpp:
> +        (WebCore::countParagraphs):
> +        (WebCore::IndentOutdentCommand::indentRegion): Split text nodes into one node per paragraph
> +                                                       so moveParagraph doesn't get confused.
> +        (WebCore::IndentOutdentCommand::splitTextNodes):
> +        * editing/IndentOutdentCommand.h:
> +
>  2010-05-11  David Hyatt  <[e-mail]>
>  
>          Reviewed by Maciej Stachowiak.
> diff --git a/WebCore/editing/IndentOutdentCommand.cpp b/WebCore/editing/IndentOutdentCommand.cpp
> index 0f3975b0c13f6e8e93e32e8c659ab3b77b463f39..b68527d044ee3af7d12d7d6f2b605fba4f912290 100644
> --- a/WebCore/editing/IndentOutdentCommand.cpp
> +++ b/WebCore/editing/IndentOutdentCommand.cpp
> @@ -34,6 +34,7 @@
>  #include "InsertListCommand.h"
>  #include "Range.h"
>  #include "SplitElementCommand.h"
> +#include "Text.h"
>  #include "TextIterator.h"
>  #include "htmlediting.h"
>  #include "visible_units.h"
> @@ -62,6 +63,22 @@ static bool isListOrIndentBlockquote(const Node* node)
>      return node && (node->hasTagName(ulTag) || node->hasTagName(olTag) || node->hasTagName(blockquoteTag));
>  }
>  
> +// This function can return -1 if we are unable to count the paragraphs between |start| and |end|.
> +static int countParagraphs(const VisiblePosition& start, const VisiblePosition& end)
> +{
> +    int count = 0;
> +    VisiblePosition cur = start;
> +    while (cur != end) {
> +        ++count;
> +        cur = endOfParagraph(cur.next());
> +        // If start is before a table and end is inside a table, we will never hit end because the
> +        // whole table is considered a single paragraph.
> +        if (cur.isNull())
> +            return -1;
> +    }
> +    return count;
> +}
> +
>  IndentOutdentCommand::IndentOutdentCommand(Document* document, EIndentType typeOfAction, int marginInPixels)
>      : CompositeEditCommand(document), m_typeOfAction(typeOfAction), m_marginInPixels(marginInPixels)
>  {
> @@ -151,6 +168,18 @@ void IndentOutdentCommand::indentRegion(const VisiblePosition& startOfSelection,
>      RefPtr<Element> blockquoteForNextIndent;
>      VisiblePosition endOfCurrentParagraph = endOfParagraph(startOfSelection);
>      VisiblePosition endAfterSelection = endOfParagraph(endOfParagraph(endOfSelection).next());
> +    int endOfCurrentParagraphIndex = indexForVisiblePosition(endOfCurrentParagraph);
> +    int endAfterSelectionIndex = indexForVisiblePosition(endAfterSelection);
> +
> +    // When indenting within a <pre> tag, we need to split each paragraph into a separate node for moveParagraphWithClones to work.
> +    // However, splitting text nodes can cause endOfCurrentParagraph and endAfterSelection to point to an invalid position if we
> +    // changed the text node it was pointing at.  So we have to reset these positions.
> +    int numParagraphs = countParagraphs(endOfCurrentParagraph, endAfterSelection);
> +    if (splitTextNodes(startOfParagraph(startOfSelection), numParagraphs + 1)) {
> +        endOfCurrentParagraph = VisiblePosition(TextIterator::rangeFromLocationAndLength(document()->documentElement(), endOfCurrentParagraphIndex, 0)->startPosition(), UPSTREAM);
> +        endAfterSelection = VisiblePosition(TextIterator::rangeFromLocationAndLength(document()->documentElement(), endAfterSelectionIndex, 0)->startPosition(), UPSTREAM);
> +    }
> +
>      while (endOfCurrentParagraph != endAfterSelection) {
>          // Iterate across the selected paragraphs...
>          VisiblePosition endOfNextParagraph = endOfParagraph(endOfCurrentParagraph.next());
> @@ -174,6 +203,28 @@ void IndentOutdentCommand::indentRegion(const VisiblePosition& startOfSelection,
>      }   
>  }
>  
> +// Returns true if at least one text node was split.
> +bool IndentOutdentCommand::splitTextNodes(const VisiblePosition& start, int numParagraphs)
> +{
> +    VisiblePosition currentParagraphStart = start;
> +    bool hasSplit = false;
> +    int paragraphCount;
> +    for (paragraphCount = 0; paragraphCount < numParagraphs; ++paragraphCount) {
> +        if (currentParagraphStart.deepEquivalent().node()->isTextNode() && currentParagraphStart.deepEquivalent().node() == startOfParagraph(currentParagraphStart.previous()).deepEquivalent().node()) {
> +            Text* textNode = static_cast<Text *>(currentParagraphStart.deepEquivalent().node());
> +            int offset = currentParagraphStart.deepEquivalent().offsetInContainerNode();
> +            splitTextNode(textNode, offset);
> +            currentParagraphStart = VisiblePosition(textNode, 0, VP_DEFAULT_AFFINITY);
> +            hasSplit = true;
> +        }
> +        VisiblePosition nextParagraph = startOfParagraph(endOfParagraph(currentParagraphStart).next());
> +        if (nextParagraph.isNull())
> +            break;
> +        currentParagraphStart = nextParagraph;
> +    }
> +    return hasSplit;
> +}
> +
>  void IndentOutdentCommand::outdentParagraph()
>  {
>      VisiblePosition visibleStartOfParagraph = startOfParagraph(endingSelection().visibleStart());
> diff --git a/WebCore/editing/IndentOutdentCommand.h b/WebCore/editing/IndentOutdentCommand.h
> index 8705bf103c22122298d1e8495dc007e9c2580911..8644cc56aa82083052e52e7a654a460fef3db964 100644
> --- a/WebCore/editing/IndentOutdentCommand.h
> +++ b/WebCore/editing/IndentOutdentCommand.h
> @@ -51,6 +51,7 @@ private:
>      void outdentParagraph();
>      bool tryIndentingAsListItem(const VisiblePosition&);
>      void indentIntoBlockquote(const VisiblePosition&, const VisiblePosition&, RefPtr<Element>&);
> +    bool splitTextNodes(const VisiblePosition& start, int numParagraphs);
>  
>      EIndentType m_typeOfAction;
>      int m_marginInPixels;

The patch looks good to me.

[Ojan Vafai]

Comment on attachment 55814 [details]
Patch

Please do the non-nits before committing.
---------------------------------
http://wkrietveld.appspot.com/38231/diff/1/4
File LayoutTests/editing/execCommand/indent-pre.html (right):

http://wkrietveld.appspot.com/38231/diff/1/4#newcode51
LayoutTests/editing/execCommand/indent-pre.html:51: setSelection(pre, 0, 0);
Nit: You always set the selection to the start of the node. May as well make a setSelection function that takes only the element to set the selection in.

http://wkrietveld.appspot.com/38231/diff/1/4#newcode66
LayoutTests/editing/execCommand/indent-pre.html:66: verifyTextSelection(document.getElementsByTagName("pre")[1].firstChild, 8,
I'd rather see this have the value is supposed to have and just have the test output output a failure for this line. That way, if someone accidentally fixes this, it'll be more clear looking at just the test output that they've improved it.

It's good to leave a FIXME in as well.

Also, is there a bug for this you can link to?

http://wkrietveld.appspot.com/38231/diff/1/4#newcode87
LayoutTests/editing/execCommand/indent-pre.html:87: // FIXME: This is wrong.
Can you do the same as I suggested above? Call verifyTextSelection with what the correct output should be? It's fine to have failures in the test output for cases where we have bugs to fix.

http://wkrietveld.appspot.com/38231/diff/1/6
File WebCore/editing/IndentOutdentCommand.cpp (right):

http://wkrietveld.appspot.com/38231/diff/1/6#newcode67
WebCore/editing/IndentOutdentCommand.cpp:67: static int countParagraphs(const VisiblePosition& start, const VisiblePosition& end)
This variable names should be changed to show that they are VisiblePositions that are at the end of paragraphs. Otherwise, the condition in the while loop below makes no sense.

http://wkrietveld.appspot.com/38231/diff/1/6#newcode209
WebCore/editing/IndentOutdentCommand.cpp:209: if (currentParagraphStart.deepEquivalent().node()->isTextNode() && currentParagraphStart.deepEquivalent().node() == startOfParagraph(currentParagraphStart.previous()).deepEquivalent().node()) {
Nit: It took me a while to figure out that this is restricting to cases where we have multiple paragraphs across a single text node. A short comment explaining that might be good.

[Tony Chang]

Created attachment 61121 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 61121 [details]
Patch for landing

Clearing flags on attachment: 61121

Committed r63039: <http://trac.webkit.org/changeset/63039>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.