Bug 35961 - Implement render style selection for pages
Summary: Implement render style selection for pages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yuzo Fujishima
URL: http://dev.w3.org/csswg/css3-page/#pa...
Keywords:
Depends on: 35853
Blocks: 15548 37538
  Show dependency treegraph
 
Reported: 2010-03-10 02:04 PST by Yuzo Fujishima
Modified: 2010-06-15 20:23 PDT (History)
12 users (show)

See Also:


Attachments
Implement render style selection for pages to support CSS3 Paged Media. (6.03 KB, patch)
2010-03-10 02:08 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Implement render style selection for pages to support CSS3 Paged Media. (9.89 KB, patch)
2010-03-10 22:51 PST, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Implement render style selection for pages to support CSS3 Paged Media. (9.68 KB, patch)
2010-03-24 23:22 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Implement render style selection for pages to support CSS3 Paged Media. (10.02 KB, patch)
2010-05-12 00:10 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Implement render style selection for pages to support CSS3 Paged Media. (30.69 KB, patch)
2010-05-31 03:21 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
trivial build and style fixes (32.17 KB, patch)
2010-06-01 00:45 PDT, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Add a test for line height. (32.80 KB, patch)
2010-06-09 02:47 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Add a test for user style sheet. (33.87 KB, patch)
2010-06-09 03:04 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Add a test for zoom, to confirm the necessity of calling updateFont(). (34.57 KB, patch)
2010-06-09 23:36 PDT, Yuzo Fujishima
hamaji: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuzo Fujishima 2010-03-10 02:04:33 PST
We need a mechanism to select render style for pages to implement
http://dev.w3.org/csswg/css3-page/#page-selector-and-context
Comment 1 Yuzo Fujishima 2010-03-10 02:08:06 PST
Created attachment 50381 [details]
Implement render style selection for pages to support CSS3 Paged Media.
Comment 2 Yuzo Fujishima 2010-03-10 02:10:00 PST
The first patch above is still work-in-progress but I'd appreciate early feedbacks.
Comment 3 WebKit Review Bot 2010-03-10 02:11:53 PST
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.
Comment 4 WebKit Review Bot 2010-03-10 02:14:05 PST
Attachment 50381 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/570020
Comment 5 Eric Seidel (no email) 2010-03-10 02:17:28 PST
Attachment 50381 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/526015
Comment 6 Yuzo Fujishima 2010-03-10 02:22:59 PST
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
Comment 7 WebKit Review Bot 2010-03-10 03:43:02 PST
Attachment 50381 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/542012
Comment 8 WebKit Review Bot 2010-03-10 03:48:50 PST
Attachment 50381 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/528026
Comment 9 Yuzo Fujishima 2010-03-10 22:51:17 PST
Created attachment 50474 [details]
Implement render style selection for pages to support CSS3 Paged Media.
Comment 10 Yuzo Fujishima 2010-03-24 23:22:06 PDT
Created attachment 51598 [details]
Implement render style selection for pages to support CSS3 Paged Media.
Comment 11 James Robinson 2010-04-15 12:41:25 PDT
Is there any way to test this?  Some bits (like left/right determination, etc) seem really fiddly and easy to regress.
Comment 12 Yuzo Fujishima 2010-05-12 00:10:28 PDT
Created attachment 55811 [details]
Implement render style selection for pages to support CSS3 Paged Media.
Comment 13 Yuzo Fujishima 2010-05-12 00:12:57 PDT
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.
Comment 14 James Robinson 2010-05-12 00:25:03 PDT
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.
Comment 15 Yuzo Fujishima 2010-05-31 03:21:43 PDT
Created attachment 57448 [details]
Implement render style selection for pages to support CSS3 Paged Media.
Comment 16 Yuzo Fujishima 2010-05-31 03:25:47 PDT
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.
Comment 17 WebKit Review Bot 2010-05-31 05:51:36 PDT
Attachment 57448 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/2819007
Comment 18 WebKit Review Bot 2010-05-31 06:56:16 PDT
Attachment 57448 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2763010
Comment 19 Shinichiro Hamaji 2010-06-01 00:45:47 PDT
Created attachment 57522 [details]
trivial build and style fixes
Comment 20 Shinichiro Hamaji 2010-06-01 00:58:56 PDT
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.
Comment 21 Yuzo Fujishima 2010-06-09 02:47:15 PDT
Created attachment 58227 [details]
Add a test for line height.
Comment 22 Yuzo Fujishima 2010-06-09 03:04:44 PDT
Created attachment 58228 [details]
Add a test for user style sheet.
Comment 23 Yuzo Fujishima 2010-06-09 03:07:21 PDT
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.
Comment 24 Yuzo Fujishima 2010-06-09 23:36:00 PDT
Created attachment 58333 [details]
Add a test for zoom, to confirm the necessity of calling updateFont().
Comment 25 Shinichiro Hamaji 2010-06-09 23:51:02 PDT
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?
Comment 26 Yuzo Fujishima 2010-06-15 20:23:30 PDT
Committed r61232: <http://trac.webkit.org/changeset/61232>