[webkit-reviews] review denied: [Bug 90611] contenteditable justify commands applied to next paragraph as well : [Attachment 213098] 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 213098: Patch
https://bugs.webkit.org/attachment.cgi?id=213098&action=review

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


The code change looks much better but still r- because there are many stylistic
issues with the patch,
and descriptions in change log entries need to be revised.

> Source/WebCore/ChangeLog:9
> +	   end reaches upto next node with offset 0 just to include the

I don't know what you mean by "reaches up to next node with offset 0".

> Source/WebCore/ChangeLog:11
> +	   paragraph element  who falls under selection causing next unselected


Typo: two spaces before "who".

> Source/WebCore/editing/ApplyStyleCommand.cpp:274
> +    // When a selection ends at the start of a paragraph and is range
selection, style should not be applied to that paragraph

This comment repeats the code below. Please remove it.

> LayoutTests/ChangeLog:10
> +	   TestCases verifying that range selection having end
> +	   selection as paragraph and offset 0 should be ignored while
> +	   treating as Justify candidate.

Why is "TestCases" camel case?	Also, this doesn't explain the end-user
behavior.
e.g. Add a test for selecting one paragraph and the beginning of another
paragraph to ensure that
only the first fully-selected paragraph should be justified when justifying the
selection.

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:10
> +    <p id="p1">Paragraph one.</p>
> +    <p id="p2">Paragraph two.</p>

Please don't use abbreviations like p. Spell out paragraph.

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:17
> +    var firstPara= document.getElementById("p1");
> +    var secondPara = document.getElementById("p2");
> +    getSelection().setBaseAndExtent(firstPara, 0, secondPara, 0);

A much better way of accomplishing the same effect is to use
getSelection().modify as in:
document.getElementById('container').focus();
getSelection().modify('Extend', 'Forward', 'Line');

>
LayoutTests/editing/execCommand/contenteditable-justify-next-paragraph.html:20
> +    Markup.description('Only first Paragraph with id "p1" should have
attribute style="text-align: center"');

We should also explain what we're testing here.
Otherwise, it won't be obvious as to why the output should be what's explained
here.