Currently, CSS property page-break-after and page-break-before are implemented for value "always", but they are not tested due to lack of testing method for printing. I propose layoutTestController.pageNumberForElementById to test them and future change around printing. Though we may eventually want to have another testing method to dump pages as png image(s) (c.f., Bug 20011), this method will be also useful as this can be used in dumpAsText() tests. Patch will be coming.
Created attachment 46901 [details] Patch v1
I checked I can compile this on Mac, Win, Gtk, and Qt. I'd like to postpone the actual implementation of the method for Win, Gtk, and Qt after this patch is landed.
Created attachment 47157 [details] Patch v2
I found I didn't remove a fprintf before I submitted the previous patch. I also changed the place of pageNumberForElement.
Comment on attachment 47157 [details] Patch v2 pageNumberForElement should take in a width/height, or maybe a Vector of IntSize's (since that's what other parts of this code use). Otherwise I think this is a fantastic change!
Created attachment 47500 [details] Patch v3
(In reply to comment #5) > (From update of attachment 47157 [details]) > pageNumberForElement should take in a width/height, or maybe a Vector of > IntSize's (since that's what other parts of this code use). Thanks for your review! I modified my patch so the new method may take width and height. I made them optional because some tests (e.g., tests for page-break-*) won't care the size of pages. We may eventually want to add other parameters such as userScaleFactor, the size of physical pages (page size + margin size, they would be necessary once we implement media queries like "@media print and (width: 21cm)" http://www.w3.org/TR/css3-page/#page-size-media-query), and etc. I'd like to avoid adding them for now because I didn't figure out sufficient set of such parameters yet.
Comment on attachment 47500 [details] Patch v3 Oh. I meant that the WebCore implementation function should take a width/height. I care less about the JS function being able to. :) But it's fine that it does. My concern was more about the fact that WebCore shouldn't know how large the page size DumpRenderTree uses is. At least in the Mac DumpRenderTree the 600x800 size is held in some constants at the topof the files and could theoretically be changed at some later point. :)
Comment on attachment 47500 [details] Patch v3 Why not use a FloatSize here? 83 void PrintContext::computePageRectsWithPageSize(float pageWidth, float pageHeight, float userScaleFactor) And what are the units? Pixels? 85 float printedPagesHeight = 0.0; 102 float printedPagesHeight = 0; Is the compiler smart enough to amke that 0.0f? Likewise here? 92 if (userScaleFactor <= 0) { 0.0f? (the compiler may be smart, I don't know). FloatRect? 56 void computePageRectsWithPageSize(float pageWidth, float pageHeight, float userScaleFactor); Why? 2 * Copyright (C) 2005, 2006 Apple Computer, Inc. All rights reserved. 2 * copyright (C) 2005, 2006 Apple Computer, Inc. All rights reserved. What do these defaults mean? 469 float pageWidth = 982; 470 float pageHeight = 1291; I think it's a nice feature that pageNumberForElementById now takes a width/height, and I'm glad they're not required. I'm not sure why the various tests use that feature, and I'm also confused by the default values and the units. r- mostly due to my confusion. I think we need a few more uses of FloatSize, and we should consider making these variable names carry units in their name. Like pageSizeInPixels? or pageSizeInInches?
Again, I think this is a *fantastic* change. I'm very excited to see it land. I'm mostly confused by some of the details, and would like to sort that out before marking r+.
Created attachment 47668 [details] Patch v4
Thanks for the review! > Why not use a FloatSize here? > 83 void PrintContext::computePageRectsWithPageSize(float pageWidth, float > pageHeight, float userScaleFactor) Done. > And what are the units? Pixels? I added InPixels suffix for all value names in headers. > 85 float printedPagesHeight = 0.0; > 102 float printedPagesHeight = 0; > Is the compiler smart enough to amke that 0.0f? > > Likewise here? > 92 if (userScaleFactor <= 0) { > 0.0f? > (the compiler may be smart, I don't know). Yes, it seems the compiler works for this case. > Why? > 2 * Copyright (C) 2005, 2006 Apple Computer, Inc. All rights reserved. > 2 * copyright (C) 2005, 2006 Apple Computer, Inc. All rights reserved. I'm not sure why this happened... Sorry, fixed. > What do these defaults mean? > 469 float pageWidth = 982; > 470 float pageHeight = 1291; We chatted in #webkit and decided to use 800x600 as the default value. I put a FIXME here. I think we want to define this value in a header file (not sure, but maybe LayoutTestContoller.h ?) and share this value between all ports. But, I'd like to do this refactoring later as this change will touch many files.
Comment on attachment 47668 [details] Patch v4 Why not FloatSize here? 164 int PrintContext::pageNumberForElement(Element* element, float pageWidthInPixels, float pageHeightInPixels) I guess DRT doesn't know how to use FloatSize?
Attachment 47668 [details] was posted by a committer and has review+, assigning to Shinichiro Hamaji for commit.
(In reply to comment #13) > (From update of attachment 47668 [details]) > Why not FloatSize here? > 164 int PrintContext::pageNumberForElement(Element* element, float > pageWidthInPixels, float pageHeightInPixels) > > I guess DRT doesn't know how to use FloatSize? Yeah, I was unsure if it's OK to use FloatSize in WebKit directory. I'll modify the interface of PrintContext::pageNumberForElement before I land the patch. I still think we should not use WebCore class in the interface of WebKit API (except for WebCoreSuppport directory?) so I won't modify pageNumberForElement in WebCoreStatistics.h
Committed r54205: <http://trac.webkit.org/changeset/54205>