[webkit-reviews] review denied: [Bug 47233] REGRESSION: Indenting pre duplicates content : [Attachment 70134] fixes the bug; fixed release build

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 47233: REGRESSION: Indenting pre duplicates content
https://bugs.webkit.org/show_bug.cgi?id=47233

Attachment 70134: fixes the bug; fixed release build
https://bugs.webkit.org/attachment.cgi?id=70134&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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.