Set the job name for the print job on the Mac....

Set the job name for the print job on the Mac.

BUG=http://crbug.com/29188
TEST=as in bug


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

[Avi (use Gerrit), 2010-05-12 15:30:50]

Slight code update.

[stuartmorgan, 2010-05-12 15:39:51]

http://codereview.chromium.org/1997016/diff/29001/30006
File gfx/native_widget_types.h (right):

http://codereview.chromium.org/1997016/diff/29001/30006#newcode94
gfx/native_widget_types.h:94: // things like sheet hosting. On other platforms,
the renderer doesn't have
I'm a nervous about turning this on for the Mac, given that it has very
different semantics from the Windows version. The comment you have here doesn't
actually explain that ("On other platforms" implies that on the Mac the renderer
*does* have access to the native view), but even if it did it would be way too
easy for some to miss it and make the reasonable assumption that the same method
could be used basically the same way across platforms.

I would rather have a separate method with a very clear explanation, or just a
raw cast.

http://codereview.chromium.org/1997016/diff/29001/30010
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/1997016/diff/29001/30010#newcode167
printing/printing_context_win.cc:167: }
DCHECK(window) ?

[Avi (use Gerrit), 2010-05-12 16:35:20]

http://codereview.chromium.org/1997016/diff/29001/30006
File gfx/native_widget_types.h (right):

http://codereview.chromium.org/1997016/diff/29001/30006#newcode94
gfx/native_widget_types.h:94: // things like sheet hosting. On other platforms,
the renderer doesn't have
I'm not entirely sure what the NativeViewId is used for. We're obviously passing
it in on the Mac (see IdFromNativeView below). I can rephrase things, but if the
data's in there...

http://codereview.chromium.org/1997016/diff/29001/30010
File printing/printing_context_win.cc (right):

http://codereview.chromium.org/1997016/diff/29001/30010#newcode167
printing/printing_context_win.cc:167: }
On 2010/05/12 15:39:52, stuartmorgan wrote:
> DCHECK(window) ?

Done.

[stuartmorgan, 2010-05-12 16:39:56]

http://codereview.chromium.org/1997016/diff/29001/30006
File gfx/native_widget_types.h (right):

http://codereview.chromium.org/1997016/diff/29001/30006#newcode94
gfx/native_widget_types.h:94: // things like sheet hosting. On other platforms,
the renderer doesn't have
On 2010/05/12 16:35:20, Avi wrote:
> but if the data's in there...

... then when you try to use it in another process on the Mac, the way you can
on Windows, then you will crash horribly.

Which is why I don't think making it easy for someone to think that will work is
a good idea.

[Avi (use Gerrit), 2010-05-12 16:45:07]

On 2010/05/12 16:39:56, stuartmorgan wrote:
> ... then when you try to use it in another process on the Mac, the way you can
> on Windows, then you will crash horribly.

Yes, that's why we're typedef-ing it to NativeViewId, so it doesn't work from
the other process.

[Avi (use Gerrit), 2010-05-12 16:45:49]

Updated the comment anyway.

[stuartmorgan, 2010-05-12 18:54:43]

LGTM

http://codereview.chromium.org/1997016/diff/44002/46006
File gfx/native_widget_types.h (right):

http://codereview.chromium.org/1997016/diff/44002/46006#newcode108
gfx/native_widget_types.h:108:
NATIVE_VIEW_FROM_ID_NOT_AVAILABLE_OUTSIDE_BROWSER_ON_MAC
Is this line really necessary? Not having a definition at all, along with the
comment here, seems sufficient.

http://codereview.chromium.org/1997016/diff/44002/46009
File printing/printing_context_mac.mm (right):

http://codereview.chromium.org/1997016/diff/44002/46009#newcode53
printing/printing_context_mac.mm:53: PMPrintSettingsSetJobName(printSettings,
(CFStringRef)job_title);
Do you need to nil-check job_title before calling this?

[jeremy, 2010-05-12 18:58:30]

I think the bug number in the CL description is wrong :)

[Avi (use Gerrit), 2010-05-12 19:00:00]

On 2010/05/12 18:58:30, jeremy wrote:
> I think the bug number in the CL description is wrong :)

Nice catch; thanks!