Create a Factory for NativeMetafile.

Applying factory pattern (through NativeMetafileFactory class). It is used to retrieve different printing contexts (based on the platform and user preferences).


BUG=NONE
TEST=NONE

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

Reverted: http://src.chromium.org/viewvc/chrome?view=rev&revision=76555

Recommitted: http://src.chromium.org/viewvc/chrome?view=rev&revision=76581

[Avi (use Gerrit), 2011-02-22 17:33:42]

Drive-by style complaints.

Please re-read the style guide: http://www.corp.google.com/eng/doc/cppguide.xml

Notable issues found in multiple files:
- Don't name pointers with _ptr on the end. Their use (-> vs .) will make it
obvious.
- Please keep 80 columns! Removing _ptr will fix many (but not all) of those
instances.
- Friend declarations go in the private section of class declarations, not at
the top.
- While it's a nice-to-have to update the copyright year when modifying files,
there's no excuse to not have the current year in new files in the changeset.

http://codereview.chromium.org/6544028/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6544028/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:72:
printing::MetafileFactory::GetMetafile());
For example, this is a pointer without a _ptr naming. Follow this example.

http://codereview.chromium.org/6544028/diff/1/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6544028/diff/1/printing/emf_win.h#newcode46
printing/emf_win.h:46: virtual bool CreateFileBackedDc(HDC sibling, const RECT*
rect, const FilePath& path);
80 col!

http://codereview.chromium.org/6544028/diff/1/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/1/printing/native_metafile_win.h#...
printing/native_metafile_win.h:38: virtual bool CreateFileBackedDc(HDC sibling,
const RECT* rect, const FilePath& path) = 0;
80 col!

http://codereview.chromium.org/6544028/diff/1/printing/pdf_ps_metafile_cairo_...
File printing/pdf_ps_metafile_cairo_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/1/printing/pdf_ps_metafile_cairo_...
printing/pdf_ps_metafile_cairo_unittest.cc:36:
//EXPECT_EQ(printing::PdfPsMetafile::FromCairoContext(context), pdf_ptr);
In general, don't commit commented-out code unless there's a good reason.

http://codereview.chromium.org/6544028/diff/1/printing/printed_document.h
File printing/printed_document.h (right):

http://codereview.chromium.org/6544028/diff/1/printing/printed_document.h#new...
printing/printed_document.h:15: #include "printing/native_metafile.h"
Cool, thanks for fixing!

[dpapad, 2011-02-22 18:09:30]

http://codereview.chromium.org/6544028/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6544028/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_win.cc:72:
printing::MetafileFactory::GetMetafile());
On 2011/02/22 17:33:42, Avi wrote:
> For example, this is a pointer without a _ptr naming. Follow this example.

Done.

http://codereview.chromium.org/6544028/diff/1/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6544028/diff/1/printing/emf_win.h#newcode46
printing/emf_win.h:46: virtual bool CreateFileBackedDc(HDC sibling, const RECT*
rect, const FilePath& path);
On 2011/02/22 17:33:42, Avi wrote:
> 80 col!

Done.

http://codereview.chromium.org/6544028/diff/1/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/1/printing/native_metafile_win.h#...
printing/native_metafile_win.h:38: virtual bool CreateFileBackedDc(HDC sibling,
const RECT* rect, const FilePath& path) = 0;
On 2011/02/22 17:33:42, Avi wrote:
> 80 col!

Done.

http://codereview.chromium.org/6544028/diff/1/printing/pdf_ps_metafile_cairo_...
File printing/pdf_ps_metafile_cairo_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/1/printing/pdf_ps_metafile_cairo_...
printing/pdf_ps_metafile_cairo_unittest.cc:36:
//EXPECT_EQ(printing::PdfPsMetafile::FromCairoContext(context), pdf_ptr);
On 2011/02/22 17:33:42, Avi wrote:
> In general, don't commit commented-out code unless there's a good reason.

Done.

[Avi (use Gerrit), 2011-02-22 19:00:06]

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win.h#newcode26
printing/emf_win.h:26: friend class MetafileFactory;
friends need to be in the private section.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc
File printing/metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:21: 
unnecessary line

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:28: friend class MetafileFactory;
friends are private.

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

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:29: friend class MetafileFactory;
private friends

[dpapad, 2011-02-22 19:29:10]

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win.h
File printing/emf_win.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win.h#newcode26
printing/emf_win.h:26: friend class MetafileFactory;
On 2011/02/22 19:00:07, Avi wrote:
> friends need to be in the private section.

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc
File printing/metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:21: 
On 2011/02/22 19:00:07, Avi wrote:
> unnecessary line

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:28: friend class MetafileFactory;
On 2011/02/22 19:00:07, Avi wrote:
> friends are private.

Done.

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

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:29: friend class MetafileFactory;
On 2011/02/22 19:00:07, Avi wrote:
> private friends

Done.

[Avi (use Gerrit), 2011-02-22 19:31:43]

Style gremlin is satisfied. Back to your scheduled reviewer.

[vandebo (ex-Chrome), 2011-02-22 22:18:16]

CL description should be improved.  Subject something like "Create and use a
factory class for NativeMetafile."  The detail should be more descriptive. 
Consider two audiences when writing the CL description.  One, a sheriff or
release engineer, looking for a CL that broke or regressed something and two, a
SWE is looking back at the CL six months or more from now trying to understand
why you did what you did.

You might as well update the copyright year for every file you touched.

http://codereview.chromium.org/6544028/diff/7001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6544028/diff/7001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:127: scoped_ptr<NativeMetafile>
metafile(printing::MetafileFactory::GetMetafile());
We're already in the printing namespace, you probably don't need printing::

