PrintPreview: Added code to identify the printer default duplex value.

PrintPreview: Added code to identify the printer default duplex value.

Added code to identify the printer default duplex value. If we are
unable to get the default value, hide the two sided option in the preview tab.

BUG=89204
TEST=Please refer to bug description.

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

[kmadhusu, 2011-09-02 02:08:45]

Previously, we were just sending the user selected duplex value (SIMPLEX or
LONG_EDGE) from the js to the c++ code.

I have added some code to get the printer default duplex value. It can be
UNKNOWN, ONE_SIDED or TWO_SIDED depending on what we can retrieve from the
printer default data. Now , I am sending an object with two properties
(printer_default_duplex_value and user_selected_duplex_value) from js to the c++
code. 

On win, if the printer default duplex value is UNKNOWN, we restrict the value of
dm_duplex to 0 and 1. These are the known valid values this setting take.

On Linux and Mac we don't see any issues as of now. Therefore I am not checking
the printer default duplex value and leaving the code as it was before. 

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

Thanks.

[dpapad, 2011-09-02 03:01:07]

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:26:
this.printerDefaultDuplexValue = -1;
Rename member var to printerDefaultDuplexValue_.

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:62: * @return {object}
duplex mode object specifies the printer specified
Nit: s/{object}/{Object}
Nit (optional): Change @return comment to "A custom object holding the printer
default value and user selected duplex mode value."

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:68: var SIMPLEX = 0;
You can define these constants as regular member vars instead of local, this is
what we do with other constants for example |this.maxValue_| and
|this.minValue_|.
Maybe at some point we should rename to this,MAX_VALUE etc to make it more clear
that they are supposed to be constant.

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:168: if
(this.printerDefaultDuplexValue >= 0)
I think this if statement is equivalent to
this.twoSidedCheckbox_.checked = !!this.printerDefaultDuplexValue;

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

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:239:
'printerDefaultDuplexValue': 0,
Could you use copiesSettings.simplex_ here instead of 0?

[vandebo (ex-Chrome), 2011-09-02 18:12:49]

http://codereview.chromium.org/7817013/diff/2001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7817013/diff/2001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_browsertest.cc:39: 
Probably should add a test for the new code you added.

http://codereview.chromium.org/7817013/diff/2001/printing/printing_context_wi...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/7817013/diff/2001/printing/printing_context_wi...
printing/printing_context_win.cc:379: dev_mode->dmDuplex =
user_selected_duplex_value ? DMDUP_HORIZONTAL :
This assumes that SIMPLEX is "false."  Instead, you could fix up
dev_mode->dmDuplux after the switch, i.e. if vertical isn't supported, change it
to horizontal.

A comment and reference to a bug number wouldn't hurt.

[kmadhusu, 2011-09-03 00:34:00]

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:26:
this.printerDefaultDuplexValue = -1;
On 2011/09/02 03:01:07, dpapad wrote:
> Rename member var to printerDefaultDuplexValue_.

Done.

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:62: * @return {object}
duplex mode object specifies the printer specified
On 2011/09/02 03:01:07, dpapad wrote:
> Nit: s/{object}/{Object}
> Nit (optional): Change @return comment to "A custom object holding the printer
> default value and user selected duplex mode value."

Reverted the code to return a number.

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:68: var SIMPLEX = 0;
On 2011/09/02 03:01:07, dpapad wrote:
> You can define these constants as regular member vars instead of local, this
is
> what we do with other constants for example |this.maxValue_| and
> |this.minValue_|.
> Maybe at some point we should rename to this,MAX_VALUE etc to make it more
clear
> that they are supposed to be constant.

Done.

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:168: if
(this.printerDefaultDuplexValue >= 0)
On 2011/09/02 03:01:07, dpapad wrote:
> I think this if statement is equivalent to
> this.twoSidedCheckbox_.checked = !!this.printerDefaultDuplexValue;

No. this.printerDefaultDuplexValue is not a boolean. It is a tri-state variable
(UNKNOWN = -1, SIMPLEX = 0, DUPLEX = 1)

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

http://codereview.chromium.org/7817013/diff/2001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/print_preview.js:239:
'printerDefaultDuplexValue': 0,
On 2011/09/02 03:01:07, dpapad wrote:
> Could you use copiesSettings.simplex_ here instead of 0?

Done.

http://codereview.chromium.org/7817013/diff/2001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_browsertest.cc (right):

http://codereview.chromium.org/7817013/diff/2001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_browsertest.cc:39: 
On 2011/09/02 18:12:50, vandebo wrote:
> Probably should add a test for the new code you added.

I have added a js test.

