[webkit-reviews] review denied: [Bug 36683] For compatibility, execCommand should support deprecated 'useCSS' alias for 'styleWithCSS' : [Attachment 107001] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Kiyoto Tamura
<owenestea at gmail.com>'s request for review:
Bug 36683: For compatibility, execCommand should support deprecated 'useCSS'
alias for 'styleWithCSS'
https://bugs.webkit.org/show_bug.cgi?id=36683

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

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


> Source/WebCore/ChangeLog:7
> +

Please explain what kind of change you're making here.	See other change log
entries for examples.

> LayoutTests/ChangeLog:7
> +	   Adding useCSS support for execCommand and changing the behavior of
styleWithCSS in accordance
> +	   to
http://aryeh.name/spec/editing/editing.html#the-stylewithcss-command

This description should appear in the change log for WebCore.

> LayoutTests/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

"Reviewed by" should appear before the description.

> LayoutTests/ChangeLog:13
> +	   * editing/execCommand/script-tests/toggle-text-decorations.js:
> +	   (testSingleToggle):
> +	   * editing/execCommand/toggle-text-decorations-expected.txt:

I don't think modifying these tests is a good way to test this behavior change.
 These tests are testing execCommand('underline'/'strikeThrough') behaviors,
not styleWithCSS or useCSS.

If I were you, I would write a test that does:
1. queryCommandState('styleWithCSS') returns true/fase after calling
execCommand('useCSS'...) with false/true
2. the result of execCommand('bold' or whatever command) contains span/b after
calling execCommand('useCSS') with false/true

r- because of this.