Unifying NativeMetafile class interface (as much as possible) for Linux, Mac, Win

Unifying NativeMetafile class interface (as much as possible) for Linux, Mac, Win

BUG=NONE
TEST=NONE

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

[dpapad, 2011-03-03 23:54:06]

http://codereview.chromium.org/6611032/diff/1/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/1/printing/emf_win.cc#newcode129
printing/emf_win.cc:129: 
I added this private function as an alias of the public GetPageBounds(unsigned
int), since the argument passed is ignored anyway. Alternatively, It could be
removed by replacing every occurrence of GetPageBounds() with GetPageBounds(1).

[Lei Zhang, 2011-03-04 12:02:36]

http://codereview.chromium.org/6611032/diff/4003/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6611032/diff/4003/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:159: if ((!error) &&
(!metafile->SaveTo(path_to_pdf_))) {
nit: no need to write:
  if (!foo && !bar)
as:
  if ((!foo) && (!bar))

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:41: virtual ~NativeMetafile() {};
nit: no ; needed.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:44: // rendering into the PDF. Returns false on
failure.
Comment is wrong. You don't return a context here. Also, Windows does not always
use PDF. Many more instances of referring to metadata format as PDF below.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:51: // Note: It should only be  called from within
the browser process.
nit: 2 spaces after 'be'.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:59: // |dst_buffer|. This function should ONLY be
called after Close() is invoked.
s/steam/stream/

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:78: virtual gfx::Rect GetPageBounds(unsigned int
page_number) const = 0;
Not sure if this makes sense when the Windows impl does not use |page_number|.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:83: 
nit: remove blank line

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_metafile_mac_un...
File printing/pdf_metafile_mac_unittest.cc (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_metafile_mac_un...
printing/pdf_metafile_mac_unittest.cc:20: pdf.Init();
probably good to make sure this returns true.

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:16: // Implementing NativeMetafile
nit: Unnecessary comment. Same for all the other metafile formats. The old
comments are more descriptive.

http://codereview.chromium.org/6611032/diff/4003/printing/printed_document.cc
File printing/printed_document.cc (right):

http://codereview.chromium.org/6611032/diff/4003/printing/printed_document.cc...
printing/printed_document.cc:262:
g_debug_dump_info.Get().debug_dump_path.Append(filename).value()));
I haven't looked closely, but I suspect
g_debug_dump_info.Get().debug_dump_path.Append(filename) is a FilePath. You
don't have to get its value() and construct another FilePath.

[dpapad, 2011-03-04 19:19:43]

http://codereview.chromium.org/6611032/diff/4003/chrome/browser/printing/prin...
File chrome/browser/printing/print_dialog_gtk.cc (right):

http://codereview.chromium.org/6611032/diff/4003/chrome/browser/printing/prin...
chrome/browser/printing/print_dialog_gtk.cc:159: if ((!error) &&
(!metafile->SaveTo(path_to_pdf_))) {
On 2011/03/04 12:02:36, Lei Zhang wrote:
> nit: no need to write:
>   if (!foo && !bar)
> as:
>   if ((!foo) && (!bar))

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:41: virtual ~NativeMetafile() {};
On 2011/03/04 12:02:36, Lei Zhang wrote:
> nit: no ; needed.

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:44: // rendering into the PDF. Returns false on
failure.
On 2011/03/04 12:02:36, Lei Zhang wrote:
> Comment is wrong. You don't return a context here. Also, Windows does not
always
> use PDF. Many more instances of referring to metadata format as PDF below.

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:51: // Note: It should only be  called from within
the browser process.
On 2011/03/04 12:02:36, Lei Zhang wrote:
> nit: 2 spaces after 'be'.

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:59: // |dst_buffer|. This function should ONLY be
called after Close() is invoked.
On 2011/03/04 12:02:36, Lei Zhang wrote:
> s/steam/stream/

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:78: virtual gfx::Rect GetPageBounds(unsigned int
page_number) const = 0;
On 2011/03/04 12:02:36, Lei Zhang wrote:
> Not sure if this makes sense when the Windows impl does not use |page_number|.

I had the same concern. It will make more sense later when newer implementations
of native_metafile.h will use the |page_number|. For the moment I could add a
DCHECK(page_number == 1) for windows.

http://codereview.chromium.org/6611032/diff/4003/printing/native_metafile.h#n...
printing/native_metafile.h:83: 
On 2011/03/04 12:02:36, Lei Zhang wrote:
> nit: remove blank line

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_metafile_mac_un...
File printing/pdf_metafile_mac_unittest.cc (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_metafile_mac_un...
printing/pdf_metafile_mac_unittest.cc:20: pdf.Init();
On 2011/03/04 12:02:36, Lei Zhang wrote:
> probably good to make sure this returns true.

Done.

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:16: // Implementing NativeMetafile
On 2011/03/04 12:02:36, Lei Zhang wrote:
> nit: Unnecessary comment. Same for all the other metafile formats. The old
> comments are more descriptive.

When attempting to unify the interfaces, the comments were merged and moved to
native_metafile.h (Some platform specific comments have been kept in
pdf_metafile_mac.h/emf_win.h/pdf_ps_metafile_cairo.h). Also, in a previous CL
you suggested this -->
http://codereview.chromium.org/6544028/diff/38071/printing/pdf_ps_metafile_ca...

http://codereview.chromium.org/6611032/diff/4003/printing/printed_document.cc
File printing/printed_document.cc (right):

http://codereview.chromium.org/6611032/diff/4003/printing/printed_document.cc...
printing/printed_document.cc:262:
g_debug_dump_info.Get().debug_dump_path.Append(filename).value()));
On 2011/03/04 12:02:36, Lei Zhang wrote:
> I haven't looked closely, but I suspect
> g_debug_dump_info.Get().debug_dump_path.Append(filename) is a FilePath. You
> don't have to get its value() and construct another FilePath.

