Bug 84608 – [Chromium] Added a chromium webkit public header file to support different web print scaling options.

• Component: Printing
• Status: RESOLVED FIXED

[kmadhusu]

To fix crbug.com/85132, I need a webkit public header file to provide different types of print scaling options. Actual fix for the bug(crbug.com/85132) include ppapi changes, chromium changes and webkit changes. In order to prevent any build breakage while committing the individual CL's, I would like to commit this header file first followed by the patch in https://bugs.webkit.org/show_bug.cgi?id=84312.

[kmadhusu]

Created attachment 138374 [details]
Patch

[kmadhusu]

Created attachment 138375 [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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 138375 [details]
Patch

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

How do you plan to use this enum?  It is often easier to review API changes when it is
clear how the parts will fit together.  This looks like an unused enum :-)

> Source/WebKit/chromium/public/WebPrintScalingOption.h:36
> +enum WebPrintScalingOption {

please fix naming:
http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

[kmadhusu]

Created attachment 138395 [details]
Patch

[kmadhusu]

Comment on attachment 138375 [details]
Patch

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

>> Source/WebKit/chromium/public/WebPrintScalingOption.h:36
>> +enum WebPrintScalingOption {
> 
> please fix naming:
> http://trac.webkit.org/wiki/ChromiumWebKitAPI#Enums

I assume you meant to change the enum value names.  Done.

[kmadhusu]

Sorry I didn't state it clearly in my description. You can see the usage of this enum in the latest patch uploaded in https://bugs.webkit.org/show_bug.cgi?id=84312.

And you can also find the usage of this enum in the following chromium side CL's.
http://codereview.chromium.org/10083059/
http://codereview.chromium.org/10168013/

Thanks.

[kmadhusu]

Patch in bug 84312 uses WebPrintFitToPrintableArea enum value. After addressing all the review comments in this header file, I will update the patch in bug 84312 accordingly.

[kmadhusu]

ping

[Darin Fisher (:fishd, Google)]

Comment on attachment 138395 [details]
Patch

OK, R=me

Normally, it is better to include a change like this with the patch that actually requires it.

[kmadhusu]

Thanks for reviewing the patch. I understand your concern. I am just trying to avoid chrome canary build breakages. By landing this header file first, it easier for me to land the individual patches. Will it help if I add more information in the ChangeLog file?

Can you also please review the other webkit patch (https://bugs.webkit.org/show_bug.cgi?id=84312) which actually uses this enum to make sure you are fine with the workflow?

[WebKit Review Bot]

Comment on attachment 138395 [details]
Patch

Clearing flags on attachment: 138395

Committed r115464: <http://trac.webkit.org/changeset/115464>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.