http://codereview.chromium.org/7817013/diff/2001/printing/printing_context_wi...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/7817013/diff/2001/printing/printing_context_wi...
printing/printing_context_win.cc:379: dev_mode->dmDuplex =
user_selected_duplex_value ? DMDUP_HORIZONTAL :
On 2011/09/02 18:12:50, vandebo wrote:
> This assumes that SIMPLEX is "false."  Instead, you could fix up
> dev_mode->dmDuplux after the switch, i.e. if vertical isn't supported, change
it
> to horizontal.
> 
> A comment and reference to a bug number wouldn't hurt.

As we discussed, I reverted the printing context code changes. JS will now
return SHORT_EDGE if the duplex value is unknown. Added a comment and bug
reference in js.

[vandebo (ex-Chrome), 2011-09-03 00:58:26]

LGTM with nits.

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:76: // Ref bug:
http://code.google.com/p/chromium/issues/detail?id=89204
http://crbug.com/89204

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:77: return
!this.twoSidedCheckbox_.checked ?
Double trinary is pretty ugly... how about if, else if, else

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:80: this.SHORT_EDGE :
this.LONG_EDGE);
I wonder if it would be clearer to define this.DUPLEX = 1, and use that here.

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:176: if
(this.printerDefaultDuplexValue >= 0)
!= UNKNOWN ?

[kmadhusu, 2011-09-03 02:44:53]

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:76: // Ref bug:
http://code.google.com/p/chromium/issues/detail?id=89204
On 2011/09/03 00:58:26, vandebo wrote:
> http://crbug.com/89204

Done.

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:77: return
!this.twoSidedCheckbox_.checked ?
On 2011/09/03 00:58:27, vandebo wrote:
> Double trinary is pretty ugly... how about if, else if, else

Done.

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:80: this.SHORT_EDGE :
this.LONG_EDGE);
On 2011/09/03 00:58:27, vandebo wrote:
> I wonder if it would be clearer to define this.DUPLEX = 1, and use that here.

Yeah it will be consistent with the comment. Done.

http://codereview.chromium.org/7817013/diff/9001/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:176: if
(this.printerDefaultDuplexValue >= 0)
On 2011/09/03 00:58:27, vandebo wrote:
> != UNKNOWN ?

Done.

[dpapad, 2011-09-03 04:48:00]

LGTM

[vandebo (ex-Chrome), 2011-09-04 17:06:53]

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:72: // On Windows,
some printer doesn't specify their duplex values in the
nit: printer doesn't => printers don't

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:80: return
this.DUPLEX;
Wait, I'm confused, this.DUPLEX and this.LONG_EDGE have the same value.

[kmadhusu, 2011-09-07 20:57:33]

If the printer default duplex value is unknown, hide the two sided option in
preview tab UI.

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

Thanks.

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:72: // On Windows,
some printer doesn't specify their duplex values in the
On 2011/09/04 17:06:53, vandebo wrote:
> nit: printer doesn't => printers don't

Done.

http://codereview.chromium.org/7817013/diff/9003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:80: return
this.DUPLEX;
On 2011/09/04 17:06:53, vandebo wrote:
> Wait, I'm confused, this.DUPLEX and this.LONG_EDGE have the same value.

It was a typo.. I have removed DUPLEX now.

[vandebo (ex-Chrome), 2011-09-07 21:55:57]

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/copies_settings.js:72: else if
(!this.twoSidedCheckbox_.checked)
nit: remove the ! and switch the bodies

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:242:
'printerDefaultDuplexValue': copiesSettings.UNKNOWN,
copiesSetting.UNKNOWN is kind of a strange value for DefaultDuplexValue.  How
about making it something like NO_DUPLEXER or UNSUPPORTED_DUPLEXER

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

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:285: int 
default_duplex_setting_value = printing::UNKNOWN;
nit: extra space

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:311: ppd_choice_t* ch =
ppdFindMarkedChoice(ppd, kDuplex);
Can you fix ch?  We don't use abbreviated variable names.  I assume this is
supposed to be choice?  Though I see ch->choice latter, maybe duplex_choice?

