[webkit-reviews] review denied: [Bug 90611] contenteditable justify commands applied to next paragraph as well : [Attachment 213010] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Santosh Mahto
<santosh.ma at samsung.com>'s request for review:
Bug 90611: contenteditable justify commands applied to next paragraph as well
https://bugs.webkit.org/show_bug.cgi?id=90611

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213010&action=review


> Source/WebCore/ChangeLog:18
> +	   (WebCore::ApplyStyleCommand::applyBlockStyle):Ignore the justify
> +	   command for paragraph if selection is not part of it.

Part of what?

>> Source/WebCore/editing/ApplyStyleCommand.cpp:274
>> +	VisiblePosition
beyondEnd((visibleEnd.deepEquivalent().deprecatedEditingOffset() || visibleEnd
== paragraphStart) ? endOfParagraph(visibleEnd).next() : visibleEnd);
> 
> Can you do this without using anything with “deprecated” in its name?

This change doesn't make much sense. What does the deprecated editing offset
being non-zero to do with visibleEnd == paragraphStart?

> LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:4

> +<script type="text/javascript">

There is no need to specify type in HTML5.

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:12
> +    var elem1 = document.getElementById("p1");
> +    var elem2 = document.getElementById("p2");

Please don't use abbreviations like elem and p. Spell out words.

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:14
> +    var selection = window.getSelection();
> +    selection.setBaseAndExtent(elem1, 0, elem2, 0);

There is no need for the local variable here and "window.".

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:18
> +    document.getElementById("result").innerHTML=  "Final Result:<br>" + 
"1st Paragraph:  " + elem1.getAttribute("style") +  "<br>" + str;

Why isn't this using dump-as-markup or js-test-pre.js?

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:30
> +<p id="result"> </p>

Why do we have a space here?