Bug 33840 – Provide a way to get page number with layoutTestController

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

Created attachment 46901 [details]
Patch v1

[Shinichiro Hamaji]

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.

[Shinichiro Hamaji]

Created attachment 47157 [details]
Patch v2

[Shinichiro Hamaji]

I found I didn't remove a fprintf before I submitted the previous patch. I also changed the place of pageNumberForElement.

[Eric Seidel (no email)]

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!

[Shinichiro Hamaji]

Created attachment 47500 [details]
Patch v3

[Shinichiro Hamaji]

(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.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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+.

[Shinichiro Hamaji]

Created attachment 47668 [details]
Patch v4

[Shinichiro Hamaji]

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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

Attachment 47668 [details] was posted by a committer and has review+, assigning to Shinichiro Hamaji for commit.

[Shinichiro Hamaji]

(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

[Shinichiro Hamaji]

Committed r54205: <http://trac.webkit.org/changeset/54205>