Bug 35961 – Implement render style selection for pages

• Component: CSS
• Status: RESOLVED FIXED

[Yuzo Fujishima]

We need a mechanism to select render style for pages to implement
http://dev.w3.org/csswg/css3-page/#page-selector-and-context

[Yuzo Fujishima]

Created attachment 50381 [details]
Implement render style selection for pages to support CSS3 Paged Media.

[Yuzo Fujishima]

The first patch above is still work-in-progress but I'd appreciate early feedbacks.

[WebKit Review Bot]

Attachment 50381 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:1482:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/css/CSSStyleSelector.cpp:3027:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/css/CSSStyleSelector.h:185:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 50381 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/570020

[Eric Seidel (no email)]

Attachment 50381 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/526015

[Yuzo Fujishima]

Hi, Eric,

Thank you for your attention.

For successful compilation, this patch requires all the patches in the dependence chain, i.e., 35853, 35851, ...

Yuzo

[WebKit Review Bot]

Attachment 50381 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/542012

[WebKit Review Bot]

Attachment 50381 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/528026

[Yuzo Fujishima]

Created attachment 50474 [details]
Implement render style selection for pages to support CSS3 Paged Media.

[Yuzo Fujishima]

Created attachment 51598 [details]
Implement render style selection for pages to support CSS3 Paged Media.

[James Robinson]

Is there any way to test this?  Some bits (like left/right determination, etc) seem really fiddly and easy to regress.

[Yuzo Fujishima]

Created attachment 55811 [details]
Implement render style selection for pages to support CSS3 Paged Media.

[Yuzo Fujishima]

Reviewers,

Please hold off reviewing the patch. It is not finished yet.

I uploaded it only because to keep the patch applicable to the current HEAD.

[James Robinson]

Comment on attachment 55811 [details]
Implement render style selection for pages to support CSS3 Paged Media.

Clearing r? flag since it's not ready for review.  Yuzo, you can upload with the --no-review flag to avoid setting r?.  This will keep it out of the review queue.

[Yuzo Fujishima]

Created attachment 57448 [details]
Implement render style selection for pages to support CSS3 Paged Media.

[Yuzo Fujishima]

Hi, Reviewers,

I'd appreciate it if you could resume reviewing the patch.
I've extended layoutTestController and added tests.

I'll add stubs for non-Mac platforms once the core part of the patch looks good.

Yuzo

PS
James, thank you for the comment. I'll use --no-review next time I need it.

[WebKit Review Bot]

Attachment 57448 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2819007

[WebKit Review Bot]

Attachment 57448 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2763010

[Shinichiro Hamaji]

Created attachment 57522 [details]
trivial build and style fixes

[Shinichiro Hamaji]

Comment on attachment 57522 [details]
trivial build and style fixes

I did trivial fixes as Yuzo is out of office.


WebCore/css/CSSStyleSelector.cpp:1561
 +      matchPageRules(m_userStyle, isLeft, isFirst, page);
Does this mean we can use @page with user stylesheet? If so, I think we should add a layout test to check this. I think we can remove this for now and put FIXME?


WebCore/css/html.css:689
 +      margin: 1in;
I think this is a temporal value and needs FIXME. For example, chromium-linux is using Left = 0.25 in, Right = 0.25 in, Top = 0.25 in, Bottom = 0.56 in.

[Yuzo Fujishima]

Created attachment 58227 [details]
Add a test for line height.

[Yuzo Fujishima]

Created attachment 58228 [details]
Add a test for user style sheet.

[Yuzo Fujishima]

Thank you for the review. Can you take another look?

(In reply to comment #20)
> (From update of attachment 57522 [details])
> I did trivial fixes as Yuzo is out of office.
> 
> 
> WebCore/css/CSSStyleSelector.cpp:1561
>  +      matchPageRules(m_userStyle, isLeft, isFirst, page);
> Does this mean we can use @page with user stylesheet? If so, I think we should add a layout test to check this. I think we can remove this for now and put FIXME?

I think user stylesheet can contain @page rules. Any reasons it can't?
I added a test.

> 
> 
> WebCore/css/html.css:689
>  +      margin: 1in;
> I think this is a temporal value and needs FIXME. For example, chromium-linux is using Left = 0.25 in, Right = 0.25 in, Top = 0.25 in, Bottom = 0.56 in.

Added a FIXME comment.

[Yuzo Fujishima]

Created attachment 58333 [details]
Add a test for zoom, to confirm the necessity of calling updateFont().

[Shinichiro Hamaji]

Comment on attachment 58333 [details]
Add a test for zoom, to confirm the necessity of calling updateFont().

This change looks good to me. Please wait for a while before landing this. Other reviewers might have some opinions.

LayoutTests/printing/page-rule-selection.html:22
 +          if (window.layoutTestController) {
Unnecessary?

[Yuzo Fujishima]

Committed r61232: <http://trac.webkit.org/changeset/61232>