Implement native metafile for printing on Linux....

Implement native metafile for printing on Linux.
The metafile class stores the resulting PDF/PS stream in memory.

BUG=9847

[M-A Ruel, 2009-08-19 13:53:47]

It'd be nice to have a unit test, even if incomplete.

http://codereview.chromium.org/174042/diff/1/2
File printing/native_metafile.h (right):

http://codereview.chromium.org/174042/diff/1/2#newcode35
Line 35: #include "printing/native_metafile_linux.h"
Why not forward declare like the other platforms?

http://codereview.chromium.org/174042/diff/1/4
File printing/native_metafile_linux.cc (right):

http://codereview.chromium.org/174042/diff/1/4#newcode28
Line 28: } break;
} and break on a separate line.

http://codereview.chromium.org/174042/diff/1/4#newcode36
Line 36: NOTREACHED();
you should return?

http://codereview.chromium.org/174042/diff/1/4#newcode62
Line 62: const void* src_buffer, size_t src_buffer_size)
nit: put that on 3 lines.

http://codereview.chromium.org/174042/diff/1/4#newcode72
Line 72: // Un-ref resources if we forgot to do so.
Why don't you call Close()?

http://codereview.chromium.org/174042/diff/1/4#newcode74
Line 74: cairo_surface_finish(surface_);
It'd be nice to use a scoped_malloc_ptr but I guess it's overkill.

http://codereview.chromium.org/174042/diff/1/4#newcode152
Line 152: return true;
Seems like the return value is useless. Don't return a bool unless necessary.

http://codereview.chromium.org/174042/diff/1/5
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/174042/diff/1/5#newcode32
Line 32: const void* src_buffer, size_t src_buffer_size);
nit: please put all the parameters on a separate line

http://codereview.chromium.org/174042/diff/1/5#newcode36
Line 36: // This will prepare a new cairo surface and a new cairo context for
Prepares a new ..

http://codereview.chromium.org/174042/diff/1/5#newcode40
Line 40: // Return the Cairo context for rendering the current page.
Returns ...

etc

http://codereview.chromium.org/174042/diff/1/5#newcode69
Line 69: // This is the callback function for Cairo to write the PDF/PS
contents.
Add an imperative verb to describe what this function does.
nit: I'd note that dest_buffer is in fact a std::string*

[myhuang, 2009-08-24 18:25:12]

I have created a simple unit test in this patch set.

http://codereview.chromium.org/174042/diff/1/2
File printing/native_metafile.h (right):

http://codereview.chromium.org/174042/diff/1/2#newcode35
Line 35: #include "printing/native_metafile_linux.h"
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Why not forward declare like the other platforms?

Done.

http://codereview.chromium.org/174042/diff/1/4
File printing/native_metafile_linux.cc (right):

http://codereview.chromium.org/174042/diff/1/4#newcode28
Line 28: } break;
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> } and break on a separate line.

Done.

http://codereview.chromium.org/174042/diff/1/4#newcode36
Line 36: NOTREACHED();
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> you should return?

Done.

http://codereview.chromium.org/174042/diff/1/4#newcode62
Line 62: const void* src_buffer, size_t src_buffer_size)
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> nit: put that on 3 lines.

Done.

http://codereview.chromium.org/174042/diff/1/4#newcode72
Line 72: // Un-ref resources if we forgot to do so.
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Why don't you call Close()?

Done.

http://codereview.chromium.org/174042/diff/1/4#newcode152
Line 152: return true;
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Seems like the return value is useless. Don't return a bool unless necessary.

Done.

http://codereview.chromium.org/174042/diff/1/5
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/174042/diff/1/5#newcode32
Line 32: const void* src_buffer, size_t src_buffer_size);
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> nit: please put all the parameters on a separate line

Done.

http://codereview.chromium.org/174042/diff/1/5#newcode36
Line 36: // This will prepare a new cairo surface and a new cairo context for
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Prepares a new ..

Done.

http://codereview.chromium.org/174042/diff/1/5#newcode40
Line 40: // Return the Cairo context for rendering the current page.
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Returns ...
> 
> etc

Done.

http://codereview.chromium.org/174042/diff/1/5#newcode69
Line 69: // This is the callback function for Cairo to write the PDF/PS
contents.
On 2009/08/19 13:53:47, Marc-Antoine Ruel wrote:
> Add an imperative verb to describe what this function does.
> nit: I'd note that dest_buffer is in fact a std::string*

Done.

[M-A Ruel, 2009-08-24 18:30:47]

lgtm with comments (especially the #include in native_metafile.h)

http://codereview.chromium.org/174042/diff/9/1007
File printing/native_metafile.h (right):

http://codereview.chromium.org/174042/diff/9/1007#newcode35
Line 35: #include "printing/pdf_ps_linux.h"
Still not needed?

http://codereview.chromium.org/174042/diff/9/1008
File printing/pdf_ps_linux.h (right):

http://codereview.chromium.org/174042/diff/9/1008#newcode20
Line 20: class PdfPs {
I guess you exhibit how ill-named is the 'Emf' class. :)

Can you rename it to PdfPsStream or PdfPsMetaFile please?

In fact, I'd almost prefer PostScriptStream since it's much clearer.

As you prefer.

http://codereview.chromium.org/174042/diff/9/1011
File printing/printing.gyp (right):

http://codereview.chromium.org/174042/diff/9/1011#newcode118
Line 118: #            '../app/app.gyp:app_base',  # Only required for Font
support
Remove it if it is not needed after all.

[myhuang, 2009-08-24 21:14:11]

http://codereview.chromium.org/174042/diff/9/1008
File printing/pdf_ps_linux.h (right):

http://codereview.chromium.org/174042/diff/9/1008#newcode20
Line 20: class PdfPs {
On 2009/08/24 18:30:47, Marc-Antoine Ruel wrote:
> I guess you exhibit how ill-named is the 'Emf' class. :)
> 
> Can you rename it to PdfPsStream or PdfPsMetaFile please?
> 
> In fact, I'd almost prefer PostScriptStream since it's much clearer.
> 
> As you prefer.

Done.

http://codereview.chromium.org/174042/diff/9/1011
File printing/printing.gyp (right):

http://codereview.chromium.org/174042/diff/9/1011#newcode118
Line 118: #            '../app/app.gyp:app_base',  # Only required for Font
support
On 2009/08/24 18:30:47, Marc-Antoine Ruel wrote:
> Remove it if it is not needed after all.

Done.

[M-A Ruel, 2009-08-24 21:16:57]

still lgtm

http://codereview.chromium.org/174042/diff/18/20
File printing/pdf_ps_metafile_linux.h (right):

http://codereview.chromium.org/174042/diff/18/20#newcode88
Line 88: // The Cairo surface and context for the entire resulting PDF/PS file.
Remove 'The' everywhere, less is better and this word doesn't add anything to
the explanation. :)

[myhuang, 2009-08-24 22:34:36]

http://codereview.chromium.org/174042/diff/18/20
File printing/pdf_ps_metafile_linux.h (right):

http://codereview.chromium.org/174042/diff/18/20#newcode88
Line 88: // The Cairo surface and context for the entire resulting PDF/PS file.
On 2009/08/24 21:16:57, Marc-Antoine Ruel wrote:
> Remove 'The' everywhere, less is better and this word doesn't add anything to
> the explanation. :)

Done.

[tony, 2009-08-25 17:37:16]

Landed in r24243