Bug 84608 - [Chromium] Added a chromium webkit public header file to support different web print scaling options.
Summary: [Chromium] Added a chromium webkit public header file to support different we...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Printing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: kmadhusu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-23 10:16 PDT by kmadhusu
Modified: 2012-04-27 12:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (5.91 KB, patch)
2012-04-23 10:18 PDT, kmadhusu
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2012-04-23 10:20 PDT, kmadhusu
no flags Details | Formatted Diff | Diff
Patch (3.82 KB, patch)
2012-04-23 11:46 PDT, kmadhusu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description kmadhusu 2012-04-23 10:16:02 PDT
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.
Comment 1 kmadhusu 2012-04-23 10:18:00 PDT
Created attachment 138374 [details]
Patch
Comment 2 kmadhusu 2012-04-23 10:20:22 PDT
Created attachment 138375 [details]
Patch
Comment 3 WebKit Review Bot 2012-04-23 10:21:09 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Darin Fisher (:fishd, Google) 2012-04-23 11:26:46 PDT
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
Comment 5 kmadhusu 2012-04-23 11:46:42 PDT
Created attachment 138395 [details]
Patch
Comment 6 kmadhusu 2012-04-23 11:47:37 PDT
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.
Comment 7 kmadhusu 2012-04-23 11:48:28 PDT
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.
Comment 8 kmadhusu 2012-04-23 11:50:44 PDT
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.
Comment 9 kmadhusu 2012-04-25 18:39:07 PDT
ping
Comment 10 Darin Fisher (:fishd, Google) 2012-04-27 11:07:03 PDT
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.
Comment 11 kmadhusu 2012-04-27 11:18:20 PDT
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?
Comment 12 WebKit Review Bot 2012-04-27 12:01:54 PDT
Comment on attachment 138395 [details]
Patch

Clearing flags on attachment: 138395

Committed r115464: <http://trac.webkit.org/changeset/115464>
Comment 13 WebKit Review Bot 2012-04-27 12:01:59 PDT
All reviewed patches have been landed.  Closing bug.