[Webkit-unassigned] [Bug 47233] REGRESSION: Indenting pre duplicates content

[bugzilla-daemon at webkit.org]

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


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #70134|review?                     |review-
               Flag|                            |




--- Comment #6 from Tony Chang <tony at chromium.org>  2010-10-07 15:32:05 PST ---
(From update of attachment 70134)
View in context: https://bugs.webkit.org/attachment.cgi?id=70134&action=review

Overall, this looks great.  Using explicit start and end positions is a lot better than passing in the end of a paragraph and using prev/next paragraph.  r- because of function naming.

> WebCore/ChangeLog:22
> +        (WebCore::ApplyBlockElementCommand::rangeForParagraphSplitingTextNodesIfNeeded): Added.

Nit: Splitting is spelled wrong.

> WebCore/editing/ApplyBlockElementCommand.cpp:151
> +static UChar charAtPosition(const Position& position)

Do you plan on check for a char other than '\n'?  If not, you could simplify the callers by just making this function isNewlineCharAtPosition.

> WebCore/editing/ApplyBlockElementCommand.cpp:213
> +        // If start is in the middle of a text node, split.

Nit: If *end* is . . .

> LayoutTests/editing/execCommand/indent-pre-expected.txt:4
> +CONSOLE MESSAGE: line 41: Wrong end node type: [object HTMLBRElement]
> +CONSOLE MESSAGE: line 44: Wrong node selected.

Is this change intentional?

Would extra tests with <pre> and <table> be useful at this point?  Maybe it should just be a follow up patch.

> LayoutTests/editing/execCommand/indent-pre-expected.txt:49
> +|               "list two"
> +|               <br>
> +|               "list three"
> +|               <br>

The additional <br>s look weird, but I guess it's harmless.

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