Bug 40452 - REGRESSION: printing is broken if stylesheet has @page
Summary: REGRESSION: printing is broken if stylesheet has @page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Normal
Assignee: Yuzo Fujishima
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-06-10 16:22 PDT by Simon Fraser (smfr)
Modified: 2010-07-12 04:19 PDT (History)
3 users (show)

See Also:


Attachments
Testcase (4.18 KB, text/html)
2010-06-10 16:22 PDT, Simon Fraser (smfr)
no flags Details
Fix Bug 40452: REGRESSION: printing is broken if stylesheet has @page (2.21 KB, patch)
2010-06-10 20:20 PDT, Yuzo Fujishima
no flags Details | Formatted Diff | Diff
Refactored and added a test. (17.13 KB, patch)
2010-06-10 22:07 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.
Comment 1 Simon Fraser (smfr) 2010-06-10 16:23:02 PDT
<rdar://problem/8081713>
Comment 2 Yuzo Fujishima 2010-06-10 18:22:33 PDT
Sorry for the regression. I'll look into this.
Comment 3 Yuzo Fujishima 2010-06-10 20:20:29 PDT
Created attachment 58441 [details]
Fix Bug 40452: REGRESSION: printing is broken if stylesheet has @page
Comment 4 Shinichiro Hamaji 2010-06-10 20:54:33 PDT
Comment on attachment 58441 [details]
Fix Bug 40452: REGRESSION: printing is broken if stylesheet has @page

WebCore/css/CSSStyleSelector.cpp:2795
 +                          if (childItem->isPageRule()) {
Cannot we create a function to do this so we can share code?

WebCore/ChangeLog:8
 +          No new tests because currently there is no easy way to test print layout.
I'm not 100% sure, but I guess we can test this patch by setPrinting though pixel dump isn't supported yet.
Comment 5 Yuzo Fujishima 2010-06-10 22:07:54 PDT
Created attachment 58444 [details]
Refactored and added a test.
Comment 6 Yuzo Fujishima 2010-06-10 22:09:54 PDT
Thank you for the review. Can you take another look?

(In reply to comment #4)
> (From update of attachment 58441 [details])
> WebCore/css/CSSStyleSelector.cpp:2795
>  +                          if (childItem->isPageRule()) {
> Cannot we create a function to do this so we can share code?

Agreed. Extracted a method.

> 
> WebCore/ChangeLog:8
>  +          No new tests because currently there is no easy way to test print layout.
> I'm not 100% sure, but I guess we can test this patch by setPrinting though pixel dump isn't supported yet.

Added a test.
Comment 7 Shinichiro Hamaji 2010-06-10 22:18:10 PDT
Comment on attachment 58444 [details]
Refactored and added a test.

Looks good to me.
Comment 8 Shinichiro Hamaji 2010-06-10 22:18:49 PDT
Ah, please add chromium's test_expectations.txt before you land.
Comment 9 Yuzo Fujishima 2010-06-10 22:51:55 PDT
Committed r60992: <http://trac.webkit.org/changeset/60992>
Comment 10 Shinichiro Hamaji 2010-07-12 04:19:00 PDT
Committed r63070: <http://trac.webkit.org/changeset/63070>