Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
388783003:
Add support to send print preset options in one message (Closed)
DescriptionAdd support to send print preset options in one message
This patch introduces structure that should be filled with all
print preset options that we need to get from PDF and set in print
preview.
Before this patch one message was being sent to browser to set PrintScaling
property but going forward, more properties will be supported and so
it is better to send only one message with all properties set in one structure.
BUG=169120
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285159
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review feedback (update param structure) #
Total comments: 4
Patch Set 3 : Review feedback (add DUPLEX_MODE_LAST) #
Total comments: 4
Patch Set 4 : Remove change for DUPLEX_MODE_LAST #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Fix for Android trybot failure #Messages
Total messages: 48 (0 generated)
This patch is result of discussion at https://codereview.chromium.org/375253002/ It introduces structure and IPC message to be used to set print preset options from PDF document and inform browser. PTAL. Thanks!
https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages.cc File chrome/common/print_messages.cc (right): https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.cc:88: print_range() { duplex(UNKNOWN_DUPLEX_MODE), no need print_range() https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.cc:97: print_copies = 0; we don't need reset(), we are not going to reuse structure. https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages.h File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.h:82: int print_copies; bool is_scaling_disabled; int copies; printing::DuplexMode duplex; printing::PageRanges page_ranges;
Incorporated the changes, PTAL. Thanks! https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages.cc File chrome/common/print_messages.cc (right): https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.cc:88: print_range() { On 2014/07/11 19:28:37, Vitaly Buka wrote: > duplex(UNKNOWN_DUPLEX_MODE), > no need print_range() Done. https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.cc:97: print_copies = 0; On 2014/07/11 19:28:37, Vitaly Buka wrote: > we don't need reset(), we are not going to reuse structure. Done. https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages.h File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/1/chrome/common/print_messages... chrome/common/print_messages.h:82: int print_copies; On 2014/07/11 19:28:37, Vitaly Buka wrote: > bool is_scaling_disabled; > int copies; > printing::DuplexMode duplex; > printing::PageRanges page_ranges; Done.
Let's try to complete this one now :) pdfium changes were submitted yesterday (https://codereview.chromium.org/345123002/).
lgtm +tsepez for print_messages.*
Messages LGTM.
The CQ bit was checked by vitalybuka@chromium.org
The CQ bit was unchecked by vitalybuka@chromium.org
The CQ bit was checked by vitalybuka@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/388783003/20001
https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... chrome/common/print_messages.h:90: printing::MARGIN_TYPE_LAST) Needs IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...)
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/388783003/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
PTAL, Thanks! https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... chrome/common/print_messages.h:90: printing::MARGIN_TYPE_LAST) On 2014/07/15 23:08:39, Vitaly Buka wrote: > Needs IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...) Done.
On 2014/07/16 08:03:22, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) Adding more reviewers for OWNERS review. jochen@ - OWNERS review for chrome/common/print_messages.cc please. ** Presubmit ERRORS ** Missing LGTM from an OWNER for these files: chrome/common/print_messages.cc
once the ctor/dtor is inline, tsepez's looks-good is good enough anyway https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... chrome/common/print_messages.h:76: PrintHostMsg_SetOptionsFromDocument_Params(); can you make the ctor and dtor inline please? https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... chrome/common/print_messages.h:82: printing::PageRanges page_ranges; why is page_ranges not initialized by your ctor?
jochen@ - When I try to make ctor and dtor inline, I get following errors - error: [chromium-style] Complex constructor has an inlined body. error: [chromium-style] Complex destructor has an inline body. https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... chrome/common/print_messages.h:90: printing::MARGIN_TYPE_LAST) On 2014/07/16 08:16:52, Nikhil wrote: > On 2014/07/15 23:08:39, Vitaly Buka wrote: > > Needs IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...) > > Done. Removed the change from this patch. I'll check more; simply adding it here broke the build. https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... chrome/common/print_messages.h:76: PrintHostMsg_SetOptionsFromDocument_Params(); On 2014/07/16 08:53:37, jochen wrote: > can you make the ctor and dtor inline please? I followed style currently followed in the file. But now that I tried to make ctor and dtor inline, I got an error. error: [chromium-style] Complex constructor has an inlined body. Please let me know what do you think should be changed. https://codereview.chromium.org/388783003/diff/40001/chrome/common/print_mess... chrome/common/print_messages.h:82: printing::PageRanges page_ranges; On 2014/07/16 08:53:37, jochen wrote: > why is page_ranges not initialized by your ctor? Based on review comment on patchset 1 (line 88). Also, PageRanges is defined here https://code.google.com/p/chromium/codesearch#chromium/src/printing/page_rang...
vitaly@ - It looks like range for printing::DuplexMode is already defined. https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... File chrome/common/print_messages.h (right): https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... chrome/common/print_messages.h:90: printing::MARGIN_TYPE_LAST) On 2014/07/15 23:08:39, Vitaly Buka wrote: > Needs IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...) The range for printing::DuplexMode seems to be already defined at https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro... Please let me know what do you think.
lgtm https://codereview.chromium.org/388783003/diff/80001/chrome/common/print_mess... File chrome/common/print_messages.cc (right): https://codereview.chromium.org/388783003/diff/80001/chrome/common/print_mess... chrome/common/print_messages.cc:83: PrintHostMsg_SetOptionsFromDocument_Params:: plz clang-format
https://codereview.chromium.org/388783003/diff/80001/chrome/common/print_mess... File chrome/common/print_messages.cc (right): https://codereview.chromium.org/388783003/diff/80001/chrome/common/print_mess... chrome/common/print_messages.cc:83: PrintHostMsg_SetOptionsFromDocument_Params:: On 2014/07/16 13:20:23, jochen wrote: > plz clang-format I already did "git cl format" before uploading the patch.
vitalybuka@ - <ping!> This is ready to submit once you take a look at response to your last review comment (copied below). Thanks! On 2014/07/16 10:25:06, Nikhil wrote: > vitaly@ - It looks like range for printing::DuplexMode is already defined. > > https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... > File chrome/common/print_messages.h (right): > > https://codereview.chromium.org/388783003/diff/20001/chrome/common/print_mess... > chrome/common/print_messages.h:90: printing::MARGIN_TYPE_LAST) > On 2014/07/15 23:08:39, Vitaly Buka wrote: > > Needs IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...) > > The range for printing::DuplexMode seems to be already defined at > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/chro... > > Please let me know what do you think.
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel_swarming on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30943)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/30951)
Thanks for reviews! Bots probably failed because of rebase. I've done so now, and will try to submit this. Thanks again
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/388783003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
The CQ bit was checked by n.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/n.bansal@samsung.com/388783003/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
On 2014/07/21 17:59:39, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_dbg on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) Could someone please help me understand why these two bots are failing.
On 2014/07/21 19:08:47, Nikhil wrote: > On 2014/07/21 17:59:39, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > android_dbg on tryserver.chromium > > > (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) > > Could someone please help me understand why these two bots are failing. It can't see IPC_ENUM_TRAITS_MAX_VALUE(printing::DuplexMode, ...)
PTAL at fix for Android trybot failure. I was able to reproduce error locally by doing Android build. I've moved required macros to print_messages.h now. If you've any suggestion or review comment, then please let me know. Thanks!
vitalybuka@ - <ping!> On 2014/07/22 12:37:18, Nikhil wrote: > PTAL at fix for Android trybot failure. > > I was able to reproduce error locally by doing Android build. I've moved > required macros to print_messages.h now. > > If you've any suggestion or review comment, then please let me know. > > Thanks!
lgtm sorry for delay.
Message was sent while issue was closed.
Change committed as 285159 |