Done.

[Lei Zhang, 2011-03-04 19:36:40]

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:16: // Implementing NativeMetafile
On 2011/03/04 19:19:43, dpapad wrote:
> On 2011/03/04 12:02:36, Lei Zhang wrote:
> > nit: Unnecessary comment. Same for all the other metafile formats. The old
> > comments are more descriptive.
> 
> When attempting to unify the interfaces, the comments were merged and moved to
> native_metafile.h (Some platform specific comments have been kept in
> pdf_metafile_mac.h/emf_win.h/pdf_ps_metafile_cairo.h). Also, in a previous CL
> you suggested this -->
>
http://codereview.chromium.org/6544028/diff/38071/printing/pdf_ps_metafile_ca...

Imagine someone who has never read the code before. They don't know what
"PdfPsMetafile" is based on just the class name. The comment that was here is a
good description. With your change, that knowledge has been lost. It hasn't been
transfered into native_metafile.h either.

[dpapad, 2011-03-04 21:48:19]

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/4003/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:16: // Implementing NativeMetafile
On 2011/03/04 19:36:40, Lei Zhang wrote:
> On 2011/03/04 19:19:43, dpapad wrote:
> > On 2011/03/04 12:02:36, Lei Zhang wrote:
> > > nit: Unnecessary comment. Same for all the other metafile formats. The old
> > > comments are more descriptive.
> > 
> > When attempting to unify the interfaces, the comments were merged and moved
to
> > native_metafile.h (Some platform specific comments have been kept in
> > pdf_metafile_mac.h/emf_win.h/pdf_ps_metafile_cairo.h). Also, in a previous
CL
> > you suggested this -->
> >
>
http://codereview.chromium.org/6544028/diff/38071/printing/pdf_ps_metafile_ca...
> 
> Imagine someone who has never read the code before. They don't know what
> "PdfPsMetafile" is based on just the class name. The comment that was here is
a
> good description. With your change, that knowledge has been lost. It hasn't
been
> transfered into native_metafile.h either.

Done.

[vandebo (ex-Chrome), 2011-03-08 00:39:35]

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode1
printing/emf_win.h:1: // Copyright (c) 2011 The Chromium Authors. All rights
reserved.
You should generalize comments made in this file to the other platform files.

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode19
printing/emf_win.h:19: // Implementing NativeMetafile
This comment isn't useful.

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode28
printing/emf_win.h:28: virtual bool Init() { return true; }
Comment here: //NativeMetafile methods.

