Bug 84608

Summary: [Chromium] Added a chromium webkit public header file to support different web print scaling options.
Product: WebKit Reporter: kmadhusu <kmadhusu>
Component: PrintingAssignee: kmadhusu <kmadhusu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, jamesr, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

kmadhusu
Reported 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.
Attachments
Patch (5.91 KB, patch)
2012-04-23 10:18 PDT, kmadhusu
no flags
Patch (3.82 KB, patch)
2012-04-23 10:20 PDT, kmadhusu
no flags
Patch (3.82 KB, patch)
2012-04-23 11:46 PDT, kmadhusu
no flags
kmadhusu
Comment 1 2012-04-23 10:18:00 PDT
kmadhusu
Comment 2 2012-04-23 10:20:22 PDT
WebKit Review Bot
Comment 3 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.
Darin Fisher (:fishd, Google)
Comment 4 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
kmadhusu
Comment 5 2012-04-23 11:46:42 PDT
kmadhusu
Comment 6 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.
kmadhusu
Comment 7 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.
kmadhusu
Comment 8 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.
kmadhusu
Comment 9 2012-04-25 18:39:07 PDT
ping
Darin Fisher (:fishd, Google)
Comment 10 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.
kmadhusu
Comment 11 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?
WebKit Review Bot
Comment 12 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>
WebKit Review Bot
Comment 13 2012-04-27 12:01:59 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.