http://codereview.chromium.org/7817013/diff/19001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7817013/diff/19001/printing/print_job_constant...
printing/print_job_constants.h:58: enum DefaultDuplexSetting {
Can we use the DuplexMode enum instead of introducing another enum with similar
values?  Maybe add DUPLEX_UNSUPPORTED as an option there?

[kmadhusu, 2011-09-07 23:21:56]

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/copies_settings.js:72: else if
(!this.twoSidedCheckbox_.checked)
On 2011/09/07 21:55:57, vandebo wrote:
> nit: remove the ! and switch the bodies

Done.

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/print_preview.js (right):

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/print_preview.js:242:
'printerDefaultDuplexValue': copiesSettings.UNKNOWN,
On 2011/09/07 21:55:57, vandebo wrote:
> copiesSetting.UNKNOWN is kind of a strange value for DefaultDuplexValue.  How
> about making it something like NO_DUPLEXER or UNSUPPORTED_DUPLEXER

copiesSetting.UNKNOWN_DUPLEX_MODE;

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

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:285: int 
default_duplex_setting_value = printing::UNKNOWN;
On 2011/09/07 21:55:57, vandebo wrote:
> nit: extra space

Done.

http://codereview.chromium.org/7817013/diff/19001/chrome/browser/ui/webui/pri...
chrome/browser/ui/webui/print_preview_handler.cc:311: ppd_choice_t* ch =
ppdFindMarkedChoice(ppd, kDuplex);
On 2011/09/07 21:55:57, vandebo wrote:
> Can you fix ch?  We don't use abbreviated variable names.  I assume this is
> supposed to be choice?  Though I see ch->choice latter, maybe duplex_choice?

Done.

http://codereview.chromium.org/7817013/diff/19001/printing/print_job_constants.h
File printing/print_job_constants.h (right):

http://codereview.chromium.org/7817013/diff/19001/printing/print_job_constant...
printing/print_job_constants.h:58: enum DefaultDuplexSetting {
On 2011/09/07 21:55:57, vandebo wrote:
> Can we use the DuplexMode enum instead of introducing another enum with
similar
> values?  Maybe add DUPLEX_UNSUPPORTED as an option there?

Added UNKNOWN_DUPLEX_MODE in DuplexMode enum.

[vandebo (ex-Chrome), 2011-09-07 23:32:22]

LGTM

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

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:203: const char* cups_duplex_mode;
Put this entire block in the if(not unknown) block

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/copies_settings.js:203: * Takes care of
showing/hideing the two sided option and also updates the
nit: hiding

[kmadhusu, 2011-09-07 23:43:48]

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

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:203: const char* cups_duplex_mode;
On 2011/09/07 23:32:22, vandebo wrote:
> Put this entire block in the if(not unknown) block

Done.

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/resources/pr...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/27001/chrome/browser/resources/pr...
chrome/browser/resources/print_preview/copies_settings.js:203: * Takes care of
showing/hideing the two sided option and also updates the
On 2011/09/07 23:32:22, vandebo wrote:
> nit: hiding

oops..Fixed..

[vandebo (ex-Chrome), 2011-09-08 00:03:57]

LGTM

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

http://codereview.chromium.org/7817013/diff/26018/chrome/browser/printing/pri...
chrome/browser/printing/print_dialog_gtk.cc:216: break;
nit: add a NOTREACHED or remove the default

[dpapad, 2011-09-08 15:52:06]

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:75: return
this.SIMPLEX;
Nit: you could compress this if statement to
if (this.twoSidedOption_.hidden)
  return this.UNKNOWN_DUPLEX_MODE;
else
  return this.twoSidedCheckbox_.checked ? LONG_EDGE : SIMPLEX;

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:214:
(defaultDuplexValue == this.UNKNOWN_DUPLEX_MODE);
Is |defaultDuplexValue| a boolean? It is compared with a non boolean here. If it
is not, update the @param comment otherwise use !!this.UNKNOWN_DUPLEX_MODE.

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:218:
this.twoSidedCheckbox_.checked = defaultDuplexValue;
Same here, if |defaultDuplexValue| is not a boolean use !!defaultDuplexValue.

[kmadhusu, 2011-09-08 17:13:10]

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
File chrome/browser/resources/print_preview/copies_settings.js (right):

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:75: return
this.SIMPLEX;
On 2011/09/08 15:52:06, dpapad wrote:
> Nit: you could compress this if statement to
> if (this.twoSidedOption_.hidden)
>   return this.UNKNOWN_DUPLEX_MODE;
> else
>   return this.twoSidedCheckbox_.checked ? LONG_EDGE : SIMPLEX;

I prefer if..elseif.. structure here.

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:214:
(defaultDuplexValue == this.UNKNOWN_DUPLEX_MODE);
On 2011/09/08 15:52:06, dpapad wrote:
> Is |defaultDuplexValue| a boolean? It is compared with a non boolean here. If
it
> is not, update the @param comment otherwise use !!this.UNKNOWN_DUPLEX_MODE.

It is not a boolean. Updated the @param comment.

http://codereview.chromium.org/7817013/diff/5003/chrome/browser/resources/pri...
chrome/browser/resources/print_preview/copies_settings.js:218:
this.twoSidedCheckbox_.checked = defaultDuplexValue;
On 2011/09/08 15:52:06, dpapad wrote:
> Same here, if |defaultDuplexValue| is not a boolean use !!defaultDuplexValue.

Done.

[dpapad, 2011-09-08 17:32:16]

LGTM