[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