Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(703)

Issue 10083059: [Print Preview] Modified PP_PrintSettings_Dev interface to support auto fit to page functionality. (Closed)

Created:
8 years, 8 months ago by kmadhusu
Modified:
8 years, 7 months ago
CC:
chromium-reviews, piman+watch_chromium.org, ihf+watch_chromium.org, darin-cc_chromium.org, yzshen+watch_chromium.org, brettw-cc_chromium.org, b viettrungluu_chromium.org, brettw
Visibility:
Public.

Description

[Print Preview] Modified PP_PrintSettings_Dev interface to support auto fit to page functionality. BUG=85132 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137453

Patch Set 1 #

Patch Set 2 : Added fit_pdf_page_to_paper_size variable in mock_printer #

Patch Set 3 : Fix compile errors #

Patch Set 4 : '' #

Total comments: 19

Patch Set 5 : rebase #

Patch Set 6 : Addressed review comments #

Patch Set 7 : '' #

Patch Set 8 : Removed chrome/renderer/print_web_view_helper changes. #

Patch Set 9 : Removed OVERRIDE keyword to fix compile errors. #

Patch Set 10 : Rename WebPrintScalingOptions to WebPrintScalingOption. #

Total comments: 12

Patch Set 11 : Addressed review comments #

Total comments: 3

Patch Set 12 : Rebase #

Patch Set 13 : Rebase + Use WebKit::WebPrintParams #

Patch Set 14 : '' #

Total comments: 4

Patch Set 15 : Fixed nits #

Total comments: 2

Patch Set 16 : Fix version #

Unified diffs Side-by-side diffs Delta from patch set Stats (+79 lines, -19 lines) Patch
M ppapi/api/dev/ppp_printing_dev.idl View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +12 lines, -2 lines 0 comments Download
M ppapi/c/dev/ppp_printing_dev.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +16 lines, -6 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_plugin_instance.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +2 lines, -1 line 0 comments Download
M webkit/plugins/ppapi/ppapi_plugin_instance.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +24 lines, -5 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_webplugin_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -2 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_webplugin_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +18 lines, -3 lines 0 comments Download

Messages

