PrintPreview: Fix printer color settings issues based on printer ppd/schema information.

PrintPreview: Fix printer color settings issues based on printer ppd/schema information.

Show/Hide the color options based on printer ppd/schema information. Some printers does not provide sufficient information in the printer schema/ppd regarding the color settings and they use custom advance settings to print in black & white/greyscale. In those cases, users need to print using native dialog in order to set these advance color settings.

BUG=93811, 93490, 87344, 96658, 98768
TEST= Please refer to bug description.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105087

[kmadhusu, 2011-10-07 01:42:37]

vandebo: Please review c++ code.
dpapad: Please review js code.

As per windows printer schema specification(Ref:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms715146(v=vs.85).aspx),
"PageOutputColor" feature  specifies the possible color settings for the output.
We use this information to identify whether the printer can support color or
not.

On Linux and Mac, the "ColorModel" directive present in the ppd file specifies
the supported color models of the printer.

Some of the printers, does not provide sufficient information in the printer
schema/ppd regarding the color settings and they use custom advance settings to
print in black and white/greyscale.

If we are unable to get the required information from the printer ppd/schema, we
are going to hide the color settings and default it to either black/color
depending on the available data. Users need to print using native dialog in
order to set the advance color settings.

Thanks.

[dpapad, 2011-10-07 17:04:40]

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:18: this.COLOR = 2;
COLOR and GRAY are constants, they dont need to be member vars, they should be
the equivalent of static vars. Move them outside and right after the constructor
and update references.

ColorSettings.GRAY = 1;
ColorSettings.COLOR = 2;

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:44: * @return {Number}
Returns the printer color space
Nit: s/Number/number. Remove line 43 and "Returns" from @return comment.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:94: * Listener
executing when a PDFLoaded event occurs.
Nit: s/Listener executing/Executes
(I know you did not change this, but I am trying to make the comments of all
listeners to be consistent and "Listener executing" is redundant.)