http://codereview.chromium.org/6544028/diff/7001/chrome/plugin/webplugin_dele...
File chrome/plugin/webplugin_delegate_stub.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/plugin/webplugin_dele...
chrome/plugin/webplugin_delegate_stub.cc:17: #include
"printing/native_metafile.h"
NativeMetafile is still used, you should continue to include the header for it.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:11: #include
"printing/native_metafile.h"
NativeMetafile is still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_mac.mm (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:14: #include
"printing/native_metafile.h"
NativeMetafile is still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:29: printing::NativeMetafile*
metafile =
Looks like a mem leak. scoped_ptr ?

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:84: printing::NativeMetafile*
metafile =
scoped_ptr ?

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:13: #include
"printing/native_metafile.h"
NativeMetafile is still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_pepper.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_pepper.cc:53: #include
"printing/native_metafile.h"
NativeMetafile is still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_pepper.cc:1225:
printing::MetafileFactory::GetMetafileFrom(context);
This is cairo specific specific, so probably should go into the factory.  Can
you look into the details of what's going on here and then figure out what needs
to be done when we have two different ways to produce PDFs?

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_proxy.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_proxy.cc:36: #include
"printing/native_metafile.h"
Still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/service/service_utili...
File chrome/service/service_utility_process_host.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/service/service_utili...
chrome/service/service_utility_process_host.cc:16: #include
"printing/native_metafile.h"
Still used.

http://codereview.chromium.org/6544028/diff/7001/chrome/utility/utility_threa...
File chrome/utility/utility_thread.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/utility/utility_threa...
chrome/utility/utility_thread.cc:21: #include "printing/native_metafile.h"
Still used.

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win_unittest.cc
File printing/emf_win_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win_unittest.cc...
printing/emf_win_unittest.cc:45: printing::Emf emf;
I know you haven't build windows yet, but it looks like this will fail.  You
could either use the factory, or make EmfPrintingTest a friend.

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win_unittest.cc...
printing/emf_win_unittest.cc:199: TEST(EmfTest, DCFactory) {
Maybe put this next to DC.  You could pull out the common code to a shared
function.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc
File printing/metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:9: /* static */
// static

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:12: #if defined(OS_WIN)
These can all be of the form: return new printing::Foo;

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h
File printing/metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:5: #ifndef METAFILE_FACTORY_H_
PRINTING_NATIVE_METAFILE_FACTORY_H_

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:21: // This class is used to obtain pointers to
NativeMetafile objects. It is the
This comment can be improved; most readers will understand the concept of a
factory, however it might be useful to say why we are using a factory.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:23: class MetafileFactory {
NativeMetafileFactory

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:25: virtual ~MetafileFactory();
Not necessary, as there is currently no way to create an instance of this class.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:29: static printing::NativeMetafile* GetMetafile();
Get implies retrieval and not creation.  Since this instantiates a new object,
the name should indicate that it is making something.  CreateMetafile is a
reasonable choice.

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile.h#n...
printing/native_metafile.h:5: #ifndef NATIVE_METAFILE_H_
PRINTING_NATIVE_METAFILE_H_

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_lin...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_lin...
printing/native_metafile_linux.h:5: #ifndef NATIVE_METAFILE_LINUX_H_
PRINTING_...

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_lin...
printing/native_metafile_linux.h:27: enum FileFormat {
We don't use PS (iirc, because the cairo backend has to write to a temp file to
do it).  A follow up CL to remove FileFormat from the interface would be good.

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_mac.h
File printing/native_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_mac...
printing/native_metafile_mac.h:5: #ifndef NATIVE_METAFILE_MAC_H_
PRINTING_...

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_win...
printing/native_metafile_win.h:5: #ifndef NATIVE_METAFILE_WIN_H_
PRINTING_...

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

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:33: //TODO dpapad: move it to protected (or
delete, PDF is supported only anyway)
+1

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:109: virtual void CleanUpAll();
private virtual ?

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:29:
scoped_ptr<printing::NativeMetafile> pdf(
This is a unit test for pdf_ps_metafile, so it should stay specific to that. 
The easiest way to do that may be to make PpdfPsTest a friend of PdfPsMetfile.

http://codereview.chromium.org/6544028/diff/7001/printing/print_settings_init...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:12: #include
"printing/native_metafile.h"
No longer needed.

http://codereview.chromium.org/6544028/diff/7001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (left):

http://codereview.chromium.org/6544028/diff/7001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:25: #include
"printing/native_metafile.h"
Still used.

[dpapad, 2011-02-23 00:44:38]

http://codereview.chromium.org/6544028/diff/7001/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6544028/diff/7001/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:127: scoped_ptr<NativeMetafile>
metafile(printing::MetafileFactory::GetMetafile());
On 2011/02/22 22:18:16, vandebo wrote:
> We're already in the printing namespace, you probably don't need printing::

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/plugin/webplugin_dele...
File chrome/plugin/webplugin_delegate_stub.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/plugin/webplugin_dele...
chrome/plugin/webplugin_delegate_stub.cc:17: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafile is still used, you should continue to include the header for
it.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_linux.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_linux.cc:11: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafile is still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_mac.mm (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:14: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafile is still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_mac.mm (right):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:29: printing::NativeMetafile*
metafile =
On 2011/02/22 22:18:16, vandebo wrote:
> Looks like a mem leak. scoped_ptr ?

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:84: printing::NativeMetafile*
metafile =
On 2011/02/22 22:18:16, vandebo wrote:
> scoped_ptr ?

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:13: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafile is still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_pepper.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_pepper.cc:53: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafile is still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_proxy.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_proxy.cc:36: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> Still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/service/service_utili...
File chrome/service/service_utility_process_host.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/service/service_utili...
chrome/service/service_utility_process_host.cc:16: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> Still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/chrome/utility/utility_threa...
File chrome/utility/utility_thread.cc (left):

http://codereview.chromium.org/6544028/diff/7001/chrome/utility/utility_threa...
chrome/utility/utility_thread.cc:21: #include "printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> Still used.

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win_unittest.cc
File printing/emf_win_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/emf_win_unittest.cc...
printing/emf_win_unittest.cc:199: TEST(EmfTest, DCFactory) {
On 2011/02/22 22:18:16, vandebo wrote:
> Maybe put this next to DC.  You could pull out the common code to a shared
> function.

The unit test is basically identical, only the initialization of NativeMetafile
is different. I am merging them to a single unit test (should have done it from
the start).

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc
File printing/metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:9: /* static */
On 2011/02/22 22:18:16, vandebo wrote:
> // static

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.cc...
printing/metafile_factory.cc:12: #if defined(OS_WIN)
On 2011/02/22 22:18:16, vandebo wrote:
> These can all be of the form: return new printing::Foo;

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h
File printing/metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:5: #ifndef METAFILE_FACTORY_H_
On 2011/02/22 22:18:16, vandebo wrote:
> PRINTING_NATIVE_METAFILE_FACTORY_H_

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:21: // This class is used to obtain pointers to
NativeMetafile objects. It is the
On 2011/02/22 22:18:16, vandebo wrote:
> This comment can be improved; most readers will understand the concept of a
> factory, however it might be useful to say why we are using a factory.

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:23: class MetafileFactory {
On 2011/02/22 22:18:16, vandebo wrote:
> NativeMetafileFactory

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:25: virtual ~MetafileFactory();
On 2011/02/22 22:18:16, vandebo wrote:
> Not necessary, as there is currently no way to create an instance of this
class.

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/metafile_factory.h#...
printing/metafile_factory.h:29: static printing::NativeMetafile* GetMetafile();
On 2011/02/22 22:18:16, vandebo wrote:
> Get implies retrieval and not creation.  Since this instantiates a new object,
> the name should indicate that it is making something.  CreateMetafile is a
> reasonable choice.

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile.h#n...
printing/native_metafile.h:5: #ifndef NATIVE_METAFILE_H_
On 2011/02/22 22:18:16, vandebo wrote:
> PRINTING_NATIVE_METAFILE_H_

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_lin...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_lin...
printing/native_metafile_linux.h:5: #ifndef NATIVE_METAFILE_LINUX_H_
On 2011/02/22 22:18:16, vandebo wrote:
> PRINTING_...

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/7001/printing/native_metafile_win...
printing/native_metafile_win.h:5: #ifndef NATIVE_METAFILE_WIN_H_
On 2011/02/22 22:18:16, vandebo wrote:
> PRINTING_...

Done.

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

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:33: //TODO dpapad: move it to protected (or
delete, PDF is supported only anyway)
On 2011/02/22 22:18:16, vandebo wrote:
> +1

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:109: virtual void CleanUpAll();
On 2011/02/22 22:18:16, vandebo wrote:
> private virtual ?

Done.

http://codereview.chromium.org/6544028/diff/7001/printing/print_settings_init...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/7001/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:12: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> No longer needed.

Done.

http://codereview.chromium.org/6544028/diff/7001/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (left):

http://codereview.chromium.org/6544028/diff/7001/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:25: #include
"printing/native_metafile.h"
On 2011/02/22 22:18:16, vandebo wrote:
> Still used.

Done.

[dpapad, 2011-02-23 01:09:29]

Still working on webplugin_delegate_pepper.cc:1225 (cairo specific)

[vandebo (ex-Chrome), 2011-02-23 02:01:41]

You might as well update the copyright year for every file you touched.

http://codereview.chromium.org/6544028/diff/4064/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (left):

http://codereview.chromium.org/6544028/diff/4064/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:21: #include
"printing/native_metafile.h"
Still use this.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:140:
printing::NativeMetafileFactory::CreateMetafile());
indention.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:263:
printing::NativeMetafileFactory::CreateMetafile());
indention.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_pepper.cc:1226:
printing::NativeMetafileFactory::CreateMetafileFrom(context));
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/image.cc#newcode11
printing/image.cc:11: #include "base/scoped_ptr.h"
order

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h#n...
printing/native_metafile.h:11: #include "printing/native_metafile_win.h"
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h#n...
printing/native_metafile.h:18: #endif /* PRINTING_NATIVE_METAFILE_H_ */
space space // not space /*

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:1: /// Copyright (c) 2011 The Chromium
Authors. All rights reserved.
extra /

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:9: /* static */
// static

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:31: } // namespace printing
two spaces before a comment.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.h:12: #include "printing/emf_win.h"
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.h:44: #endif /*
PRINTING_NATIVE_METAFILE_FACTORY_H_ */
style.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_lin...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_lin...
printing/native_metafile_linux.h:89: #endif /* PRINTING_NATIVE_METAFILE_LINUX_H_
*/
style

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_mac.h
File printing/native_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_mac...
printing/native_metafile_mac.h:90: #endif /* PRINTING_NATIVE_METAFILE_MAC_H_ */
style

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:24: 
extra line.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:42: // Load an EMF file.
extra spaces

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:104: #endif /* PRINTING_NATIVE_METAFILE_WIN_H_ */
style

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:26: class PdfMetafile: public NativeMetafile {
PdfMetafile : public NativeMetafile
           ^ space.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:100: virtual CGPDFDocumentRef GetPDFDocument()
const;
virtual private?

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.cc:75: surface_(NULL), context_(NULL) {
style (I know you didn't make the mistake, but you can fix it).

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

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:27: class PdfPsMetafile: public NativeMetafile 
{
space

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:30:
printing::NativeMetafileFactory::CreateMetafile());
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:60:
printing::NativeMetafileFactory::CreateMetafile());
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:76:
printing::NativeMetafileFactory::CreateMetafile());
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:69: (PdfPsMetafile::kLeftMarginInInch
+
indention.

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:72: (PdfPsMetafile::kTopMarginInInch
+
indention.

[dpapad, 2011-02-24 20:56:59]

http://codereview.chromium.org/6544028/diff/4064/chrome/browser/printing/prin...
File chrome/browser/printing/print_view_manager.cc (left):

http://codereview.chromium.org/6544028/diff/4064/chrome/browser/printing/prin...
chrome/browser/printing/print_view_manager.cc:21: #include
"printing/native_metafile.h"
On 2011/02/23 02:01:41, vandebo wrote:
> Still use this.

Done.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:140:
printing::NativeMetafileFactory::CreateMetafile());
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_win.cc:263:
printing::NativeMetafileFactory::CreateMetafile());
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/webplugin_de...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/4064/chrome/renderer/webplugin_de...
chrome/renderer/webplugin_delegate_pepper.cc:1226:
printing::NativeMetafileFactory::CreateMetafileFrom(context));
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/image.cc#newcode11
printing/image.cc:11: #include "base/scoped_ptr.h"
On 2011/02/23 02:01:41, vandebo wrote:
> order

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h#n...
printing/native_metafile.h:11: #include "printing/native_metafile_win.h"
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile.h#n...
printing/native_metafile.h:18: #endif /* PRINTING_NATIVE_METAFILE_H_ */
On 2011/02/23 02:01:41, vandebo wrote:
> space space // not space /*

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:1: /// Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2011/02/23 02:01:41, vandebo wrote:
> extra /

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:9: /* static */
On 2011/02/23 02:01:41, vandebo wrote:
> // static

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.cc:31: } // namespace printing
On 2011/02/23 02:01:41, vandebo wrote:
> two spaces before a comment.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.h:12: #include "printing/emf_win.h"
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_fac...
printing/native_metafile_factory.h:44: #endif /*
PRINTING_NATIVE_METAFILE_FACTORY_H_ */
On 2011/02/23 02:01:41, vandebo wrote:
> style.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_lin...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_lin...
printing/native_metafile_linux.h:89: #endif /* PRINTING_NATIVE_METAFILE_LINUX_H_
*/
On 2011/02/23 02:01:41, vandebo wrote:
> style

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_mac.h
File printing/native_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_mac...
printing/native_metafile_mac.h:90: #endif /* PRINTING_NATIVE_METAFILE_MAC_H_ */
On 2011/02/23 02:01:41, vandebo wrote:
> style

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:24: 
On 2011/02/23 02:01:41, vandebo wrote:
> extra line.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:42: // Load an EMF file.
On 2011/02/23 02:01:41, vandebo wrote:
> extra spaces

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/native_metafile_win...
printing/native_metafile_win.h:104: #endif /* PRINTING_NATIVE_METAFILE_WIN_H_ */
On 2011/02/23 02:01:41, vandebo wrote:
> style

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:26: class PdfMetafile: public NativeMetafile {
On 2011/02/23 02:01:41, vandebo wrote:
> PdfMetafile : public NativeMetafile
>            ^ space.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_metafile_mac.h#...
printing/pdf_metafile_mac.h:100: virtual CGPDFDocumentRef GetPDFDocument()
const;
On 2011/02/23 02:01:41, vandebo wrote:
> virtual private?

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.cc:75: surface_(NULL), context_(NULL) {
On 2011/02/23 02:01:41, vandebo wrote:
> style (I know you didn't make the mistake, but you can fix it).

Done.

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

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo.h:27: class PdfPsMetafile: public NativeMetafile 
{
On 2011/02/23 02:01:41, vandebo wrote:
> space

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
File printing/pdf_ps_metafile_cairo_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:30:
printing::NativeMetafileFactory::CreateMetafile());
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:60:
printing::NativeMetafileFactory::CreateMetafile());
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/pdf_ps_metafile_cai...
printing/pdf_ps_metafile_cairo_unittest.cc:76:
printing::NativeMetafileFactory::CreateMetafile());
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:69: (PdfPsMetafile::kLeftMarginInInch
+
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/4064/printing/print_settings_init...
printing/print_settings_initializer_gtk.cc:72: (PdfPsMetafile::kTopMarginInInch
+
On 2011/02/23 02:01:41, vandebo wrote:
> indention.

Done.

http://codereview.chromium.org/6544028/diff/25002/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (left):

http://codereview.chromium.org/6544028/diff/25002/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:26: public:
Deleted this part because only PDF is supported.

[vandebo (ex-Chrome), 2011-02-24 22:42:50]

Getting close.

A better subject for this issue would be: "Create a Factory for NativeMetafile."

http://codereview.chromium.org/6544028/diff/28001/chrome/renderer/webplugin_d...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/28001/chrome/renderer/webplugin_d...
chrome/renderer/webplugin_delegate_pepper.cc:1229:
printing::PdfPsMetafile::FromCairoContext(context);
4 spaces.

http://codereview.chromium.org/6544028/diff/28001/chrome/utility/utility_thre...
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/6544028/diff/28001/chrome/utility/utility_thre...
chrome/utility/utility_thread.cc:146: render_dpi, page_ranges,
Args should either be one per line, or as dense as the 80 column limit allows.

http://codereview.chromium.org/6544028/diff/28001/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6544028/diff/28001/printing/image.cc#newcode152
printing/image.cc:152: return LoadMetafile(*metafile.get());
I think just *metafile will work.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
printing/native_metafile_factory.cc:6: 
But the headers for the implementations need to be here.

We prefer to minimize the number of includes in headers, pushing them into the
.cc files where necessary.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
printing/native_metafile_factory.h:11: #if defined(OS_WIN)
I don't think the this needs the headers for the implementations.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_li...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_li...
printing/native_metafile_linux.h:23: // This class uses Cairo graphics library
to generate PostScript/PDF stream
This file shouldn't be cairo specific (even though it is right now).  Maybe
clean up this comment and what ever else you can.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_wi...
printing/native_metafile_win.h:21: // Simple wrapper class that manage an EMF
data stream and its virtual HDC.
Same here, this will eventually not be EMF specific.

http://codereview.chromium.org/6544028/diff/28001/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo.h (left):

http://codereview.chromium.org/6544028/diff/28001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:59: FileFormat GetFileFormat() const { return
format_; }
Since you removed the constructor that takes the file format, you can also
remove this accessor, and the FileFormat enum from native_metafile_linux.h  Then
again, maybe better to do this in a different CL.

http://codereview.chromium.org/6544028/diff/28001/printing/pdf_ps_metafile_ca...
File printing/pdf_ps_metafile_cairo_unittest.cc (left):

http://codereview.chromium.org/6544028/diff/28001/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo_unittest.cc:85: TEST_F(PdfPsTest, Ps) {
Since this CL breaks the PS construction, maybe that other CL should go in
first...

http://codereview.chromium.org/6544028/diff/28001/printing/printing_context_c...
File printing/printing_context_cairo.cc (left):

http://codereview.chromium.org/6544028/diff/28001/printing/printing_context_c...
printing/printing_context_cairo.cc:16: 
This space is actually useful, it separates system defines from out defines.

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

http://codereview.chromium.org/6544028/diff/28001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:27: #if defined(OS_LINUX)
per platform includes generally go at the end.

[dpapad, 2011-02-26 01:42:29]

Fixed style issues, removed NativeMetafileFactory from unittests, tested on
linux, mac, win.

http://codereview.chromium.org/6544028/diff/28001/chrome/renderer/webplugin_d...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/28001/chrome/renderer/webplugin_d...
chrome/renderer/webplugin_delegate_pepper.cc:1229:
printing::PdfPsMetafile::FromCairoContext(context);
On 2011/02/24 22:42:50, vandebo wrote:
> 4 spaces.

Done.

http://codereview.chromium.org/6544028/diff/28001/chrome/utility/utility_thre...
File chrome/utility/utility_thread.cc (right):

http://codereview.chromium.org/6544028/diff/28001/chrome/utility/utility_thre...
chrome/utility/utility_thread.cc:146: render_dpi, page_ranges,
On 2011/02/24 22:42:50, vandebo wrote:
> Args should either be one per line, or as dense as the 80 column limit allows.

Done.

http://codereview.chromium.org/6544028/diff/28001/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6544028/diff/28001/printing/image.cc#newcode152
printing/image.cc:152: return LoadMetafile(*metafile.get());
On 2011/02/24 22:42:50, vandebo wrote:
> I think just *metafile will work.

Done.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
printing/native_metafile_factory.cc:6: 
On 2011/02/24 22:42:50, vandebo wrote:
> But the headers for the implementations need to be here.
> 
> We prefer to minimize the number of includes in headers, pushing them into the
> .cc files where necessary.

Done.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_fa...
printing/native_metafile_factory.h:11: #if defined(OS_WIN)
On 2011/02/24 22:42:50, vandebo wrote:
> I don't think the this needs the headers for the implementations.

Done.

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/28001/printing/native_metafile_wi...
printing/native_metafile_win.h:21: // Simple wrapper class that manage an EMF
data stream and its virtual HDC.
On 2011/02/24 22:42:50, vandebo wrote:
> Same here, this will eventually not be EMF specific.

Done.

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

http://codereview.chromium.org/6544028/diff/28001/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:27: #if defined(OS_LINUX)
On 2011/02/24 22:42:50, vandebo wrote:
> per platform includes generally go at the end.

Done.

[dpapad, 2011-02-28 17:59:38]

Fixed mac unittests and removed 
include "native_metafile_factory.h" from emf_win.h pdf_ps_metafile_cairo.h and
pdf_metafile_mac.h

[vandebo (ex-Chrome), 2011-02-28 21:43:59]

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile.h#...
printing/native_metafile.h:8: #include "base/basictypes.h"
nit: should stay build/build_config.h no?

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:12: #include "base/gtest_prod_util.h"
What is this include for?

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

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:11: #include "base/gtest_prod_util.h"
And this one?

[dpapad, 2011-02-28 22:03:38]

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile.h
File printing/native_metafile.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile.h#...
printing/native_metafile.h:8: #include "base/basictypes.h"
On 2011/02/28 21:44:00, vandebo wrote:
> nit: should stay build/build_config.h no?

I guess it has to stay.

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:12: #include "base/gtest_prod_util.h"
On 2011/02/28 21:44:00, vandebo wrote:
> What is this include for?

This is where the macro FRIEND_TEST_ALL_PREFIXES is defined (is being used in
the private section of the class to declare the unit_test as a friend, so it can
use the constructor directly).

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

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:11: #include "base/gtest_prod_util.h"
On 2011/02/28 21:44:00, vandebo wrote:
> And this one?

Same reason for this one (also appears in emf_win.h).

[James Hawkins, 2011-02-28 22:26:45]

Is there a design somewhere where I can read the changes that are happening to
these interfaces? I have concerns.

http://codereview.chromium.org/6544028/diff/35006/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6544028/diff/35006/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:22: #include
"printing/native_metafile_factory.h"
nit: The rule for alphabetizing includes is to remove all punctuation
characters, then sort. That would put native_metafile_factory.h before
native_metafile.h.

This applies to a few other files in this CL.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_fa...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_fa...
printing/native_metafile_factory.h:19: 
nit: Remove blank line.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_fa...
printing/native_metafile_factory.h:20: // This method takes care of returning a
pointer to the appropriate
s/takes care of returning/returns/

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:8: #include <string>
This include is unused in this file.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:23: // This class is used to generate a PDF
stream
nit: You haven't used all of the 80 cols here.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:24: // and to  store rendering results in a
string buffer.
nit: Remove double space.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:30: // Note: Only call in the renderer to
allocate rendering resources.
Note: This note is confusing.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:50: double height_in_points,
nit: Line up parameters. If they don't fit past the "(", then start on the next
line and indent 4 spaces.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_wi...
printing/native_metafile_win.h:26: // Initializes the data stream with the data
in |src_buffer|. Returns true on success.
80 cols.

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:90: // To create PDF data, callers should also call
Init() to set up the
I don't think you mean to use the word "also" in these sentences.

[vandebo (ex-Chrome), 2011-02-28 22:36:47]

On 2011/02/28 22:26:45, James Hawkins wrote:
> Is there a design somewhere where I can read the changes that are happening to
> these interfaces? I have concerns.

There is no explicit design.  The problem to be solved is that with the Skia
backend we will have to support multiple types of "NativeMetafiles" per
platform.  The solution is to make them implement an ABC interface and use a
factory to create the appropriate type based on current settings.

dpapad is working on a follow up CL to unify the the NativeMetafile interface
across platforms as much as possible.  Further work will change that interface
again in order to introduce the Skia implementations.

Feel free to voice your concerns here or chat with us about it.

[James Hawkins, 2011-02-28 22:38:29]

On 2011/02/28 22:36:47, vandebo wrote:
> On 2011/02/28 22:26:45, James Hawkins wrote:
> > Is there a design somewhere where I can read the changes that are happening
to
> > these interfaces? I have concerns.
> 
> There is no explicit design.  The problem to be solved is that with the Skia
> backend we will have to support multiple types of "NativeMetafiles" per
> platform.  The solution is to make them implement an ABC interface and use a
> factory to create the appropriate type based on current settings.
> 
> dpapad is working on a follow up CL to unify the the NativeMetafile interface
> across platforms as much as possible.  Further work will change that interface
> again in order to introduce the Skia implementations.
> 
> Feel free to voice your concerns here or chat with us about it.

Ok. I am/was mostly concerned about the NativeMetafile interface being defined
per-platform. My preference would be to have the refactoring in this CL, but I
won't push it as it's quite large already.

[dpapad, 2011-02-28 23:50:58]

http://codereview.chromium.org/6544028/diff/35006/chrome/browser/printing/pri...
File chrome/browser/printing/print_view_manager.cc (right):

http://codereview.chromium.org/6544028/diff/35006/chrome/browser/printing/pri...
chrome/browser/printing/print_view_manager.cc:22: #include
"printing/native_metafile_factory.h"
On 2011/02/28 22:26:45, James Hawkins wrote:
> nit: The rule for alphabetizing includes is to remove all punctuation
> characters, then sort. That would put native_metafile_factory.h before
> native_metafile.h.
> 
> This applies to a few other files in this CL.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_fa...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_fa...
printing/native_metafile_factory.h:19: 
On 2011/02/28 22:26:45, James Hawkins wrote:
> nit: Remove blank line.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:8: #include <string>
On 2011/02/28 22:26:45, James Hawkins wrote:
> This include is unused in this file.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:23: // This class is used to generate a PDF
stream
On 2011/02/28 22:26:45, James Hawkins wrote:
> nit: You haven't used all of the 80 cols here.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:24: // and to  store rendering results in a
string buffer.
On 2011/02/28 22:26:45, James Hawkins wrote:
> nit: Remove double space.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:30: // Note: Only call in the renderer to
allocate rendering resources.
On 2011/02/28 22:26:45, James Hawkins wrote:
> Note: This note is confusing.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_li...
printing/native_metafile_linux.h:50: double height_in_points,
On 2011/02/28 22:26:45, James Hawkins wrote:
> nit: Line up parameters. If they don't fit past the "(", then start on the
next
> line and indent 4 spaces.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_win.h
File printing/native_metafile_win.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/native_metafile_wi...
printing/native_metafile_win.h:26: // Initializes the data stream with the data
in |src_buffer|. Returns true on success.
On 2011/02/28 22:26:45, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h
File printing/pdf_metafile_mac.h (right):

http://codereview.chromium.org/6544028/diff/35006/printing/pdf_metafile_mac.h...
printing/pdf_metafile_mac.h:90: // To create PDF data, callers should also call
Init() to set up the
On 2011/02/28 22:26:45, James Hawkins wrote:
> I don't think you mean to use the word "also" in these sentences.

Done.

[dpapad, 2011-03-01 17:38:11]

Patch 13 passed the Mac trybots, Patch 15 passed win, linux. Patch 16 is a
simple style fix.

[vandebo (ex-Chrome), 2011-03-01 19:01:00]

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
chrome/common/common_param_traits_unittest.cc:235: // Tests printing::Emf
serialization.
This is trying to tests NativeMetafile, not EMF.  It just does a poor job of
testing across platforms.  So it should use NativeMatafile and therefore the
factory.  Don't forget to update the headers.

http://codereview.chromium.org/6544028/diff/33020/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/6544028/diff/33020/printing/printing.gyp#newco...
printing/printing.gyp:39: 'native_metafile.h',
Alphabetize as james pointed out.

[kmadhusu, 2011-03-01 19:42:38]

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
chrome/common/common_param_traits_unittest.cc:17: #if defined(OS_WIN)
Platform-specific headers needs to follow the cross platform headers in a
separate section. 

Eg:
#include "foo/bar/someclass.h"

#include <set>

#include "foo/bar/a.h"
#include "foo/bar/b.h"

#if defined(OS_WIN)
#include "foo/bar/a_win.h"
#include "foo/bar/b_win.h"
#elif defined(OS_POSIX)
#include "foo/bar/a_posix.h"
#endif

http://codereview.chromium.org/6544028/diff/33020/chrome/plugin/webplugin_del...
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/plugin/webplugin_del...
chrome/plugin/webplugin_delegate_stub.cc:19: #include
"printing/native_metafile_factory.h"
Alphabetize the header files.

#include "printing/native_metafile_factory.h"
#include "printing/native_metafile.h"

http://codereview.chromium.org/6544028/diff/33020/chrome/renderer/webplugin_d...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/renderer/webplugin_d...
chrome/renderer/webplugin_delegate_pepper.cc:56: #include
"printing/pdf_ps_metafile_cairo.h"
As per Chromium coding-style, platform-specific headers needs to follow the
cross platform headers in a separate section.

http://codereview.chromium.org/6544028/diff/33020/printing/print_settings_ini...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/33020/printing/print_settings_ini...
printing/print_settings_initializer_gtk.cc:64: static_cast<int>(
Argument indentation is not correct.

Lines 64, 66, 68, 71 -> needs 4 space indentation.
Lines  69, 70, 72 & 73 -> 4 space indentation is not required.

[dpapad, 2011-03-01 22:41:24]

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
File chrome/common/common_param_traits_unittest.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
chrome/common/common_param_traits_unittest.cc:17: #if defined(OS_WIN)
On 2011/03/01 19:42:38, kmadhusu wrote:
> Platform-specific headers needs to follow the cross platform headers in a
> separate section. 
> 
> Eg:
> #include "foo/bar/someclass.h"
> 
> #include <set>
> 
> #include "foo/bar/a.h"
> #include "foo/bar/b.h"
> 
> #if defined(OS_WIN)
> #include "foo/bar/a_win.h"
> #include "foo/bar/b_win.h"
> #elif defined(OS_POSIX)
> #include "foo/bar/a_posix.h"
> #endif
> 

Done. I did it also for platform specific includes that already existed.

http://codereview.chromium.org/6544028/diff/33020/chrome/common/common_param_...
chrome/common/common_param_traits_unittest.cc:235: // Tests printing::Emf
serialization.
On 2011/03/01 19:01:01, vandebo wrote:
> This is trying to tests NativeMetafile, not EMF.  It just does a poor job of
> testing across platforms.  So it should use NativeMatafile and therefore the
> factory.  Don't forget to update the headers.

Done. I also removed this test from the friend list of emf_win.h since it no
longer needs to invoke the constructor.

http://codereview.chromium.org/6544028/diff/33020/chrome/plugin/webplugin_del...
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/plugin/webplugin_del...
chrome/plugin/webplugin_delegate_stub.cc:19: #include
"printing/native_metafile_factory.h"
On 2011/03/01 19:42:38, kmadhusu wrote:
> Alphabetize the header files.
> 
> #include "printing/native_metafile_factory.h"
> #include "printing/native_metafile.h"

Done.

http://codereview.chromium.org/6544028/diff/33020/chrome/renderer/webplugin_d...
File chrome/renderer/webplugin_delegate_pepper.cc (right):

http://codereview.chromium.org/6544028/diff/33020/chrome/renderer/webplugin_d...
chrome/renderer/webplugin_delegate_pepper.cc:56: #include
"printing/pdf_ps_metafile_cairo.h"
On 2011/03/01 19:42:38, kmadhusu wrote:
> As per Chromium coding-style, platform-specific headers needs to follow the
> cross platform headers in a separate section.

Done.

http://codereview.chromium.org/6544028/diff/33020/printing/print_settings_ini...
File printing/print_settings_initializer_gtk.cc (right):

http://codereview.chromium.org/6544028/diff/33020/printing/print_settings_ini...
printing/print_settings_initializer_gtk.cc:64: static_cast<int>(
On 2011/03/01 19:42:38, kmadhusu wrote:
> Argument indentation is not correct.
> 
> Lines 64, 66, 68, 71 -> needs 4 space indentation.
> Lines  69, 70, 72 & 73 -> 4 space indentation is not required.

Done.

http://codereview.chromium.org/6544028/diff/33020/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/6544028/diff/33020/printing/printing.gyp#newco...
printing/printing.gyp:39: 'native_metafile.h',
On 2011/03/01 19:01:01, vandebo wrote:
> Alphabetize as james pointed out.

Done.

[vandebo (ex-Chrome), 2011-03-01 22:50:50]

LGTM

[Lei Zhang, 2011-03-02 00:05:15]

LGTM with nits below.

http://codereview.chromium.org/6544028/diff/38071/chrome/plugin/webplugin_del...
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/6544028/diff/38071/chrome/plugin/webplugin_del...
chrome/plugin/webplugin_delegate_stub.cc:18: #include
"printing/native_metafile_factory.h"
nit: new includes are Windows only. Same goes for:
chrome/renderer/webplugin_delegate_proxy.cc
chrome/utility/utility_thread.cc
chrome/service/service_utility_process_host.cc

http://codereview.chromium.org/6544028/diff/38071/printing/native_metafile_li...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/38071/printing/native_metafile_li...
printing/native_metafile_linux.h:25: virtual ~NativeMetafile() {};
nit: no need for ';'. Same goes for native_metafile_{mac,win}.h

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

http://codereview.chromium.org/6544028/diff/38071/printing/pdf_ps_metafile_ca...
printing/pdf_ps_metafile_cairo.h:31: // Initializes to a fresh new metafile.
Returns true on success.
you can get rid of all the comments here, since they are in
printing/native_metafile_linux.h already. Having two copies means they may get
out of sync.

Instead, just write:
// NativeMetafile implementation.

See example for BackgroundContents in
content/browser/tab_contents/background_contents.h.

http://codereview.chromium.org/6544028/diff/38071/printing/printed_document.h
File printing/printed_document.h (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printed_document.h...
printing/printed_document.h:15: #include "printing/native_metafile.h"
You can probably forward declare NativeMetafile in the printing namespace
instead.

http://codereview.chromium.org/6544028/diff/38071/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printing.gyp#newco...
printing/printing.gyp:41: 'native_metafile.h',
nit: this should go after native_metafile_win.h, based on '_' before '.'.

http://codereview.chromium.org/6544028/diff/38071/printing/printing_context_c...
File printing/printing_context_cairo.cc (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printing_context_c...
printing/printing_context_cairo.cc:13: #include
"printing/pdf_ps_metafile_cairo.h"
nit: would you mind adding a line to separate this from the system headers above
it? (see example in #else block below)

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

http://codereview.chromium.org/6544028/diff/38071/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:25: #include
"printing/native_metafile_factory.h"
nit: mac only.

http://codereview.chromium.org/6544028/diff/38071/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:26: #include
"printing/native_metafile.h"
nit: POSIX only.

[dpapad, 2011-03-02 01:17:18]

http://codereview.chromium.org/6544028/diff/38071/chrome/plugin/webplugin_del...
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/6544028/diff/38071/chrome/plugin/webplugin_del...
chrome/plugin/webplugin_delegate_stub.cc:18: #include
"printing/native_metafile_factory.h"
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: new includes are Windows only. Same goes for:
> chrome/renderer/webplugin_delegate_proxy.cc
> chrome/utility/utility_thread.cc
> chrome/service/service_utility_process_host.cc

Done. Also I think that the same holds for the already existing include
"printing/native_metafile.h" (for all these files). So i moved this as well
inside the conditional includes.

http://codereview.chromium.org/6544028/diff/38071/chrome/renderer/webplugin_d...
File chrome/renderer/webplugin_delegate_proxy.cc (right):

http://codereview.chromium.org/6544028/diff/38071/chrome/renderer/webplugin_d...
chrome/renderer/webplugin_delegate_proxy.cc:18: #include "base/scoped_ptr.h"
scoped_ptr is used in line 668 (outside OS_WIN) so I am keeping it here.

http://codereview.chromium.org/6544028/diff/38071/printing/native_metafile_li...
File printing/native_metafile_linux.h (right):

http://codereview.chromium.org/6544028/diff/38071/printing/native_metafile_li...
printing/native_metafile_linux.h:25: virtual ~NativeMetafile() {};
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: no need for ';'. Same goes for native_metafile_{mac,win}.h

Done.

http://codereview.chromium.org/6544028/diff/38071/printing/printed_document.h
File printing/printed_document.h (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printed_document.h...
printing/printed_document.h:15: #include "printing/native_metafile.h"
On 2011/03/02 00:05:15, Lei Zhang wrote:
> You can probably forward declare NativeMetafile in the printing namespace
> instead.

Done.

http://codereview.chromium.org/6544028/diff/38071/printing/printing.gyp
File printing/printing.gyp (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printing.gyp#newco...
printing/printing.gyp:41: 'native_metafile.h',
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: this should go after native_metafile_win.h, based on '_' before '.'.

Done.

http://codereview.chromium.org/6544028/diff/38071/printing/printing_context_c...
File printing/printing_context_cairo.cc (right):

http://codereview.chromium.org/6544028/diff/38071/printing/printing_context_c...
printing/printing_context_cairo.cc:13: #include
"printing/pdf_ps_metafile_cairo.h"
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: would you mind adding a line to separate this from the system headers
above
> it? (see example in #else block below)

Done.

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

http://codereview.chromium.org/6544028/diff/38071/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:25: #include
"printing/native_metafile_factory.h"
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: mac only.

Done.

http://codereview.chromium.org/6544028/diff/38071/webkit/plugins/ppapi/ppapi_...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:26: #include
"printing/native_metafile.h"
On 2011/03/02 00:05:15, Lei Zhang wrote:
> nit: POSIX only.

Done.

[vandebo (ex-Chrome), 2011-03-02 19:18:33]

LG