[Webkit-unassigned] [Bug 38231] crash in WebCore::CompositeEditCommand::splitTreeToNode when indenting pre

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38231


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55814|review?                     |review+
               Flag|                            |




--- Comment #14 from Ojan Vafai <ojan at chromium.org>  2010-07-07 11:03:42 PST ---
(From update of attachment 55814)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.