[vandebo (ex-Chrome), 2011-10-07 18:23:30]

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:196: const char* color_mode;
maybe call these two color_setting_name and color_value ?

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:297: class PrintSystemTaskProxy
Maybe this class should get moved out of webui?

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:399: int
printer_color_space_for_color = printing::COLOR;
Move both of these down to line 448 - just before you set them.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:452: if (color_model) {
I would pull each of these blocks into a method...
bool getBasicColorModelSettings(int* color_model_for_black, int*
color_model_for_color, bool* color_is_default);
bool getFoomaticColorModelSetting(int* color_model_for_black, int*
color_model_for_color, bool* color_is_default);
etc...
Maybe you need a constant for invalid color model too?

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:453: if
(ppdFindChoice(color_model, printing::kBlack))
Does this need to happen even if it's not a color model?

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:632: disable_color_options =
!supports_color || !supports_grayscale;
Can you use this logic above?

http://codereview.chromium.org/8138020/diff/10001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8138020/diff/10001/printing/print_job_constant...
printing/print_job_constants.h:92: enum ColorMode {
Maybe this should be named ColorModels now?

http://codereview.chromium.org/8138020/diff/10001/printing/printing_context_m...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8138020/diff/10001/printing/printing_context_m...
printing/printing_context_mac.mm:254: base::mac::ScopedCFTypeRef<CFStringRef>
color_setting(kColorModel);
Can this code be shared with the gtk code somehow?

[kmadhusu, 2011-10-10 23:34:25]

Addressed review comments. Please review the latest patch.

Thanks.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/printing/pri...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:196: const char* color_mode;
On 2011/10/07 18:23:30, vandebo wrote:
> maybe call these two color_setting_name and color_value ?

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:18: this.COLOR = 2;
On 2011/10/07 17:04:40, dpapad wrote:
> COLOR and GRAY are constants, they dont need to be member vars, they should be
> the equivalent of static vars. Move them outside and right after the
constructor
> and update references.
> 
> ColorSettings.GRAY = 1;
> ColorSettings.COLOR = 2;

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:44: * @return {Number}
Returns the printer color space
On 2011/10/07 17:04:40, dpapad wrote:
> Nit: s/Number/number. Remove line 43 and "Returns" from @return comment.

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/color_settings.js:94: * Listener
executing when a PDFLoaded event occurs.
On 2011/10/07 17:04:40, dpapad wrote:
> Nit: s/Listener executing/Executes
> (I know you did not change this, but I am trying to make the comments of all
> listeners to be consistent and "Listener executing" is redundant.)

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:297: class PrintSystemTaskProxy
On 2011/10/07 18:23:30, vandebo wrote:
> Maybe this class should get moved out of webui?

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:399: int
printer_color_space_for_color = printing::COLOR;
On 2011/10/07 18:23:30, vandebo wrote:
> Move both of these down to line 448 - just before you set them.

This is also used by windows code.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:452: if (color_model) {
On 2011/10/07 18:23:30, vandebo wrote:
> I would pull each of these blocks into a method...
> bool getBasicColorModelSettings(int* color_model_for_black, int*
> color_model_for_color, bool* color_is_default);
> bool getFoomaticColorModelSetting(int* color_model_for_black, int*
> color_model_for_color, bool* color_is_default);
> etc...
> Maybe you need a constant for invalid color model too?

Done.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:453: if
(ppdFindChoice(color_model, printing::kBlack))
On 2011/10/07 18:23:30, vandebo wrote:
> Does this need to happen even if it's not a color model?

No. I have seen Black and Gray only as ColorModel attribute values.

http://codereview.chromium.org/8138020/diff/10001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:632: disable_color_options =
!supports_color || !supports_grayscale;
On 2011/10/07 18:23:30, vandebo wrote:
> Can you use this logic above?

Done.

http://codereview.chromium.org/8138020/diff/10001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8138020/diff/10001/printing/print_job_constant...
printing/print_job_constants.h:92: enum ColorMode {
On 2011/10/07 18:23:30, vandebo wrote:
> Maybe this should be named ColorModels now?

Done.

http://codereview.chromium.org/8138020/diff/10001/printing/printing_context_m...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8138020/diff/10001/printing/printing_context_m...
printing/printing_context_mac.mm:254: base::mac::ScopedCFTypeRef<CFStringRef>
color_setting(kColorModel);
On 2011/10/07 18:23:30, vandebo wrote:
> Can this code be shared with the gtk code somehow?

Done.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
File chrome/browser/printing/print_system_task_proxy.cc (right):

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:427: if (is_color_device &&
ppdFindOption(ppd, kColorModel)) {
All the data members and member functions of this class are moved from
print_preview_handler.cc. 

Please review the new helper functions(getBasicColorModelSettings,
getPrintOutModeColorSettings, getColorModeSettings, getHPColorSettings and
getProcessColorModelSettings) and lines 427 - 512.

[dpapad, 2011-10-11 01:00:11]

LGTM for js.

[vandebo (ex-Chrome), 2011-10-11 19:11:42]

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
File chrome/browser/printing/print_system_task_proxy.cc (right):

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:271: const char kPskColor[] =
"psk:Color";
Consider putting this block first since it is much shorter than the other block.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:403: bool is_color_device =
false;
consider moving this block down to 425, so all the color code is together.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:427: if (is_color_device &&
ppdFindOption(ppd, kColorModel)) {
Hmm, this structure ends up calling ppdFindOption twice for each of the
different option styles.  I think you could express the same sentiment at this
level with a slightly clever trick:  make each of the methods return true/false
if they processed the result/didn't.  Then make this a single line:
getBasicColorModelSettings(...) ||
getPrintOutModeColorSettings(...) ||
...

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:481: set_color_as_default =
move with the other color code.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:187: static
printing::ColorModels last_used_color_model_;
Does this mean that we could have used color (CMYK) last time, but then switched
to a different printer that uses RGB, but has a default of grey, and it would
switch to "B&W", whereas if we switched to a different printer that did have
CMYK it would stay set to "Color"?

http://codereview.chromium.org/8138020/diff/31001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8138020/diff/31001/printing/print_job_constant...
printing/print_job_constants.h:118: // Get the color model setting name and
value for the |color_mode|.
These aren't constants... Is there a better place for them?

[kmadhusu, 2011-10-11 21:36:02]

vandebo: Addressed review comments. Please review again.

Thanks.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
File chrome/browser/printing/print_system_task_proxy.cc (right):

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:271: const char kPskColor[] =
"psk:Color";
On 2011/10/11 19:11:42, vandebo wrote:
> Consider putting this block first since it is much shorter than the other
block.

Done.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:403: bool is_color_device =
false;
On 2011/10/11 19:11:42, vandebo wrote:
> consider moving this block down to 425, so all the color code is together.

Done.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:427: if (is_color_device &&
ppdFindOption(ppd, kColorModel)) {
On 2011/10/11 19:11:42, vandebo wrote:
> Hmm, this structure ends up calling ppdFindOption twice for each of the
> different option styles.  I think you could express the same sentiment at this
> level with a slightly clever trick:  make each of the methods return
true/false
> if they processed the result/didn't.  Then make this a single line:
> getBasicColorModelSettings(...) ||
> getPrintOutModeColorSettings(...) ||
> ...

Done.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/printing/pri...
chrome/browser/printing/print_system_task_proxy.cc:481: set_color_as_default =
On 2011/10/11 19:11:42, vandebo wrote:
> move with the other color code.

Done.

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/ui/webui/pri...
File chrome/browser/ui/webui/print_preview_handler.h (right):

http://codereview.chromium.org/8138020/diff/31001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.h:187: static
printing::ColorModels last_used_color_model_;
On 2011/10/11 19:11:42, vandebo wrote:
> Does this mean that we could have used color (CMYK) last time, but then
switched
> to a different printer that uses RGB, but has a default of grey, and it would
> switch to "B&W", whereas if we switched to a different printer that did have
> CMYK it would stay set to "Color"?

(repeating our conversation): I don't compare the actual values. I just check if
the last used color model is black or color. Depending on that value, I use the
selected printer color model values accordingly.

http://codereview.chromium.org/8138020/diff/31001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/8138020/diff/31001/printing/print_job_constant...
printing/print_job_constants.h:118: // Get the color model setting name and
value for the |color_mode|.
On 2011/10/11 19:11:42, vandebo wrote:
> These aren't constants... Is there a better place for them?

Moved to print_settings.h & .cc

[vandebo (ex-Chrome), 2011-10-11 21:41:19]

LGTM