Bug 40770 – Add tests for CSS3 Paged Media's page breaks.

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[Hayato Ito]

We need tests for CSS3 Paged Media's page breaks.

[Hayato Ito]

Created attachment 58981 [details]
add-tests-for-pagebreaks

[Hayato Ito]

Created attachment 58984 [details]
add-tests-for-pagebreaks-fix-style-and-comments

[Hayato Ito]

FYI.  The patch is splitted from the following patch, including only newly added tests.

https://bugs.webkit.org/show_bug.cgi?id=39735
https://bug-39735-attachments.webkit.org/attachment.cgi?id=57215

[Shinichiro Hamaji]

Comment on attachment 58984 [details]
add-tests-for-pagebreaks-fix-style-and-comments

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)

[Hayato Ito]

Created attachment 59090 [details]
test-for-page-breaks-others

[Hayato Ito]

Thank you for the review.

(In reply to comment #4)
> 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)

Sure. I 've split it to three patches: 
  1. for orphans and widows (3 tests) -> Split to  https://bugs.webkit.org/show_bug.cgi?id=40821 
  2. for page-break-*avoid (3 tests) -> Split to  https://bugs.webkit.org/show_bug.cgi?id=40814
  3. for others (4 tests) -> Attached to this bug id.

I've reflected your comments in each patch. Could you review them?

[Adam Barth]

Comment on attachment 59090 [details]
test-for-page-breaks-others

WebCore/ChangeLog:23
 +          git-svn-id: http://svn.webkit.org/repository/webkit/trunk@61310 268f45cc-cd09-0410-ab3c-d52691b4dbfc
This ChangeLog looks screwed up.  Maybe you didn't mean to include it?

[Hayato Ito]

Created attachment 59230 [details]
test-for-page-breaks-others-remove-unintentional-chnagelog

[Hayato Ito]

Ops. Thank you for pointing out my mistake. It's unintentional.
I've remove that WebCore/ChangeLog entry from the patch.
Could you review it again?

(In reply to comment #7)
> (From update of attachment 59090 [details])
> WebCore/ChangeLog:23
>  +          git-svn-id: http://svn.webkit.org/repository/webkit/trunk@61310 268f45cc-cd09-0410-ab3c-d52691b4dbfc
> This ChangeLog looks screwed up.  Maybe you didn't mean to include it?

[Kent Tamura]

Comment on attachment 59230 [details]
test-for-page-breaks-others-remove-unintentional-chnagelog

OK

[WebKit Commit Bot]

Comment on attachment 59230 [details]
test-for-page-breaks-others-remove-unintentional-chnagelog

Clearing flags on attachment: 59230

Committed r61530: <http://trac.webkit.org/changeset/61530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.