PrintPreview: [MAC] Set the system default page size in NSPrintInfo PMPageFormat.

PrintPreview: [MAC] Set the system default page size in NSPrintInfo PMPageFormat.

BUG=147411
TEST=Preview a webpage. Make sure the preview page size is same as default page size in print&fax preference pane.


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

[csilv, 2011-11-01 17:34:14]

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:202: bool PrintingContextMac::SetPaperName() {
'SetPaperName' seems like an odd name for this method.  Perhaps
'SetPaperFormat', 'ConfigPaperFormat', or something similar would be a better
choice?

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:234: PMPaper paper = (PMPaper) [(NSArray* )
paper_list objectAtIndex: i];
This can be simplified by using NSArray fast enumeration.  Replace lines 231-233
with:

  for (PMPaper paper in (NSArray*)paper_list) {

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:263: CFStringRef paper_name =
base::SysUTF8ToCFStringRef(paper_name_str);
paper_id and paper_name will leak memory because SysUTF8ToCFStringRef returns a
new object. Instead, replace 260-263 with:

  CFStringRef paper_id = CFSTR("Custom paper ID");
  CFStringRef paper_name = CFSTR("Custom paper");

[kmadhusu, 2011-11-01 20:00:40]

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:202: bool PrintingContextMac::SetPaperName() {
On 2011/11/01 17:34:14, csilv wrote:
> 'SetPaperName' seems like an odd name for this method.  Perhaps
> 'SetPaperFormat', 'ConfigPaperFormat', or something similar would be a better
> choice?

SetPaperName => UpdatePageFormatWithPaperInfo

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:234: PMPaper paper = (PMPaper) [(NSArray* )
paper_list objectAtIndex: i];
On 2011/11/01 17:34:14, csilv wrote:
> This can be simplified by using NSArray fast enumeration.  Replace lines
231-233
> with:
> 
>   for (PMPaper paper in (NSArray*)paper_list) {

As we discussed, I am ignoring this comment.

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:263: CFStringRef paper_name =
base::SysUTF8ToCFStringRef(paper_name_str);
On 2011/11/01 17:34:14, csilv wrote:
> paper_id and paper_name will leak memory because SysUTF8ToCFStringRef returns
a
> new object. Instead, replace 260-263 with:
> 
>   CFStringRef paper_id = CFSTR("Custom paper ID");
>   CFStringRef paper_name = CFSTR("Custom paper");

Done.

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:274: if (PMPaperCreate(current_printer,
paper_id, paper_name,
This code will run on machines whose version is MAC_OS_X_VERSION_10_5 or less.
Do we have any try bot or machines similar to this configuration?

[csilv, 2011-11-01 20:11:59]

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:202: bool PrintingContextMac::SetPaperName() {
Fantastic, thanks.

On 2011/11/01 20:00:41, kmadhusu wrote:
> On 2011/11/01 17:34:14, csilv wrote:
> > 'SetPaperName' seems like an odd name for this method.  Perhaps
> > 'SetPaperFormat', 'ConfigPaperFormat', or something similar would be a
better
> > choice?
> 
> SetPaperName => UpdatePageFormatWithPaperInfo

http://codereview.chromium.org/8426002/diff/5001/printing/printing_context_ma...
printing/printing_context_mac.mm:274: if (PMPaperCreate(current_printer,
paper_id, paper_name,
On 2011/11/01 20:00:41, kmadhusu wrote:
> This code will run on machines whose version is MAC_OS_X_VERSION_10_5 or less.
> Do we have any try bot or machines similar to this configuration?

Oh, thanks for asking about this.  You can remove this section entirely. 
MacChrome has always required 10.5 or later.

[kmadhusu, 2011-11-01 20:41:27]

Addressed review comments.

Thanks.

[csilv, 2011-11-01 20:55:58]

LGTM, just one change to make:

http://codereview.chromium.org/8426002/diff/8001/printing/printing_context_ma...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8426002/diff/8001/printing/printing_context_ma...
printing/printing_context_mac.mm:262: if (PMPaperCreateCustom != NULL) {
Remove this check.  This would only be needed pre-10.5.

[kmadhusu, 2011-11-01 22:06:18]

http://codereview.chromium.org/8426002/diff/8001/printing/printing_context_ma...
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/8426002/diff/8001/printing/printing_context_ma...
printing/printing_context_mac.mm:262: if (PMPaperCreateCustom != NULL) {
On 2011/11/01 20:55:58, csilv wrote:
> Remove this check.  This would only be needed pre-10.5.

Done.

[commit-bot: I haz the power, 2011-11-02 01:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/8426002/6004

[commit-bot: I haz the power, 2011-11-02 02:32:58]

Change committed as 108235

[Vitaly Buka (NO REVIEWS), 2012-09-08 02:41:59]

I have attached new bug to help understand the reason for this change.

On 2011/11/02 02:32:58, I haz the power (commit-bot) wrote:
> Change committed as 108235