[webkit-reviews] review granted: [Bug 38231] crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre : [Attachment 55814] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Tony Chang (Google)
<tony at chromium.org>'s request for review:
Bug 38231: crash in WebCore::CompositeEditCommand::splitTreeToNode when
indenting pre
https://bugs.webkit.org/show_bug.cgi?id=38231

Attachment 55814: Patch
https://bugs.webkit.org/attachment.cgi?id=55814&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.