[webkit-reviews] review denied: [Bug 84312] [Chromium] Added more params to Chromium-Webkit print api to implement auto fit to page functionality. : [Attachment 139405] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied kmadhusu
<kmadhusu at chromium.org>'s request for review:
Bug 84312: [Chromium] Added more params to Chromium-Webkit print api to
implement auto fit to page functionality.
https://bugs.webkit.org/show_bug.cgi?id=84312

Attachment 139405: Patch
https://bugs.webkit.org/attachment.cgi?id=139405&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139405&action=review


> Source/WebKit/chromium/public/WebFrame.h:468
> +    // TODO(kmadhusu at chromium.org): This is a temporary interface to avoid
the

nit: webkit style doesn't use TODO(user).  just use FIXME.

> Source/WebKit/chromium/public/WebFrame.h:488
> +    virtual int printBegin(const WebSize& printContentSize,

I suspect it would be better to bottle up these parameters into a
WebPrintParams
struct.  See for example WebPluginParams.  Exclude the constrainToNode
parameter
so that you can reuse WebPrintParams in the WebPlugin printBegin method.

> Source/WebKit/chromium/public/WebPlugin.h:104
> +    // TODO(kmadhusu at chromium.org): Change the data type of
printScalingOption to

nit: TODO(user) -> FIXME

Use WebPrintParams here too.