Bug 37538 – Implement page format data programming interface

• Component: CSS
• Status: RESOLVED FIXED

[Yuzo Fujishima]

CSS Paged Media Module Level 3 (http://dev.w3.org/csswg/css3-page/) enables the document author to specify the size and orientation of the page box. Combined with Media Queries (http://www.w3.org/TR/2009/CR-css3-mediaqueries-20090423/), it also enables the document author to specify page format depending on the page size.

This means WebCore needs to provide a programming interface for page size negotiation.

[Yuzo Fujishima]

Created attachment 53306 [details]
Add skeletal methods.

[Dimitri Glazkov (Google)]

This seems very detached and hard to understand without the context in which it's used. I'd say this is a rare case where the patch is too small :)

[Eric Seidel (no email)]

Comment on attachment 53306 [details]
Add skeletal methods.

We don't generally add unused/untested code to the repository.

[Yuzo Fujishima]

Created attachment 58977 [details]
Implement page format data programming interface.

[Shinichiro Hamaji]

Comment on attachment 58977 [details]
Implement page format data programming interface.

WebCore/dom/Document.cpp:1517
 +      int x = style->marginLeft().calcValue(maxValue) + style->borderLeft().width() + style->paddingLeft().calcValue(maxValue);
I'm not 100% sure, but maybe we want to make margin=0px when display is none.

WebCore/css/CSSStyleSelector.cpp:5516
 +      case 2:
We usually don't put a newline after case's colon?

WebCore/css/CSSStyleSelector.cpp:5534
 +                  if (!pageSizeFromName(primitiveValue0, primitiveValue1, width, height))
Is this OK even for "size: a4 a4" ?

LayoutTests/printing/page-format-data-expected.txt:22
 +  PASS layoutTestController.preferredPageSizeInPixels(0) is "(816,1056)"
Cannot we make the results more readable? For example, inch(30) or something like this?

LayoutTests/printing/page-format-data.html:120
 +  
We might want to test more invalid cases such as size: 100px 100px 100px, size: -100px, etc.

[Yuzo Fujishima]

Created attachment 59227 [details]
Addressed review comments.

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #5)
> (From update of attachment 58977 [details])
> WebCore/dom/Document.cpp:1517
>  +      int x = style->marginLeft().calcValue(maxValue) + style->borderLeft().width() + style->paddingLeft().calcValue(maxValue);
> I'm not 100% sure, but maybe we want to make margin=0px when display is none.

Changed as suggested.

> 
> WebCore/css/CSSStyleSelector.cpp:5516
>  +      case 2:
> We usually don't put a newline after case's colon?

Done.

> 
> WebCore/css/CSSStyleSelector.cpp:5534
>  +                  if (!pageSizeFromName(primitiveValue0, primitiveValue1, width, height))
> Is this OK even for "size: a4 a4" ?

Added a test.

> 
> LayoutTests/printing/page-format-data-expected.txt:22
>  +  PASS layoutTestController.preferredPageSizeInPixels(0) is "(816,1056)"
> Cannot we make the results more readable? For example, inch(30) or something like this?

Changed to use mmSize() and inchSize().

> 
> LayoutTests/printing/page-format-data.html:120
>  +  
> We might want to test more invalid cases such as size: 100px 100px 100px, size: -100px, etc.

Added tests.

I also changed test expectations for non-mac platforms.

[Shinichiro Hamaji]

Comment on attachment 59227 [details]
Addressed review comments.

WebCore/css/CSSStyleSelector.cpp:5613
 +      Length& h = portrait ? height : width;
This is OK, but I slightly prefer to swap them after the switch sentence if portrait is true.

WebCore/css/CSSStyleSelector.h:266
 +          Length inchLength(double inch);
I think they (or at least the latter three functions) should be placed in private section of this class?

[Yuzo Fujishima]

Created attachment 59479 [details]
display property doesn't apply to @page, addressed comments

[Yuzo Fujishima]

Thank you for the review.

It turned out that display property doesn't apply to @page. I've changed the code accordingly.

(In reply to comment #8)
> (From update of attachment 59227 [details])
> WebCore/css/CSSStyleSelector.cpp:5613
>  +      Length& h = portrait ? height : width;
> This is OK, but I slightly prefer to swap them after the switch sentence if portrait is true.

Done.

> 
> WebCore/css/CSSStyleSelector.h:266
>  +          Length inchLength(double inch);
> I think they (or at least the latter three functions) should be placed in private section of this class?

They are already private, although "Review Patch" shows them as if they were public.

[Shinichiro Hamaji]

Comment on attachment 59479 [details]
display property doesn't apply to @page, addressed comments

> They are already private, although "Review Patch" shows them as if they were public.

Oops. It seems our code pretty printer didn't work well here... Sorry for the confusion.

[Yuzo Fujishima]

Committed r61670: <http://trac.webkit.org/changeset/61670>