Bug 33840 - Provide a way to get page number with layoutTestController
Summary: Provide a way to get page number with layoutTestController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Shinichiro Hamaji
URL:
Keywords:
Depends on:
Blocks: 9526
  Show dependency treegraph
 
Reported: 2010-01-19 05:34 PST by Shinichiro Hamaji
Modified: 2010-02-01 22:28 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (23.63 KB, patch)
2010-01-19 05:39 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v2 (22.68 KB, patch)
2010-01-21 16:52 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v3 (28.04 KB, patch)
2010-01-26 22:55 PST, Shinichiro Hamaji
no flags Details | Formatted Diff | Diff
Patch v4 (28.58 KB, patch)
2010-01-28 19:48 PST, Shinichiro Hamaji
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinichiro Hamaji 2010-01-19 05:34:01 PST
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.
Comment 1 Shinichiro Hamaji 2010-01-19 05:39:20 PST
Created attachment 46901 [details]
Patch v1
Comment 2 Shinichiro Hamaji 2010-01-19 05:45:36 PST
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.
Comment 3 Shinichiro Hamaji 2010-01-21 16:52:32 PST
Created attachment 47157 [details]
Patch v2
Comment 4 Shinichiro Hamaji 2010-01-21 16:55:04 PST
I found I didn't remove a fprintf before I submitted the previous patch. I also changed the place of pageNumberForElement.
Comment 5 Eric Seidel (no email) 2010-01-26 15:06:03 PST
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!
Comment 6 Shinichiro Hamaji 2010-01-26 22:55:55 PST
Created attachment 47500 [details]
Patch v3
Comment 7 Shinichiro Hamaji 2010-01-26 23:04:56 PST
(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 8 Eric Seidel (no email) 2010-01-28 18:15:08 PST
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 9 Eric Seidel (no email) 2010-01-28 18:24:49 PST
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?
Comment 10 Eric Seidel (no email) 2010-01-28 18:25:24 PST
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+.
Comment 11 Shinichiro Hamaji 2010-01-28 19:48:07 PST
Created attachment 47668 [details]
Patch v4
Comment 12 Shinichiro Hamaji 2010-01-28 19:49:56 PST
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 13 Eric Seidel (no email) 2010-02-01 15:33:10 PST
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?
Comment 14 Eric Seidel (no email) 2010-02-01 16:12:24 PST
Attachment 47668 [details] was posted by a committer and has review+, assigning to Shinichiro Hamaji for commit.
Comment 15 Shinichiro Hamaji 2010-02-01 17:07:28 PST
(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
Comment 16 Shinichiro Hamaji 2010-02-01 22:28:34 PST
Committed r54205: <http://trac.webkit.org/changeset/54205>