Bug 41150 – Improve default value handling of page format properties

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yuzo Fujishima]

Programming interface to page format properties was added by
https://bugs.webkit.org/show_bug.cgi?id=37538

But the default values (page size and orientation) were hard-coded.
Also, margins should be returned instead of page rect.
These need to be improved.

[Yuzo Fujishima]

Created attachment 59639 [details]
Improve default value handling for page format properties.

[Shinichiro Hamaji]

Comment on attachment 59639 [details]
Improve default value handling for page format properties.

WebCore/ChangeLog:7
 +          Also, margins should be returned instead of page area rect.
I think you should also mention we need the default paper size to calculate % .

WebCore/css/CSSStyleSelector.h:98
 +          void setDefaultPageSizeInPixels(IntSize s) { m_defaultPageSize = s; }
Maybe we might want to set this value to m_parentElement or m_rootElement (not sure which is more appropriate). Or, could you write a brief comment how we will use m_defaultPageSize?

WebCore/dom/Document.h:487
 +      void pageSizeAndMarginsInPixels(int pageIndex, IntSize& pageSize, int& marginTop, int& marginRight, int& marginBottom, int& marginLeft);
We need a comment here. Copy & paste from WebFrame.h would be sufficient.

WebKit/chromium/public/WebFrame.h:415
 +  
Unnecessary blank line.

[Yuzo Fujishima]

Created attachment 59739 [details]
Addressed review comments.

[Yuzo Fujishima]

Thank you for the review.

(In reply to comment #2)
> (From update of attachment 59639 [details])
> WebCore/ChangeLog:7
>  +          Also, margins should be returned instead of page area rect.
> I think you should also mention we need the default paper size to calculate % .

Done.

> 
> WebCore/css/CSSStyleSelector.h:98
>  +          void setDefaultPageSizeInPixels(IntSize s) { m_defaultPageSize = s; }
> Maybe we might want to set this value to m_parentElement or m_rootElement (not sure which is more appropriate). Or, could you write a brief comment how we will use m_defaultPageSize?

On second thought, I should not have tried to resolve auto size in CSSStyleSelector.
I changed the code such that auto page size is resolved in Document just as auto margins are.

> 
> WebCore/dom/Document.h:487
>  +      void pageSizeAndMarginsInPixels(int pageIndex, IntSize& pageSize, int& marginTop, int& marginRight, int& marginBottom, int& marginLeft);
> We need a comment here. Copy & paste from WebFrame.h would be sufficient.
> 

Done.

> WebKit/chromium/public/WebFrame.h:415
>  +  
> Unnecessary blank line.

Done.

[Shinichiro Hamaji]

Comment on attachment 59739 [details]
Addressed review comments.

Looks good. A few nitpicks:

WebCore/css/CSSStyleSelector.cpp:5517
 +      m_style->setPageSizeType(PAGE_SIZE_UNDEFINED);
The standard way would be to define RenderStyle::resetPageSizeType

WebCore/dom/Document.cpp:1518
 +      case PAGE_SIZE_UNDEFINED:
Can this happen? If not, we might want ASSERT_NOT_REACHED. Otherwise, we might not need PAGE_SIZE_UNDEFINED and we can just use PAGE_SIZE_AUTO as the initial value of pageSizeType.

[Yuzo Fujishima]

Created attachment 59743 [details]
Addressed second set of review comments.

[Yuzo Fujishima]

Thank you again for the review.

(In reply to comment #5)
> (From update of attachment 59739 [details])
> Looks good. A few nitpicks:
> 
> WebCore/css/CSSStyleSelector.cpp:5517
>  +      m_style->setPageSizeType(PAGE_SIZE_UNDEFINED);
> The standard way would be to define RenderStyle::resetPageSizeType

Done.

> 
> WebCore/dom/Document.cpp:1518
>  +      case PAGE_SIZE_UNDEFINED:
> Can this happen? If not, we might want ASSERT_NOT_REACHED. Otherwise, we might not need PAGE_SIZE_UNDEFINED and we can just use PAGE_SIZE_AUTO as the initial value of pageSizeType.

Removed PAGE_SIZE_UNDEFINED and used PAGE_SIZE_AUTO.

Also updated LayoutTests/printing/page-rule-selection{.html,-expected.txt} to be consistent
with the new default page size value.

[Shinichiro Hamaji]

Comment on attachment 59743 [details]
Addressed second set of review comments.

Looks good!

[Yuzo Fujishima]

Committed r61975: <http://trac.webkit.org/changeset/61975>