[webkit-reviews] review denied: [Bug 40770] Add tests for CSS3 Paged Media's page breaks. : [Attachment 58984] add-tests-for-pagebreaks-fix-style-and-comments

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 40770: Add tests for CSS3 Paged Media's page breaks.
https://bugs.webkit.org/show_bug.cgi?id=40770

Attachment 58984: add-tests-for-pagebreaks-fix-style-and-comments
https://bugs.webkit.org/attachment.cgi?id=58984&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
LayoutTests/ChangeLog:5
 +	    Add tests for CSS3 Paged Media's page breaks. Some tests are
expected to fail because some features are not implemented.
Could you write the names of failing tests?

LayoutTests/ChangeLog:5
 +	    Add tests for CSS3 Paged Media's page breaks. Some tests are
expected to fail because some features are not implemented.
We usually don't write long lines in ChangeLog. Also, it would be better to
break line after the bugzilla summary.

LayoutTests/printing/script-tests/page-break-after-avoid.js:6
 +	// block 'page2-1' must move to the next page because it has
'page-break-after:avoid' and 'page2-1' and 'page2-2' cannot be placed in the
current page at the same time.
We usually don't write long line comments either. Maybe ~100 characters would
be the threshold.

LayoutTests/ChangeLog:21
 +	    * printing/page-break-mergin-collapsed-expected.txt: Added.
s/mergin/margin/

It's somewhat difficult to review 10 tests at once. Could you split this patch?
We don't need 10 patches. Maybe

1. for orphans and widows (3 tests)
2. for page-break-*avoid (3 tests)
3. for others (4 tests)