PrintPreview: Fixed RICOH MP C3501 color print job issues.

PrintPreview: Fixed RICOH MP C3501 color print job issues.

BUG=92358
TEST=Please refer to bug description.

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

[kmadhusu, 2011-09-02 21:58:01]

Verified my changes on Linux. Need to verify on 
Win and Mac.

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

Thanks.

[dpapad, 2011-09-02 22:15:06]

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:17: this.cmyk_ = 0;
Since these are supposed to be constants I think it is better to capitalize the
names and remove the _. We can start in colorSettings and do the same for any
other constants already defined in other files later.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:17: this.cmyk_ = 0;
What is cmyk? Could you document it?

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:44: * @return {Number}
Returns the printer color space
Nit: "Returns" is redundant.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:46: getColorMode:
function() {
Could you use the get syntax like above?

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:240:
'printerColorSpace': colorSettings.color_,
In general we do not allow accessing member vars outside the class without a
getter. See my other comment about how to name constants.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:348: 'color':
colorSettings.getColorMode(),
Replace with "colorSettings.colorMode" after you use the proper get syntax.

[vandebo (ex-Chrome), 2011-09-02 22:38:18]

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:20:
this.printerColorSpace_ = this.color_;
Is this the color model used when printing in color?  A better name might be
along the lines pinterColorModelForColor

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:50: if
(this.printerColorSpace_ == this.color_)
Can you just return this.printerColorSpace_ ?

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:341: printer_color_space =
printing::COLOR;
Does this mean that the bug this is trying to solve doesn't repo on Windows?

http://codereview.chromium.org/7826040/diff/3002/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7826040/diff/3002/printing/print_job_constants...
printing/print_job_constants.h:72: CMYK,
Is there a reason for this enum ordering?  Putting CMYK first seems odd if not.

http://codereview.chromium.org/7826040/diff/3002/printing/printing_context_ma...
File printing/printing_context_mac.mm (left):

http://codereview.chromium.org/7826040/diff/3002/printing/printing_context_ma...
printing/printing_context_mac.mm:251: CFStringRef output_color = color ? NULL :
kGrayColor;
Setting the color model to NULL is color? That's kind of strange...

[kmadhusu, 2011-09-04 01:52:51]

dpapad, vandebo: Please review the latest patch.

Thanks.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:17: this.cmyk_ = 0;
On 2011/09/02 22:15:06, dpapad wrote:
> Since these are supposed to be constants I think it is better to capitalize
the
> names and remove the _. We can start in colorSettings and do the same for any
> other constants already defined in other files later.

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:17: this.cmyk_ = 0;
On 2011/09/02 22:15:06, dpapad wrote:
> Since these are supposed to be constants I think it is better to capitalize
the
> names and remove the _. We can start in colorSettings and do the same for any
> other constants already defined in other files later.

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:20:
this.printerColorSpace_ = this.color_;
On 2011/09/02 22:38:18, vandebo wrote:
> Is this the color model used when printing in color?  A better name might be
> along the lines pinterColorModelForColor

printerColorSpace_ => printerColorModelForColor

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:44: * @return {Number}
Returns the printer color space
On 2011/09/02 22:15:06, dpapad wrote:
> Nit: "Returns" is redundant.

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:46: getColorMode:
function() {
On 2011/09/02 22:15:06, dpapad wrote:
> Could you use the get syntax like above?

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:50: if
(this.printerColorSpace_ == this.color_)
On 2011/09/02 22:38:18, vandebo wrote:
> Can you just return this.printerColorSpace_ ?

oops.. Fixed. Good catch.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:240:
'printerColorSpace': colorSettings.color_,
On 2011/09/02 22:15:06, dpapad wrote:
> In general we do not allow accessing member vars outside the class without a
> getter. See my other comment about how to name constants.

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:348: 'color':
colorSettings.getColorMode(),
On 2011/09/02 22:15:06, dpapad wrote:
> Replace with "colorSettings.colorMode" after you use the proper get syntax.

Done.

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/ui/webui/prin...
File chrome/browser/ui/webui/print_preview_handler.cc (right):

http://codereview.chromium.org/7826040/diff/3002/chrome/browser/ui/webui/prin...
chrome/browser/ui/webui/print_preview_handler.cc:341: printer_color_space =
printing::COLOR;
On 2011/09/02 22:38:18, vandebo wrote:
> Does this mean that the bug this is trying to solve doesn't repo on Windows?

(repeating our conversation for future reference): This bug is repro on Windows.
Users need a seperate ICM to handle CMYK printers. We cannot set it in the
DEVMODE structure.

http://codereview.chromium.org/7826040/diff/3002/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7826040/diff/3002/printing/print_job_constants...
printing/print_job_constants.h:72: CMYK,
On 2011/09/02 22:38:18, vandebo wrote:
> Is there a reason for this enum ordering?  Putting CMYK first seems odd if
not.

On Win, DMCOLOR_COLOR is 2 and DMCOLOR_MONOCHROME is 1. I thought I can try to
use the same value. 

GRAY = 1 & COLOR = 2  

Fixed this by setting a default value for GRAY.

http://codereview.chromium.org/7826040/diff/3002/printing/printing_context_ma...
File printing/printing_context_mac.mm (left):

http://codereview.chromium.org/7826040/diff/3002/printing/printing_context_ma...
printing/printing_context_mac.mm:251: CFStringRef output_color = color ? NULL :
kGrayColor;
On 2011/09/02 22:38:18, vandebo wrote:
> Setting the color model to NULL is color? That's kind of strange...

If I remember correct, NULL will clear the previous print setting. I think, If
there is no "ColorModel" setting in the PMPrintSettings it will default it to
Color.

[dpapad, 2011-09-04 04:55:16]

LGTM for js

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:49: return
this.printerColorModelForColor_;
Nit (optional): return this.bwRadioButton_.checked ? this.GRAY :
this.printerColorModelForColor_;

[vandebo (ex-Chrome), 2011-09-04 17:24:39]

LGTM

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:198: color_mode =kGrayscale;
nit: space

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:201:
gtk_print_settings_set(gtk_settings_, kCUPSColorModel,color_mode);
nit: space

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:18: this.CMYK = 3; //
cmyk - Cyan, magenta, yellow, black
nit: two spaces before an inline comment.

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:19: this.COLOR = 2;
nit: put these in numerical order - gray, color, cmyk

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:49: return
this.printerColorModelForColor_;
On 2011/09/04 04:55:16, dpapad wrote:
> Nit (optional): return this.bwRadioButton_.checked ? this.GRAY :
> this.printerColorModelForColor_;

There's no particular reason to use a tristate here

[kmadhusu, 2011-09-05 17:37:20]

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:198: color_mode =kGrayscale;
On 2011/09/04 17:24:39, vandebo wrote:
> nit: space

Done.

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:201:
gtk_print_settings_set(gtk_settings_, kCUPSColorModel,color_mode);
On 2011/09/04 17:24:39, vandebo wrote:
> nit: space

Done.

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/color_settings.js (right):

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:18: this.CMYK = 3; //
cmyk - Cyan, magenta, yellow, black
On 2011/09/04 17:24:39, vandebo wrote:
> nit: two spaces before an inline comment.

Done.

http://codereview.chromium.org/7826040/diff/7012/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/color_settings.js:19: this.COLOR = 2;
On 2011/09/04 17:24:39, vandebo wrote:
> nit: put these in numerical order - gray, color, cmyk

Done.