[webkit-reviews] review granted: [Bug 52495] Beef up PrintContext for WebKit2 : [Attachment 79037] proposed patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Alexey Proskuryakov <ap at webkit.org>'s request for
review:
Bug 52495: Beef up PrintContext for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=52495

Attachment 79037: proposed patch
https://bugs.webkit.org/attachment.cgi?id=79037&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=79037&action=review

> Source/WebCore/ChangeLog:19
> +	   Round pahe height to an integer, because Mac code does that, and
because page height is

Typo: pahe

> Source/WebCore/page/PrintContext.cpp:35
> +// print in IE and Camino. This lets them use fewer sheets than they

Camino, really?

> Source/WebCore/page/PrintContext.cpp:38
> +const float PrintingMinimumShrinkFactor = 1.25f;

Can you drop the f? Our current style is to use a lowercase p here. Not sure
there’s a reason to say “printing” in this context.

> Source/WebCore/page/PrintContext.cpp:44
> +const float PrintingMaximumShrinkFactor = 2.0f;

Can you drop the .0f and use a lowercase P?

> Source/WebCore/page/PrintContext.cpp:68
> +void PrintContext::computePageRects(const FloatRect& printRect, float
headerHeight, float footerHeight, float userScaleFactor, float& outPageHeight,
bool allowHorizontalMultiPages)

I have no idea what a multi-page is. Perhaps allowHorizontalTiling or
allowTilingHorizontally?

> Source/WebCore/page/PrintContext.cpp:141
> +	   return 1.0f;

Is the .0f necessary?

> Source/WebCore/page/PrintContext.cpp:145
> +	   return 1.0f;

Ditto

> Source/WebCore/page/PrintContext.cpp:147
> +    float maxShrinkToFitScaleFactor = 1.0f / PrintingMaximumShrinkFactor;

This .0f is not needed.