[webkit-reviews] review denied: [Bug 33840] Provide a way to get page number with layoutTestController : [Attachment 47500] Patch v3
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 28 18:24:48 PST 2010
Eric Seidel <eric at webkit.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 33840: Provide a way to get page number with layoutTestController
https://bugs.webkit.org/show_bug.cgi?id=33840
Attachment 47500: Patch v3
https://bugs.webkit.org/attachment.cgi?id=47500&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
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?
More information about the webkit-reviews
mailing list