Total messages: 25 (0 generated)
kmadhusu
dmichael: As per our discussion, I added more params to the PP_PrintSettings_Dev to support auto ...
8 years, 8 months ago (2012-04-19 03:13:02 UTC) #1
vandebo (ex-Chrome)
http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_messages.h File chrome/common/print_messages.h (right): http://codereview.chromium.org/10083059/diff/4003/chrome/common/print_messages.h#newcode44 chrome/common/print_messages.h:44: bool fit_pdf_page_to_paper_size; For some reason this name sticks out ...
8 years, 8 months ago (2012-04-19 18:45:31 UTC) #2
dmichael (off chromium)
+viettrungluu to confirm that PPP_Printing is not used by flash and thus does not need ...
8 years, 8 months ago (2012-04-19 20:11:46 UTC) #3
viettrungluu
Flash doesn't use it quite yet (well, it's behind a flag) ... and this change ...
8 years, 8 months ago (2012-04-19 20:19:05 UTC) #4
kmadhusu
Thanks for all your comments. CC'ed brettw@. I have addressed all your review comments. This ...
8 years, 8 months ago (2012-04-20 22:29:57 UTC) #5
dmichael (off chromium)
http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.cc File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.cc#newcode1162 webkit/plugins/ppapi/ppapi_plugin_instance.cc:1162: print_scaling_option); Please explicitly convert the enum values or add ...
8 years, 8 months ago (2012-04-20 23:01:45 UTC) #6
vandebo (ex-Chrome)
http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.h File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.h#newcode238 webkit/plugins/ppapi/ppapi_plugin_instance.h:238: // WebKit::WebPrintScalingOption after fixing crbug.com/85132. On 2012/04/20 23:01:45, dmichael ...
8 years, 8 months ago (2012-04-21 01:06:48 UTC) #7
dmichael (off chromium)
lgtm
8 years, 8 months ago (2012-04-23 15:21:09 UTC) #8
kmadhusu
dmichael: Thanks for the lgtm stamp. I assume you reviewed my latest patch (PatchSet 11). ...
8 years, 8 months ago (2012-04-23 16:39:29 UTC) #9
dmichael (off chromium)
On 2012/04/23 16:39:29, kmadhusu wrote: > dmichael: Thanks for the lgtm stamp. I assume you ...
8 years, 8 months ago (2012-04-23 17:02:26 UTC) #10
dmichael (off chromium)
still lgtm http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.h File webkit/plugins/ppapi/ppapi_plugin_instance.h (right): http://codereview.chromium.org/10083059/diff/29001/webkit/plugins/ppapi/ppapi_plugin_instance.h#newcode238 webkit/plugins/ppapi/ppapi_plugin_instance.h:238: // WebKit::WebPrintScalingOption after fixing crbug.com/85132. On 2012/04/23 ...
8 years, 8 months ago (2012-04-23 17:02:40 UTC) #11
brettw
http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h File ppapi/c/dev/ppp_printing_dev.h (right): http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h#newcode94 ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 { This versioning isn't right. The version ...
8 years, 8 months ago (2012-04-23 17:57:48 UTC) #12
kmadhusu
http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h File ppapi/c/dev/ppp_printing_dev.h (right): http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h#newcode94 ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 { On 2012/04/23 17:57:48, brettw wrote: > ...
8 years, 8 months ago (2012-04-23 18:20:12 UTC) #13
dmichael (off chromium)
http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h File ppapi/c/dev/ppp_printing_dev.h (right): http://codereview.chromium.org/10083059/diff/31001/ppapi/c/dev/ppp_printing_dev.h#newcode94 ppapi/c/dev/ppp_printing_dev.h:94: struct PPP_Printing_Dev_0_6 { On 2012/04/23 17:57:48, brettw wrote: > ...
8 years, 8 months ago (2012-04-23 19:00:39 UTC) #14
brettw
Okay, we can do what you did. Normally we have to provide backwards compat for ...
8 years, 8 months ago (2012-04-23 22:16:44 UTC) #15
kmadhusu
dmichael@ (repeating the message for future reference): Darin reviewed my webkit patch and asked me ...
8 years, 7 months ago (2012-05-11 21:38:53 UTC) #16
dmichael (off chromium)
lgtm https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_plugin_instance.cc File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_plugin_instance.cc#newcode126 webkit/plugins/ppapi/ppapi_plugin_instance.cc:126: using WebKit::WebPrintParams; nit: these should be in alphabetical ...
8 years, 7 months ago (2012-05-14 17:16:38 UTC) #17
dmichael (off chromium)
oops, one more nit. still lgtm. https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_webplugin_impl.cc File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (right): https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_webplugin_impl.cc#newcode36 webkit/plugins/ppapi/ppapi_webplugin_impl.cc:36: using WebKit::WebPrintParams; nit: ...
8 years, 7 months ago (2012-05-14 17:18:24 UTC) #18
kmadhusu
https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_plugin_instance.cc File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right): https://chromiumcodereview.appspot.com/10083059/diff/59002/webkit/plugins/ppapi/ppapi_plugin_instance.cc#newcode126 webkit/plugins/ppapi/ppapi_plugin_instance.cc:126: using WebKit::WebPrintParams; On 2012/05/14 17:16:39, dmichael wrote: > nit: ...
8 years, 7 months ago (2012-05-14 18:00:32 UTC) #19
viettrungluu
Note: If this doesn't land soon, Flapper will start using the old (current) PPP_Printing interface, ...
8 years, 7 months ago (2012-05-15 20:23:57 UTC) #20
kmadhusu
On 2012/05/15 20:23:57, viettrungluu wrote: > Note: If this doesn't land soon, Flapper will start ...
8 years, 7 months ago (2012-05-15 20:26:44 UTC) #21
kmadhusu
Addressed review comments. FYI: I got r+ for the webkit patch. Once it is landed, ...
8 years, 7 months ago (2012-05-15 22:55:39 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10083059/62004
8 years, 7 months ago (2012-05-16 04:30:26 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10083059/62004
8 years, 7 months ago (2012-05-16 16:29:32 UTC) #24
commit-bot: I haz the power
8 years, 7 months ago (2012-05-16 17:59:21 UTC) #25
Change committed as 137453

Powered by Google App Engine
This is Rietveld 408576698