Gives default PDF settings when no printer is installed.

Gives default PDF settings when no printer is installed.

When there's no printer in system, invalid empty settings will be provided for Print to PDF.  This patch fixes it.

BUG=95975
TEST=none


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

[arthurhsu, 2011-09-27 19:47:20]

ping

[kmadhusu, 2011-09-27 20:28:53]

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.cc
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:26: // Constants for setting default PDF
settings.
nit: Add an empty line before this.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:352: wchar_t paper_buffer[4] = {0};
const int paper_type_count = 4;
wchar_t paper_buffer[paper_type_count] = {0};

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:352: wchar_t paper_buffer[4] = {0};
paper_buffer => paper_type_buffer

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:353: GetLocaleInfo(LOCALE_USER_DEFAULT,
LOCALE_IPAPERSIZE, paper_buffer, 4);
Do we need to get LOCALE_SYSTEM_DEFAULT values if this function call fails?

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:353: GetLocaleInfo(LOCALE_USER_DEFAULT,
LOCALE_IPAPERSIZE, paper_buffer, 4);
4 => arraysize(paper_buffer);

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:383: }
else {
  return OnError();
}

[arthurhsu, 2011-09-27 21:48:36]

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.cc
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:26: // Constants for setting default PDF
settings.
On 2011/09/27 20:28:53, kmadhusu wrote:
> nit: Add an empty line before this.

Done.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:352: wchar_t paper_buffer[4] = {0};
On 2011/09/27 20:28:53, kmadhusu wrote:
> const int paper_type_count = 4;
> wchar_t paper_buffer[paper_type_count] = {0};

Done.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:352: wchar_t paper_buffer[4] = {0};
On 2011/09/27 20:28:53, kmadhusu wrote:
> paper_buffer => paper_type_buffer

Done.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:353: GetLocaleInfo(LOCALE_USER_DEFAULT,
LOCALE_IPAPERSIZE, paper_buffer, 4);
On 2011/09/27 20:28:53, kmadhusu wrote:
> Do we need to get LOCALE_SYSTEM_DEFAULT values if this function call fails?

No.  That will cause UAC.  If we can't, we can't.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:353: GetLocaleInfo(LOCALE_USER_DEFAULT,
LOCALE_IPAPERSIZE, paper_buffer, 4);
On 2011/09/27 20:28:53, kmadhusu wrote:
> 4 => arraysize(paper_buffer);

I use the paper_type_count instead.

http://codereview.chromium.org/8044008/diff/1/printing/printing_context_win.c...
printing/printing_context_win.cc:383: }
On 2011/09/27 20:28:53, kmadhusu wrote:
> else {
>   return OnError();
> }

Based on our discussion, I use LETTER as the default fallback case so that the
IsEmpty() check is no longer needed.

[kmadhusu, 2011-09-28 00:58:28]

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

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:34: const int kPDFLegalHeight = 11 * kPDFDpi;
Legal height is 14.

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:351: if
(settings_.page_setup_device_units().printable_area().IsEmpty()) {
(repeating our conversation for reference): This check is not required. When the
user selects print to pdf or print to cloud, we will always set the paper size
based on locale paper size info.

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:375: // Windows will only return Letter, Legal,
A4, and A3.
nit: This comment is not required.

[arthurhsu, 2011-09-28 01:28:01]

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

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:34: const int kPDFLegalHeight = 11 * kPDFDpi;
On 2011/09/28 00:58:29, kmadhusu wrote:
> Legal height is 14.

Done.

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:351: if
(settings_.page_setup_device_units().printable_area().IsEmpty()) {
On 2011/09/28 00:58:29, kmadhusu wrote:
> (repeating our conversation for reference): This check is not required. When
the
> user selects print to pdf or print to cloud, we will always set the paper size
> based on locale paper size info. 

Done.

http://codereview.chromium.org/8044008/diff/5001/printing/printing_context_wi...
printing/printing_context_win.cc:375: // Windows will only return Letter, Legal,
A4, and A3.
On 2011/09/28 00:58:29, kmadhusu wrote:
> nit: This comment is not required.

Done.

[kmadhusu, 2011-09-28 17:16:09]

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

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:355:
settings_.SetPrinterPrintableArea(paper_size, paper_rect, kPDFDpi);
You want to call SetPrinterPrintableArea() after line 381.

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:358: // Get settings from locale. Paper size
length at most 4.
Paper size length is ambiguous. How about "default paper type count" ?

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:368: paper_rect.SetRect(0, 0, kPDFLegalWidth,
kPDFLegalHeight);
You can just do the following once rather than calling SetRect() in all the
cases.

paper_rect.SetRect(0, 0, paper_size.width(), paper_size.height());

[arthurhsu, 2011-09-28 17:57:39]

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

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:355:
settings_.SetPrinterPrintableArea(paper_size, paper_rect, kPDFDpi);
On 2011/09/28 17:16:10, kmadhusu wrote:
> You want to call SetPrinterPrintableArea() after line 381.

Done.

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:358: // Get settings from locale. Paper size
length at most 4.
On 2011/09/28 17:16:10, kmadhusu wrote:
> Paper size length is ambiguous. How about "default paper type count" ?

paper_type_buffer_len is used instead.

http://codereview.chromium.org/8044008/diff/8001/printing/printing_context_wi...
printing/printing_context_win.cc:368: paper_rect.SetRect(0, 0, kPDFLegalWidth,
kPDFLegalHeight);
On 2011/09/28 17:16:10, kmadhusu wrote:
> You can just do the following once rather than calling SetRect() in all the
> cases.
> 
> paper_rect.SetRect(0, 0, paper_size.width(), paper_size.height());

Done.

[kmadhusu, 2011-09-28 18:07:21]

lgtm with a comment

http://codereview.chromium.org/8044008/diff/11001/printing/printing_context_w...
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/8044008/diff/11001/printing/printing_context_w...
printing/printing_context_win.cc:355: // Get settings from locale. Paper type
buffer length at most 4.
"length is "

[commit-bot: I haz the power, 2011-09-28 20:18:46]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/arthurhsu@chromium.org/8044008/10002

[commit-bot: I haz the power, 2011-09-28 21:44:43]

Change committed as 103187