Put these in the same order as in native_metafile.h

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode81
printing/emf_win.h:81: gfx::Rect Emf::GetPageBounds() const;
No overloading.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (left):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:9: #include "build/build_config.h"
Still need this, no?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:24: namespace gfx {
These can be done outside of a platform ifdef

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:30: typedef struct _cairo_surface cairo_surface_t;
cairo_surface_t isn't used here.  What does use it?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:31: typedef struct _cairo cairo_t;
Looks like a per platform typedef might be useful...
PlatformContext:
linux -> _cairo
win -> HDC
mac -> CGContextRef

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:38: // This class creates a graphics context that
renders into a PDF data stream.
It doesn't always render to a PDF data stream.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:67: // Closes the metafile. No further rendering is
allowed.
Maybe note that this implicitly finishes the current page?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:72: virtual bool SaveTo(const FilePath& file_path)
const = 0;
Is this only used by tests?  If so, we can make it protected and use friend
tests?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:76: virtual gfx::Rect GetPageBounds(unsigned int
page_number) const = 0;
Lets make these not platform specific.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:86: virtual bool CreateDc(HDC sibling, const RECT*
rect) = 0;
It'd be great to know if these methods are specific to the way we need to print
on windows, or if they are specific to EMF.  Maybe we'll only figure that out
when when we have a non EMF metafile implementation.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:93: // TODO(maruel): CreateFromFile(). If ever used.
Maybe users would like to
Is this ever used?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:117: virtual bool SafePlayback(HDC hdc) const = 0;
Are both of the playback methods still used?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:125: virtual bool StartPage() = 0;
For methods that are the same on the different platforms except for their
arguments etc., let's put them in their own ifdef block with just the one
method.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:171: virtual cairo_t* StartPage(double
width_in_points,
These arguments could be turned into two gfx::Size values.  Not sure if that
would get you close enough to the mac interface to unite them.

http://codereview.chromium.org/6611032/diff/10024/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/10024/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:227: if (file_util::WriteFile(file_path,
Fits on one line now.

[dpapad, 2011-03-08 19:39:22]

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode19
printing/emf_win.h:19: // Implementing NativeMetafile
On 2011/03/08 00:39:35, vandebo wrote:
> This comment isn't useful.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode28
printing/emf_win.h:28: virtual bool Init() { return true; }
On 2011/03/08 00:39:35, vandebo wrote:
> Comment here: //NativeMetafile methods.
> 
> Put these in the same order as in native_metafile.h

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/emf_win.h#newcode81
printing/emf_win.h:81: gfx::Rect Emf::GetPageBounds() const;
On 2011/03/08 00:39:35, vandebo wrote:
> No overloading.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (left):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:9: #include "build/build_config.h"
On 2011/03/08 00:39:35, vandebo wrote:
> Still need this, no?

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:24: namespace gfx {
On 2011/03/08 00:39:35, vandebo wrote:
> These can be done outside of a platform ifdef

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:30: typedef struct _cairo_surface cairo_surface_t;
On 2011/03/08 00:39:35, vandebo wrote:
> cairo_surface_t isn't used here.  What does use it?

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:31: typedef struct _cairo cairo_t;
On 2011/03/08 00:39:35, vandebo wrote:
> Looks like a per platform typedef might be useful...
> PlatformContext:
> linux -> _cairo
> win -> HDC
> mac -> CGContextRef

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:38: // This class creates a graphics context that
renders into a PDF data stream.
On 2011/03/08 00:39:35, vandebo wrote:
> It doesn't always render to a PDF data stream.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:67: // Closes the metafile. No further rendering is
allowed.
On 2011/03/08 00:39:35, vandebo wrote:
> Maybe note that this implicitly finishes the current page?

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:72: virtual bool SaveTo(const FilePath& file_path)
const = 0;
On 2011/03/08 00:39:35, vandebo wrote:
> Is this only used by tests?  If so, we can make it protected and use friend
> tests?

No, it is used in non-test code (print_dialog_gtk.h:159).

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:76: virtual gfx::Rect GetPageBounds(unsigned int
page_number) const = 0;
On 2011/03/08 00:39:35, vandebo wrote:
> Lets make these not platform specific.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:86: virtual bool CreateDc(HDC sibling, const RECT*
rect) = 0;
On 2011/03/08 00:39:35, vandebo wrote:
> It'd be great to know if these methods are specific to the way we need to
print
> on windows, or if they are specific to EMF.  Maybe we'll only figure that out
> when when we have a non EMF metafile implementation.

These functions call internaly CreateEnhMetaFile(...). The description of that
method is (taken from the Windows dcoumentation)
"The CreateEnhMetaFile function creates a device context for an enhanced-format
metafile. This device context can be used to store a device-independent
picture."

I am not sure if they are useful for non-EMF related operations.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:93: // TODO(maruel): CreateFromFile(). If ever used.
Maybe users would like to
On 2011/03/08 00:39:35, vandebo wrote:
> Is this ever used?

No, it is not used anywhere. Should I remove it?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:117: virtual bool SafePlayback(HDC hdc) const = 0;
On 2011/03/08 00:39:35, vandebo wrote:
> Are both of the playback methods still used?

They are both used (but only in emf_win_unittest.cc).

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:125: virtual bool StartPage() = 0;
On 2011/03/08 00:39:35, vandebo wrote:
> For methods that are the same on the different platforms except for their
> arguments etc., let's put them in their own ifdef block with just the one
> method.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:171: virtual cairo_t* StartPage(double
width_in_points,
On 2011/03/08 00:39:35, vandebo wrote:
> These arguments could be turned into two gfx::Size values.  Not sure if that
> would get you close enough to the mac interface to unite them.

gfx::Size stores width and height as integers. Here width and height are doubles
because that is what function cairo_pdf_surface_set_size(...), which is called
internally, expects. If there is no need for them to be doubles I could convert
them right before calling cairo_pdf_surface_set_size to ints, but I am not sure
that this is the case here.

http://codereview.chromium.org/6611032/diff/10024/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/10024/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:227: if (file_util::WriteFile(file_path,
On 2011/03/08 00:39:35, vandebo wrote:
> Fits on one line now.

Done.

[vandebo (ex-Chrome), 2011-03-08 20:16:53]

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:86: virtual bool CreateDc(HDC sibling, const RECT*
rect) = 0;
On 2011/03/08 19:39:22, dpapad wrote:
> On 2011/03/08 00:39:35, vandebo wrote:
> > It'd be great to know if these methods are specific to the way we need to
> print
> > on windows, or if they are specific to EMF.  Maybe we'll only figure that
out
> > when when we have a non EMF metafile implementation.
> 
> These functions call internaly CreateEnhMetaFile(...). The description of that
> method is (taken from the Windows dcoumentation)
> "The CreateEnhMetaFile function creates a device context for an
enhanced-format
> metafile. This device context can be used to store a device-independent
> picture."
> 
> I am not sure if they are useful for non-EMF related operations.

That makes it sound like they are EMF specific.  Can you look at it more and see
if we can remove them from the nativemetafile interface and just have them in
the emf implementation?

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:93: // TODO(maruel): CreateFromFile(). If ever used.
Maybe users would like to
On 2011/03/08 19:39:22, dpapad wrote:
> On 2011/03/08 00:39:35, vandebo wrote:
> > Is this ever used?
> 
> No, it is not used anywhere. Should I remove it?

Yea, we like to remove dead code.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:117: virtual bool SafePlayback(HDC hdc) const = 0;
On 2011/03/08 19:39:22, dpapad wrote:
> On 2011/03/08 00:39:35, vandebo wrote:
> > Are both of the playback methods still used?
> 
> They are both used (but only in emf_win_unittest.cc).

If only used in emf unit tests, then they can be moved out of the nativemetafile
interface.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:171: virtual cairo_t* StartPage(double
width_in_points,
On 2011/03/08 19:39:22, dpapad wrote:
> On 2011/03/08 00:39:35, vandebo wrote:
> > These arguments could be turned into two gfx::Size values.  Not sure if that
> > would get you close enough to the mac interface to unite them.
> 
> gfx::Size stores width and height as integers. Here width and height are
doubles
> because that is what function cairo_pdf_surface_set_size(...), which is called
> internally, expects. If there is no need for them to be doubles I could
convert
> them right before calling cairo_pdf_surface_set_size to ints, but I am not
sure
> that this is the case here.

These are in points, it's ok to round these values by 1/72nd of an inch. i.e.
ints will work fine.

[dpapad, 2011-03-08 22:17:19]

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:86: virtual bool CreateDc(HDC sibling, const RECT*
rect) = 0;
On 2011/03/08 20:16:53, vandebo wrote:
> On 2011/03/08 19:39:22, dpapad wrote:
> > On 2011/03/08 00:39:35, vandebo wrote:
> > > It'd be great to know if these methods are specific to the way we need to
> > print
> > > on windows, or if they are specific to EMF.  Maybe we'll only figure that
> out
> > > when when we have a non EMF metafile implementation.
> > 
> > These functions call internaly CreateEnhMetaFile(...). The description of
that
> > method is (taken from the Windows dcoumentation)
> > "The CreateEnhMetaFile function creates a device context for an
> enhanced-format
> > metafile. This device context can be used to store a device-independent
> > picture."
> > 
> > I am not sure if they are useful for non-EMF related operations.
> 
> That makes it sound like they are EMF specific.  Can you look at it more and
see
> if we can remove them from the nativemetafile interface and just have them in
> the emf implementation?

In several places (print_web_view_helper_win.cc, webplugin_delegate_stub.cc) the
following pattern is used

// other code
metafile->CreateDc(NULL, NULL);
HDC hdc = metafile->hdc();
skia::PlatformDevice::InitializeDC(hdc);
// other code that calls other NativeMetafile methods

Now calling CreateDc(NULL, NULL) looks like it is equivalent of calling
CreateEnhMetaFile(NULL,NULL,NULL.NULL) directly. Maybe substituting these two
calls can allow removing CreateDc from the interface. I would prefer to do this
on a separate CL.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:93: // TODO(maruel): CreateFromFile(). If ever used.
Maybe users would like to
On 2011/03/08 20:16:53, vandebo wrote:
> On 2011/03/08 19:39:22, dpapad wrote:
> > On 2011/03/08 00:39:35, vandebo wrote:
> > > Is this ever used?
> > 
> > No, it is not used anywhere. Should I remove it?
> 
> Yea, we like to remove dead code.

Done.

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:117: virtual bool SafePlayback(HDC hdc) const = 0;
On 2011/03/08 20:16:53, vandebo wrote:
> On 2011/03/08 19:39:22, dpapad wrote:
> > On 2011/03/08 00:39:35, vandebo wrote:
> > > Are both of the playback methods still used?
> > 
> > They are both used (but only in emf_win_unittest.cc).
> 
> If only used in emf unit tests, then they can be moved out of the
nativemetafile
> interface.

Actually, it turns out that SafePlayback is not only called in unittests. It is
called in printed_document_win.cc and print_system_win.cc (under
chrome/service/cloud_print/).

http://codereview.chromium.org/6611032/diff/18002/printing/pdf_metafile_mac.cc
File printing/pdf_metafile_mac.cc (right):

http://codereview.chromium.org/6611032/diff/18002/printing/pdf_metafile_mac.c...
printing/pdf_metafile_mac.cc:12: #include "ui/gfx/size.h"
The code was compiling without this, but I think it should be included here,
since it is only forward declared in printing/native_metafile.h.

[dpapad, 2011-03-10 19:32:34]

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/10024/printing/native_metafile.h#...
printing/native_metafile.h:93: // TODO(maruel): CreateFromFile(). If ever used.
Maybe users would like to
On 2011/03/08 22:17:19, dpapad wrote:
> On 2011/03/08 20:16:53, vandebo wrote:
> > On 2011/03/08 19:39:22, dpapad wrote:
> > > On 2011/03/08 00:39:35, vandebo wrote:
> > > > Is this ever used?
> > > 
> > > No, it is not used anywhere. Should I remove it?
> > 
> > Yea, we like to remove dead code.
> 
> Done.

It turns out that this function is called in
service_utility_process_host.cc:206, so I am going to restore it.

http://codereview.chromium.org/6611032/diff/24002/printing/native_metafile.h#...
printing/native_metafile.h:112: 
Restored this function since it is used (previously thought as dead code).

[vandebo (ex-Chrome), 2011-03-12 01:21:06]

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc#newcode62
printing/emf_win.cc:62: }
I don't remember if I specifically said to reorder the methods in the
implementation file.  If I did, sorry, we should leave this in the same order so
that svn blame will continue to work.

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc#newcode172
printing/emf_win.cc:172: bool Emf::CloseDc() {
CloseDC should be Close(), not CloseEmf -- CloseEmf destroys the emf data.

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.h
File printing/emf_win.h (left):

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.h#oldcode18
printing/emf_win.h:18: class Rect;
Rect is still used.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:10: #include "ui/gfx/native_widget_types.h"
What is this needed for?

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:54: virtual uint32 GetDataSize() const = 0;
Move GetDataSize and GetData below Close

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:82: virtual bool StartPage() = 0;
Put start page just above FinishPage -- We will shortly unify it to be platform
agnostic.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:141: // Retrieves the EMF stream. It is a helper
function.
This is not emf specific, use similar language as GetData() above.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:142: virtual bool GetData(std::vector<uint8>* buffer)
const = 0;
For a follow up CL: Should we use this interface across platforms instead of
GetDataSize + GetData?  Doing so would make sense only if all the current uses
of GetData allocate a buffer on the spot.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:145: virtual gfx::NativeDrawingContext hdc() const =
0;
pull this up to the common section and call it context()

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:162: virtual bool RenderPage(unsigned int
page_number,
nothing to d, but note: it seems that this method is similar in purpose to
SafePlayback.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:171: virtual gfx::NativeDrawingContext context() = 0;
To the common section.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:177: virtual bool SetRawData(const void* src_buffer,
uint32 src_buffer_size) = 0;
It looks like this is only used in pdf_ps_metafile_cairo_unittest.cc - If so, it
can be a private method in pdf_ps_metafile_cairo and not be in the metafile
interface.

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (left):

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:16: namespace gfx {
These are still used.

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:25: // This class creates a graphics context that
renders into a PDF data stream.
Keep this comment.

[dpapad, 2011-03-14 18:02:38]

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc#newcode62
printing/emf_win.cc:62: }
On 2011/03/12 01:21:06, vandebo wrote:
> I don't remember if I specifically said to reorder the methods in the
> implementation file.  If I did, sorry, we should leave this in the same order
so
> that svn blame will continue to work.

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.cc#newcode172
printing/emf_win.cc:172: bool Emf::CloseDc() {
On 2011/03/12 01:21:06, vandebo wrote:
> CloseDC should be Close(), not CloseEmf -- CloseEmf destroys the emf data.

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.h
File printing/emf_win.h (left):

http://codereview.chromium.org/6611032/diff/33023/printing/emf_win.h#oldcode18
printing/emf_win.h:18: class Rect;
On 2011/03/12 01:21:06, vandebo wrote:
> Rect is still used.

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:10: #include "ui/gfx/native_widget_types.h"
On 2011/03/12 01:21:06, vandebo wrote:
> What is this needed for?

This is where the typedef NativeDrawingContext is defined (cairo_t. HDC,
CGContextRef).

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:54: virtual uint32 GetDataSize() const = 0;
On 2011/03/12 01:21:06, vandebo wrote:
> Move GetDataSize and GetData below Close

Done (Also reordered in emf_win.h, pdf_metafile_mac.h, pdf_ps_metafile_cairo.h).

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:82: virtual bool StartPage() = 0;
On 2011/03/12 01:21:06, vandebo wrote:
> Put start page just above FinishPage -- We will shortly unify it to be
platform
> agnostic.

Done (Also reordered in emf_win.h, pdf_metafile_mac.h, pdf_ps_metafile_cairo.h).

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:141: // Retrieves the EMF stream. It is a helper
function.
On 2011/03/12 01:21:06, vandebo wrote:
> This is not emf specific, use similar language as GetData() above.

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:145: virtual gfx::NativeDrawingContext hdc() const =
0;
On 2011/03/12 01:21:06, vandebo wrote:
> pull this up to the common section and call it context()

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:171: virtual gfx::NativeDrawingContext context() = 0;
On 2011/03/12 01:21:06, vandebo wrote:
> To the common section.

Done.

http://codereview.chromium.org/6611032/diff/33023/printing/native_metafile.h#...
printing/native_metafile.h:177: virtual bool SetRawData(const void* src_buffer,
uint32 src_buffer_size) = 0;
On 2011/03/12 01:21:06, vandebo wrote:
> It looks like this is only used in pdf_ps_metafile_cairo_unittest.cc - If so,
it
> can be a private method in pdf_ps_metafile_cairo and not be in the metafile
> interface.

It is also used in webkit/plugins/ppapi/ppapi_plugin_instance.cc:1093 (through a
NativeMetafile pointer), so unless we use directly a PdfPsMetafile there it
can't be removed.

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (left):

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:16: namespace gfx {
On 2011/03/12 01:21:06, vandebo wrote:
> These are still used.

Done. FilePath is also used so I am forward declaring it here.

http://codereview.chromium.org/6611032/diff/33023/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:25: // This class creates a graphics context that
renders into a PDF data stream.
On 2011/03/12 01:21:06, vandebo wrote:
> Keep this comment.

Done.

[dpapad, 2011-03-14 19:00:35]

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:47: & file_path) const;
Accidental break line. I will fix that (along with some windows unitest compile
failures) in the following patch.

[vandebo (ex-Chrome), 2011-03-14 20:24:25]

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode71
printing/emf_win.cc:71: bool Emf::CreateFileBackedDc(HDC sibling,
Extra line break.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK(emf_ && !hdc_);
Add DCHECK_EQ(page_number, 1);

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode198
printing/emf_win.cc:198: bool Emf::StartPage() {
No reason to move these methods.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h
File printing/emf_win.h (left):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#oldcode23
printing/emf_win.h:23: // Simple wrapper class that manage an EMF data stream
and its virtual HDC.
You can leave this comment in.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode39
printing/emf_win.h:39: // Saves the EMF data to a file as-is. It is recommended
to use the .emf file
I think this comment goes with SaveTo ?

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode51
printing/emf_win.h:51: return 1;
Actually (as I understand EMF better), we could have a multipage EMF, we just
don't right now.  Add a TODO to count the number of times StartPage is called. 
(You could fix this in the StartPage CL you're working on now.)

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode108
printing/emf_win.h:108: // The entries become invalid once Emf::Close() is
called.
CloseEmf

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode161
printing/emf_win.h:161: // Emf::emf_. The entries become invalid once
Emf::Close() is called.
CloseEmf

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h#...
printing/native_metafile.h:10: #include "ui/gfx/native_widget_types.h"
For this code, let's use PlatformSurface from platfor_device.h.  Also please
file a bug that we should clean this up - area printing.

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h#...
printing/native_metafile.h:37: // This class creates a graphics context that
renders into a data stream.
nit: ...into a data stream (usually PDF or EMF).

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:42: // For testing purposes only.
Belongs to SaveTo()?

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:162: page_size.width() + margin_left_in_points
+ margin_right_in_points;
The argument obviously isn't page size, it's content size.  Move this math up to
the caller to make the name accurate (on Mac the argument is actually page
size).

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:42: virtual unsigned int GetPageCount() const {
return 1; }
Move the body of this method into the .cc file and change it to NOTIMPLEMENTED. 
1 is usually incorrect.

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:44: virtual gfx::NativeDrawingContext context()
const {
You can use cairo_t* here.

[dpapad, 2011-03-14 22:15:12]

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode71
printing/emf_win.cc:71: bool Emf::CreateFileBackedDc(HDC sibling,
On 2011/03/14 20:24:25, vandebo wrote:
> Extra line break.

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK(emf_ && !hdc_);
On 2011/03/14 20:24:25, vandebo wrote:
> Add DCHECK_EQ(page_number, 1);

Done. static_cast is necessary to avoid compilation error (comparing signed with
unsigned).

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode198
printing/emf_win.cc:198: bool Emf::StartPage() {
On 2011/03/14 20:24:25, vandebo wrote:
> No reason to move these methods.

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode39
printing/emf_win.h:39: // Saves the EMF data to a file as-is. It is recommended
to use the .emf file
On 2011/03/14 20:24:25, vandebo wrote:
> I think this comment goes with SaveTo ?

Done. It turns out that moving functions around using Eclipse doesn't move the
comments too.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode51
printing/emf_win.h:51: return 1;
On 2011/03/14 20:24:25, vandebo wrote:
> Actually (as I understand EMF better), we could have a multipage EMF, we just
> don't right now.  Add a TODO to count the number of times StartPage is called.

> (You could fix this in the StartPage CL you're working on now.)

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode108
printing/emf_win.h:108: // The entries become invalid once Emf::Close() is
called.
On 2011/03/14 20:24:25, vandebo wrote:
> CloseEmf

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#newcode161
printing/emf_win.h:161: // Emf::emf_. The entries become invalid once
Emf::Close() is called.
On 2011/03/14 20:24:25, vandebo wrote:
> CloseEmf

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h#...
printing/native_metafile.h:10: #include "ui/gfx/native_widget_types.h"
On 2011/03/14 20:24:25, vandebo wrote:
> For this code, let's use PlatformSurface from platfor_device.h.  Also please
> file a bug that we should clean this up - area printing.

There is some problem in the includes of SkTypes.h (Can't find SkUserConfig.h,
most probably because it has been moved) so I am leaving this as it is for now.

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h#...
printing/native_metafile.h:37: // This class creates a graphics context that
renders into a data stream.
On 2011/03/14 20:24:25, vandebo wrote:
> nit: ...into a data stream (usually PDF or EMF).

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/native_metafile.h#...
printing/native_metafile.h:70: double margin_bottom_in_points,
Removed this parameters. The caller already includes these margins in
page_size). The rest of the parameters are still needed.

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:42: // For testing purposes only.
On 2011/03/14 20:24:25, vandebo wrote:
> Belongs to SaveTo()?

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:47: & file_path) const;
On 2011/03/14 19:00:35, dpapad wrote:
> Accidental break line. I will fix that (along with some windows unitest
compile
> failures) in the following patch.

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:162: page_size.width() + margin_left_in_points
+ margin_right_in_points;
On 2011/03/14 20:24:25, vandebo wrote:
> The argument obviously isn't page size, it's content size.  Move this math up
to
> the caller to make the name accurate (on Mac the argument is actually page
> size).

Done. Also I removed unused args. The remaining args could be combined in to a
gfx::Point object (left for future cleanup CL).

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:42: virtual unsigned int GetPageCount() const {
return 1; }
On 2011/03/14 20:24:25, vandebo wrote:
> Move the body of this method into the .cc file and change it to
NOTIMPLEMENTED. 
> 1 is usually incorrect.

Done.

http://codereview.chromium.org/6611032/diff/33026/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:44: virtual gfx::NativeDrawingContext context()
const {
On 2011/03/14 20:24:25, vandebo wrote:
> You can use cairo_t* here.

Done.

[vandebo (ex-Chrome), 2011-03-14 22:55:04]

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.cc#newcode198
printing/emf_win.cc:198: bool Emf::StartPage() {
The CR still shows them moved.  They used to be below SafePlayback

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h
File printing/emf_win.h (left):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#oldcode23
printing/emf_win.h:23: // Simple wrapper class that manage an EMF data stream
and its virtual HDC.
On 2011/03/14 20:24:25, vandebo wrote:
> You can leave this comment in.

You didn't address this.

http://codereview.chromium.org/6611032/diff/44001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/6611032/diff/44001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:216:
static_cast<int>(content_width_in_points + margin_right_in_points +
A cast shouldn't be needed for double -> int conversion.  Did the compiler
complain?

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK_EQ(page_number, static_cast<unsigned int>(1));
I think you can use "1U".

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.h#newcode51
printing/emf_win.h:51: // TODO(dpapad): count the number of time StartPage() is
called
Should be indented two spaces.
nit: time -> times

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:130: unsigned int
PdfPsMetafile::GetPageCount() const {
Since you are adding this method, you should be it in the same place it listed
in the header file, i.e. after GetPageBounds.

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:162: // We build in extra room for the
margins. The Cairo PDF backend will scale
This comment doesn't make sense any more.

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:164: double width = page_size.width();
Doesn't look like we need these variables any more.

[Lei Zhang, 2011-03-15 05:12:29]

http://codereview.chromium.org/6611032/diff/42004/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/42004/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:154: //DCHECK_GT(page_size.width(), 0) <<
page_size.width();
Please don't do this. I believe the check is valid. Something is wrong if your
page size i 0x0. The failing tests in patch set 13 exposes the fact that the
MockPrinter is setting some bogus values.

[dpapad, 2011-03-15 16:11:06]

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h
File printing/emf_win.h (left):

http://codereview.chromium.org/6611032/diff/33026/printing/emf_win.h#oldcode23
printing/emf_win.h:23: // Simple wrapper class that manage an EMF data stream
and its virtual HDC.
On 2011/03/14 22:55:05, vandebo wrote:
> On 2011/03/14 20:24:25, vandebo wrote:
> > You can leave this comment in.
> 
> You didn't address this.

Done.

http://codereview.chromium.org/6611032/diff/44001/chrome/renderer/print_web_v...
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/6611032/diff/44001/chrome/renderer/print_web_v...
chrome/renderer/print_web_view_helper_linux.cc:216:
static_cast<int>(content_width_in_points + margin_right_in_points +
On 2011/03/14 22:55:05, vandebo wrote:
> A cast shouldn't be needed for double -> int conversion.  Did the compiler
> complain?

Done. No it didn't complain, I just thought it is better practice to avoid
implicit conversions in general.

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK_EQ(page_number, static_cast<unsigned int>(1));
On 2011/03/14 22:55:05, vandebo wrote:
> I think you can use "1U".

Done.

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK_EQ(page_number, static_cast<unsigned int>(1));
On 2011/03/14 22:55:05, vandebo wrote:
> I think you can use "1U".

Done.

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/44001/printing/emf_win.h#newcode51
printing/emf_win.h:51: // TODO(dpapad): count the number of time StartPage() is
called
On 2011/03/14 22:55:05, vandebo wrote:
> Should be indented two spaces.
> nit: time -> times

Done.

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:130: unsigned int
PdfPsMetafile::GetPageCount() const {
On 2011/03/14 22:55:05, vandebo wrote:
> Since you are adding this method, you should be it in the same place it listed
> in the header file, i.e. after GetPageBounds.

Done.

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:162: // We build in extra room for the
margins. The Cairo PDF backend will scale
On 2011/03/14 22:55:05, vandebo wrote:
> This comment doesn't make sense any more.

Done.

http://codereview.chromium.org/6611032/diff/44001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:164: double width = page_size.width();
On 2011/03/14 22:55:05, vandebo wrote:
> Doesn't look like we need these variables any more.

Done. That's correct. I just kept them because pdf_metafile_mac.cc does the
same, probably should be removed from there too.

http://codereview.chromium.org/6611032/diff/42004/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:154: double margin_left_in_points) {
On 2011/03/15 05:12:29, Lei Zhang wrote:
> Please don't do this. I believe the check is valid. Something is wrong if your
> page size i 0x0. The failing tests in patch set 13 exposes the fact that the
> MockPrinter is setting some bogus values.

I did not mean to commit it like this. Just wanted to test the rest of the
changes before figuring out where the bogus values come from.

[vandebo (ex-Chrome), 2011-03-15 20:33:01]

http://codereview.chromium.org/6611032/diff/45025/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK_EQ(page_number, 1U);
According to unwritten (as far as I know) style the order of arguments to a
DCHECK_EQ should be expected, actual.  So this is backward.

http://codereview.chromium.org/6611032/diff/45025/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/native_metafile.h#...
printing/native_metafile.h:21: namespace gfx {
See http://www.chromium.org/developers/coding-style for order of forward
declarations.  (FilePath, gfx, printing, CHROMEOS)

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.cc
File printing/pdf_metafile_mac.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.c...
printing/pdf_metafile_mac.cc:70: DLOG(INFO) << "size: " << page_size.height();
Remove

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:22: class FilePath;
FilePath before gfx

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:66: // To create PDF data, callers should call
Init() to set up the rendering
This comment doesn't seem to go with the constructor anymore.  Delete?

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:218: if (file_util::WriteFile(file_path,
data_.data(), GetDataSize()) < 0) {
Should this be != GetDataSize() ?

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:54: static const double kTopMarginInInch;
Looks like you need to rebase your change.

[dpapad, 2011-03-15 21:41:07]

http://codereview.chromium.org/6611032/diff/45025/printing/emf_win.cc
File printing/emf_win.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/emf_win.cc#newcode131
printing/emf_win.cc:131: DCHECK_EQ(page_number, 1U);
On 2011/03/15 20:33:01, vandebo wrote:
> According to unwritten (as far as I know) style the order of arguments to a
> DCHECK_EQ should be expected, actual.  So this is backward.

Done.

http://codereview.chromium.org/6611032/diff/45025/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/native_metafile.h#...
printing/native_metafile.h:21: namespace gfx {
On 2011/03/15 20:33:01, vandebo wrote:
> See http://www.chromium.org/developers/coding-style for order of forward
> declarations.  (FilePath, gfx, printing, CHROMEOS)

Done. There are no forward declarations in printing namespace here, so the order
is FilePath, gfx, CHROMEOS.

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.cc
File printing/pdf_metafile_mac.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.c...
printing/pdf_metafile_mac.cc:70: DLOG(INFO) << "size: " << page_size.height();
On 2011/03/15 20:33:01, vandebo wrote:
> Remove

Done.

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:22: class FilePath;
On 2011/03/15 20:33:01, vandebo wrote:
> FilePath before gfx

Done.

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:66: // To create PDF data, callers should call
Init() to set up the rendering
On 2011/03/15 20:33:01, vandebo wrote:
> This comment doesn't seem to go with the constructor anymore.  Delete?

Done.

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:218: if (file_util::WriteFile(file_path,
data_.data(), GetDataSize()) < 0) {
On 2011/03/15 20:33:01, vandebo wrote:
> Should this be != GetDataSize() ?

Function file_util::WriteFile returns the number of bytes written, or -1 on
error, so theoretically it could return 0 without an error having occurred. I
think the most appropriate way to check the return value is comparing to -1
directly (as in several other places in the code).

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:54: static const double kTopMarginInInch;
On 2011/03/15 20:33:01, vandebo wrote:
> Looks like you need to rebase your change.

Done.

http://codereview.chromium.org/6611032/diff/42028/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/42028/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:52: // the data returned from GetData will not be
valid PDF data.
Accidentally this comments were left elsewhere. Now they are in the right place.

http://codereview.chromium.org/6611032/diff/42028/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:65: protected:
Fixed indentation.

[Lei Zhang, 2011-03-15 21:53:32]

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:218: if (file_util::WriteFile(file_path,
data_.data(), GetDataSize()) < 0) {
On 2011/03/15 21:41:07, dpapad wrote:
> On 2011/03/15 20:33:01, vandebo wrote:
> > Should this be != GetDataSize() ?
> 
> Function file_util::WriteFile returns the number of bytes written, or -1 on
> error, so theoretically it could return 0 without an error having occurred. I
> think the most appropriate way to check the return value is comparing to -1
> directly (as in several other places in the code).

It should only return 0 if you passed in size=0, which you never get since
there's a DCHECK() is GetDataSize() that checks to make sure data_ is not empty.

Since no code calls WriteFile() and then checks for partial writes, there is an
expectation that file_util::WriteFile() handles the partial writes. Thus it's
better to check to make sure WriteFile() did write all the data.

[vandebo (ex-Chrome), 2011-03-15 21:54:05]

LGTM!

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/45025/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:218: if (file_util::WriteFile(file_path,
data_.data(), GetDataSize()) < 0) {
On 2011/03/15 21:41:07, dpapad wrote:
> On 2011/03/15 20:33:01, vandebo wrote:
> > Should this be != GetDataSize() ?
> 
> Function file_util::WriteFile returns the number of bytes written, or -1 on
> error, so theoretically it could return 0 without an error having occurred. I
> think the most appropriate way to check the return value is comparing to -1
> directly (as in several other places in the code).

< 0 is fine, but what I said is valid. On some systems in certain conditions it
is valid and not an error to write less than the amount that was requested to
write.  You would then generally want to test that the function wrote as much as
you asked it to write.

[dpapad, 2011-03-15 23:39:34]

Linux, ChromeOS failed the tests because the LKGR was before Lei's mock printer
fix (r78266). Re-trying with that revision.

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.cc:219: != static_cast<int>(GetDataSize())) {
Making sure that everything was written. (static_cast necessary to compare
unsigned with signed ints).

[Lei Zhang, 2011-03-16 00:25:26]

LGTM with minor nits below.

Also, since this change touches a lot of stuff, I would run it through the clang
try bots now and fix any problems before committing.

http://codereview.chromium.org/6611032/diff/46008/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/46008/printing/emf_win.h#newcode32
printing/emf_win.h:32: //NativeMetafile methods.
nit: space after //.

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:32: //NativeMetafile methods.
nit: space after //.

[dpapad, 2011-03-16 00:35:57]

http://codereview.chromium.org/6611032/diff/46008/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6611032/diff/46008/printing/emf_win.h#newcode32
printing/emf_win.h:32: //NativeMetafile methods.
On 2011/03/16 00:25:26, Lei Zhang wrote:
> nit: space after //.

Done.

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6611032/diff/46008/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:32: //NativeMetafile methods.
On 2011/03/16 00:25:26, Lei Zhang wrote:
> nit: space after //.

Done.

[dpapad, 2011-03-16 01:20:38]

The last patch should be named "Making virtual methods not inline (or else clang
bots wont compile)".

[Lei Zhang, 2011-03-16 01:26:21]

LGTM if the CLANG bots go green.