Bug 84312 – [Chromium] Added more params to Chromium-Webkit print api to implement auto fit to page functionality.

• Status: RESOLVED
• Resolution: FIXED
• Component: Printing

[kmadhusu]

To fix crbug.com/85132, I need to pass additional print params such as selected printer paper size, printable area and scale to fit value to the plugin print context. Therefore, I modified printBegin() function accordingly. Please review and let me know your comments.

[kmadhusu]

Created attachment 137828 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[kmadhusu]

Comment on attachment 137828 [details]
Patch

Patch is not ready for review yet. I will update the review flag as soon as it is ready for review. 

Thanks.

[kmadhusu]

Created attachment 138175 [details]
Patch

[kmadhusu]

Created attachment 138378 [details]
Patch

[kmadhusu]

Latest patch uses WebPrintScalingOption.h. This is a new header file. I would like to commit that header file first to support Chrome side changes. Therefore, I uploaded that header file patch in https://bugs.webkit.org/show_bug.cgi?id=84608.

[WebKit Review Bot]

Comment on attachment 138378 [details]
Patch

Attachment 138378 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12518135

[kmadhusu]

Created attachment 139405 [details]
Patch

[kmadhusu]

Updated patch and it is ready for review.

[kmadhusu]

ping.

[kmadhusu]

fishd@: Can you please review the latest patch?

Thanks.

[Darin Fisher (:fishd, Google)]

Comment on attachment 139405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139405&action=review

> Source/WebKit/chromium/public/WebFrame.h:468
> +    // TODO([e-mail]): 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([e-mail]): Change the data type of printScalingOption to

nit: TODO(user) -> FIXME

Use WebPrintParams here too.

[kmadhusu]

Created attachment 141493 [details]
Patch

[kmadhusu]

Created attachment 141495 [details]
Patch

[kmadhusu]

Comment on attachment 139405 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139405&action=review

Addressed review comments. PTAL. Thanks.

>> Source/WebKit/chromium/public/WebFrame.h:468
>> +    // TODO([e-mail]): This is a temporary interface to avoid the
> 
> nit: webkit style doesn't use TODO(user).  just use FIXME.

Fixed.

>> 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.

Done.

>> Source/WebKit/chromium/public/WebPlugin.h:104
>> +    // TODO([e-mail]): Change the data type of printScalingOption to
> 
> nit: TODO(user) -> FIXME
> 
> Use WebPrintParams here too.

Done.

[Darin Fisher (:fishd, Google)]

Comment on attachment 141495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141495&action=review

> Source/WebKit/chromium/public/WebPrintParams.h:60
> +          , printScalingOption(WebPrintScalingOptionFitToPrintableArea) { }

nit: indentation

unindent the comma-prefixed rows by 2 spaces

> Source/WebKit/chromium/public/WebPrintParams.h:64
> +          , printableArea(printableArea)

nit: indentation

[kmadhusu]

Created attachment 142063 [details]
Patch

[kmadhusu]

Comment on attachment 141495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141495&action=review

Fixed nits.

>> Source/WebKit/chromium/public/WebPrintParams.h:60
>> +          , printScalingOption(WebPrintScalingOptionFitToPrintableArea) { }
> 
> nit: indentation
> 
> unindent the comma-prefixed rows by 2 spaces

Fixed.

>> Source/WebKit/chromium/public/WebPrintParams.h:64
>> +          , printableArea(printableArea)
> 
> nit: indentation

Fixed.

[WebKit Review Bot]

Comment on attachment 142063 [details]
Patch

Clearing flags on attachment: 142063

Committed r117200: <http://trac.webkit.org/changeset/117200>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.