PrintPreview: Modified PrintingContextMac code to use the copy of sharedPrintInfo object.

PrintPreview: Modified PrintingContextMac code to use the copy of sharedPrintInfo object.

BUG=82739
TEST=Please refer to bug report.


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

[stuartmorgan, 2011-06-02 21:07:20]

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_mac.h
File printing/printing_context_mac.h (right):

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.h:48: // Initializes PrintSettings from cached
print info object.
s/cached print info object/|print_info_|/

I think it would be worthwhile to add a sentence here like "This must be called
after changes to |print_info_| in order for the changes to take effect in
printing."

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.h:53: // Returns the set of page ranges
constructed from cached print info object.
s/cached print info object/|print_info_|/

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

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:95: ResetPrintInfo([[NSPrintInfo
sharedPrintInfo] copy]);
copy + retain = leak

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:105: ResetPrintInfo([[NSPrintInfo
sharedPrintInfo]copy]);
copy + retain = leak

Also, why was the space removed? It should be there.

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:256: ResetSettings();
I just looked at ResetSettings(), and it doesn't do at all what I expected. This
shouldn't be called in any of the normal flow; compare with
printing_context_win.

So this whole function can be removed.

[kmadhusu, 2011-06-02 23:07:00]

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_mac.h
File printing/printing_context_mac.h (right):

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.h:48: // Initializes PrintSettings from cached
print info object.
On 2011/06/02 21:07:20, stuartmorgan wrote:
> s/cached print info object/|print_info_|/
> 
> I think it would be worthwhile to add a sentence here like "This must be
called
> after changes to |print_info_| in order for the changes to take effect in
> printing."

Done.

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.h:53: // Returns the set of page ranges
constructed from cached print info object.
On 2011/06/02 21:07:20, stuartmorgan wrote:
> s/cached print info object/|print_info_|/

Done.

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

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:95: ResetPrintInfo([[NSPrintInfo
sharedPrintInfo] copy]);
On 2011/06/02 21:07:20, stuartmorgan wrote:
> copy + retain = leak

Fixed.

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:105: ResetPrintInfo([[NSPrintInfo
sharedPrintInfo]copy]);
On 2011/06/02 21:07:20, stuartmorgan wrote:
> copy + retain = leak

Fixed.

> 
> Also, why was the space removed? It should be there.

Fixed.

http://codereview.chromium.org/7074051/diff/1003/printing/printing_context_ma...
printing/printing_context_mac.mm:256: ResetSettings();
On 2011/06/02 21:07:20, stuartmorgan wrote:
> I just looked at ResetSettings(), and it doesn't do at all what I expected.
This
> shouldn't be called in any of the normal flow; compare with
> printing_context_win.
> 
> So this whole function can be removed.

Done.

[stuartmorgan, 2011-06-03 17:12:15]

LGTM

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

http://codereview.chromium.org/7074051/diff/4001/printing/printing_context_ma...
printing/printing_context_mac.mm:105: print_info_.reset([[NSPrintInfo
sharedPrintInfo] copy]);
Maybe add a comment here explain that you are doing this to get a clean slate?

[kmadhusu, 2011-06-03 17:58:54]

Thanks for all your comments.

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

http://codereview.chromium.org/7074051/diff/4001/printing/printing_context_ma...
printing/printing_context_mac.mm:105: print_info_.reset([[NSPrintInfo
sharedPrintInfo] copy]);
On 2011/06/03 17:12:15, stuartmorgan wrote:
> Maybe add a comment here explain that you are doing this to get a clean slate?

Done.

[commit-bot: I haz the power, 2011-06-03 17:59:20]

Presubmit check for 7074051-7001 failed and returned exit status 1.

Running presubmit commit checks ...
Traceback (most recent call last):
  File "/mnt/data/b/commit-queue/verification/presubmit_shim.py", line 34, in
<module>
    sys.exit(presubmit_support.Main(None))
  File "/b/depot_tools/presubmit_support.py", line 1235, in Main
    None)
  File "/b/depot_tools/presubmit_support.py", line 1091, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1006, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 128, in CheckChangeOnCommit
  File "<string>", line 45, in _CommonChecks
  File "/b/depot_tools/presubmit_canned_checks.py", line 890, in
PanProjectChecks
    input_api, output_api, source_file_filter=sources))
  File "/b/depot_tools/presubmit_canned_checks.py", line 743, in CheckOwners
    owners_db.email_regexp)
  File "/b/depot_tools/presubmit_canned_checks.py", line 767, in
_RietveldOwnerAndApprovers
    issue_props = input_api.rietveld.get_issue_properties(
AttributeError: 'NoneType' object has no attribute 'get_issue_properties'
Sending crash report ...
  exception: 'NoneType' object has no attribute 'get_issue_prop
  args: ['/mnt/data/b/commit-queue/verification/presubmit_shim.py', '--commit',
'--author', 'kmadhusu@chromium.org', '--issue', '7074051', '--patchset', '7001',
'--name', '7074051-7001', '--description', 'PrintPreview: Modified
PrintingContextMac code to use the copy of sharedPrintInfo
object.\n\nBUG=82739\nTEST=Please refer to bug report.\n',
'printing/printing_context_mac.h', 'printing/printing_context_mac.mm']
  stack:   File "/mnt/data/b/commit-queue/verification/pres
  host: vm105-m4.golo.chromium.org
  version: 2.6.5 (r265:79063, Apr 16 2010, 13:57:41) 
[GCC 4.
  user: chrome-bot
  cwd: /mnt/data/b/commit-queue/workdir/chromium
A stack trace has been sent to the maintainers.

[M-A Ruel, 2011-06-03 18:44:20]

Fixed bug, reenabling commit bit.

[commit-bot: I haz the power, 2011-06-03 19:27:02]

Change committed as 87828