PrintPreview: Printing preview of a PDF on Mac with Skia only previews the last page of the PDF

PrintPreview: Printing preview of a PDF on Mac with Skia only previews the last page of the PDF

BUG=93145
TEST=Please refer to bug report.

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

[vandebo (ex-Chrome), 2011-09-26 17:33:05]

http://codereview.chromium.org/7719014/diff/17008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7719014/diff/17008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:238: bool is_preview);
metafile is effectively an output parameter, so is_preview should go before it.
Probably best after page_number?

http://codereview.chromium.org/7719014/diff/17008/printing/metafile_skia_wrap...
File printing/metafile_skia_wrapper.h (right):

http://codereview.chromium.org/7719014/diff/17008/printing/metafile_skia_wrap...
printing/metafile_skia_wrapper.h:32: static void SetIsPreviewMetafile(SkCanvas*
canvas, bool is_preview);
These (and DraftMode) should probably live in PlatformDevice.h  (see
Get/SetPlatformDevice).

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:863: if
(printing::MetafileSkiaWrapper::GetMetafileFromCanvas(canvas) &&
Maybe adding an explicit bool variable would make this cleaner:
bool save_for_later =
printing::MetafileSkiaWrapper::GetMetafileFromCanvas(canvas);
#if defined(OS_MACOSX)
save_for_layer = save_for_layer &&
printing::MetafileSkiaWrapper::IsPreviewMetafile(canvas)
#endif

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:442: // The hack is in PrintEnd(),
where a valid |canvas_| is preserved in
Update this comment to indicate that it is used on Mac, but only when doing
preview.

[kmadhusu, 2011-09-26 20:42:22]

http://codereview.chromium.org/7719014/diff/17008/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper.h (right):

http://codereview.chromium.org/7719014/diff/17008/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper.h:238: bool is_preview);
On 2011/09/26 17:33:05, vandebo wrote:
> metafile is effectively an output parameter, so is_preview should go before
it.
> Probably best after page_number?

Done.

http://codereview.chromium.org/7719014/diff/17008/printing/metafile_skia_wrap...
File printing/metafile_skia_wrapper.h (right):

http://codereview.chromium.org/7719014/diff/17008/printing/metafile_skia_wrap...
printing/metafile_skia_wrapper.h:32: static void SetIsPreviewMetafile(SkCanvas*
canvas, bool is_preview);
On 2011/09/26 17:33:05, vandebo wrote:
> These (and DraftMode) should probably live in PlatformDevice.h  (see
> Get/SetPlatformDevice).

Done.

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:863: if
(printing::MetafileSkiaWrapper::GetMetafileFromCanvas(canvas) &&
On 2011/09/26 17:33:05, vandebo wrote:
> Maybe adding an explicit bool variable would make this cleaner:
> bool save_for_later =
> printing::MetafileSkiaWrapper::GetMetafileFromCanvas(canvas);
> #if defined(OS_MACOSX)
> save_for_layer = save_for_layer &&
> printing::MetafileSkiaWrapper::IsPreviewMetafile(canvas)
> #endif

Done.

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/7719014/diff/17008/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:442: // The hack is in PrintEnd(),
where a valid |canvas_| is preserved in
On 2011/09/26 17:33:05, vandebo wrote:
> Update this comment to indicate that it is used on Mac, but only when doing
> preview.

Done.

[vandebo (ex-Chrome), 2011-09-26 21:01:45]

LGTM with comments.

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.cc...
skia/ext/platform_device.cc:46: void SetDraftMode(SkCanvas* canvas, bool
draft_mode) {
You might as well make these generic and pull them into they anonymous
namespace, so you end up with:

SetDraftMode(canvas, mode) {
  SetBoolMetaData(canvas, kDraftModeKey, mode);
}

GetDrafrMode(canvas) {
  return GetBoolMetaData(canvas, kDraftModeKey);
}

etc.

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h#...
skia/ext/platform_device.h:84: SkMetaData& getMetaData(SkCanvas* canvas);
const ref or ptr

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h#...
skia/ext/platform_device.h:85: void SetDraftMode(SkCanvas* canvas, bool
draft_mode);
Should these have Is in the name?

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:873: #endif
// USE_SKIA

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:438: // On all platforms, when
printing to PDF using print preview workflow,
Always when printing on Linux, and when printing for preview on Mac and Win...

[kmadhusu, 2011-09-27 00:35:14]

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.cc...
skia/ext/platform_device.cc:46: void SetDraftMode(SkCanvas* canvas, bool
draft_mode) {
On 2011/09/26 21:01:45, vandebo wrote:
> You might as well make these generic and pull them into they anonymous
> namespace, so you end up with:
> 
> SetDraftMode(canvas, mode) {
>   SetBoolMetaData(canvas, kDraftModeKey, mode);
> }
> 
> GetDrafrMode(canvas) {
>   return GetBoolMetaData(canvas, kDraftModeKey);
> }
> 
> etc.

Done.

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h#...
skia/ext/platform_device.h:84: SkMetaData& getMetaData(SkCanvas* canvas);
On 2011/09/26 21:01:45, vandebo wrote:
> const ref or ptr

Done.

http://codereview.chromium.org/7719014/diff/16007/skia/ext/platform_device.h#...
skia/ext/platform_device.h:85: void SetDraftMode(SkCanvas* canvas, bool
draft_mode);
On 2011/09/26 21:01:45, vandebo wrote:
> Should these have Is in the name?

Done.

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:873: #endif
On 2011/09/26 21:01:45, vandebo wrote:
> // USE_SKIA

Done.

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/7719014/diff/16007/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.h:438: // On all platforms, when
printing to PDF using print preview workflow,
On 2011/09/26 21:01:45, vandebo wrote:
> Always when printing on Linux, and when printing for preview on Mac and Win...

Done.

[vandebo (ex-Chrome), 2011-09-27 16:49:00]

LGTM w/ nits

http://codereview.chromium.org/7719014/diff/25001/printing/metafile_skia_wrap...
File printing/metafile_skia_wrapper.h (right):

http://codereview.chromium.org/7719014/diff/25001/printing/metafile_skia_wrap...
printing/metafile_skia_wrapper.h:23: static void SetMetafileOnCanvas(const
SkCanvas* canvas, Metafile* metafile);
const ref or pointer

http://codereview.chromium.org/7719014/diff/25001/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7719014/diff/25001/skia/ext/platform_device.cc...
skia/ext/platform_device.cc:53: 
nit: extra line

http://codereview.chromium.org/7719014/diff/25001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/25001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:862: // Note: The canvas only has
a metafile on it for print preview.
nit: "Note" is redundant.

[kmadhusu, 2011-09-27 18:15:01]

http://codereview.chromium.org/7719014/diff/25001/printing/metafile_skia_wrap...
File printing/metafile_skia_wrapper.h (right):

http://codereview.chromium.org/7719014/diff/25001/printing/metafile_skia_wrap...
printing/metafile_skia_wrapper.h:23: static void SetMetafileOnCanvas(const
SkCanvas* canvas, Metafile* metafile);
On 2011/09/27 16:49:00, vandebo wrote:
> const ref or pointer

Done.

http://codereview.chromium.org/7719014/diff/25001/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/7719014/diff/25001/skia/ext/platform_device.cc...
skia/ext/platform_device.cc:53: 
On 2011/09/27 16:49:00, vandebo wrote:
> nit: extra line

Done.

http://codereview.chromium.org/7719014/diff/25001/webkit/plugins/ppapi/ppapi_...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/7719014/diff/25001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:862: // Note: The canvas only has
a metafile on it for print preview.
On 2011/09/27 16:49:00, vandebo wrote:
> nit: "Note" is redundant.

Done.

[vandebo (ex-Chrome), 2011-09-27 18:20:54]

LGTM