Adding CreateFromData to NativeMetafileFactory

Adding CreateFromData to NativeMetafileFactory

Also making Create method calling Init on the metafile before returning it.
Usually clients of the NativeMetafileFactory call Create and the Init or
InitFromData on the object to initialize it. Now the returned object will
already be initialized.


BUG=NONE
TEST=NONE

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

[dpapad, 2011-03-24 19:54:04]

http://codereview.chromium.org/6696076/diff/1/chrome/renderer/print_web_view_...
File chrome/renderer/print_web_view_helper_linux.cc (left):

http://codereview.chromium.org/6696076/diff/1/chrome/renderer/print_web_view_...
chrome/renderer/print_web_view_helper_linux.cc:174: metafile->Init();
Now the metafile is already initialized before this method is called (in
PrintWebViewHelper::PrintPages and PrintWebViewHelper::CreatePreviewDocument).

[vandebo (ex-Chrome), 2011-03-24 20:11:55]

http://codereview.chromium.org/6696076/diff/5001/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/6696076/diff/5001/chrome/renderer/mock_printer...
chrome/renderer/mock_printer.cc:143: if (!metafile.get())
This one, we probably want to CHECK(metafile.get())

http://codereview.chromium.org/6696076/diff/5001/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6696076/diff/5001/printing/image.cc#newcode151
printing/image.cc:151: data.size()));
look for null metafile.

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

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.cc:21: metafile = new printing::Emf;
This code is duplicated.  You should pull it out into a local function.

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

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.h:21: // Init() on it.
Null is returned if Init() fails.

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.h:26: // Init(src_buffer,_buffer_size) on it.
InitFromData & above too.

[dpapad, 2011-03-24 21:08:14]

http://codereview.chromium.org/6696076/diff/5001/chrome/renderer/mock_printer.cc
File chrome/renderer/mock_printer.cc (right):

http://codereview.chromium.org/6696076/diff/5001/chrome/renderer/mock_printer...
chrome/renderer/mock_printer.cc:143: if (!metafile.get())
On 2011/03/24 20:11:55, vandebo wrote:
> This one, we probably want to CHECK(metafile.get())

Done.

http://codereview.chromium.org/6696076/diff/5001/printing/image.cc
File printing/image.cc (right):

http://codereview.chromium.org/6696076/diff/5001/printing/image.cc#newcode151
printing/image.cc:151: data.size()));
On 2011/03/24 20:11:55, vandebo wrote:
> look for null metafile.

Done.

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

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.cc:21: metafile = new printing::Emf;
On 2011/03/24 20:11:55, vandebo wrote:
> This code is duplicated.  You should pull it out into a local function.

Done.

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

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.h:21: // Init() on it.
On 2011/03/24 20:11:55, vandebo wrote:
> Null is returned if Init() fails.

Done.

http://codereview.chromium.org/6696076/diff/5001/printing/native_metafile_fac...
printing/native_metafile_factory.h:26: // Init(src_buffer,_buffer_size) on it.
On 2011/03/24 20:11:55, vandebo wrote:
> InitFromData & above too.

Done.

[vandebo (ex-Chrome), 2011-03-24 21:21:58]

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

http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:23: CHECK(metafile.get());
Doing the same thing as before is fine here - return if it fails.

http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:69: CHECK(metafile.get());
And here.

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

http://codereview.chromium.org/6696076/diff/5003/printing/native_metafile_fac...
printing/native_metafile_factory.cc:39: NativeMetafile*
NativeMetafileFactory::CreateNewMetafile(){
This doesn't need to be a class member.  We prefer to make these kinds of things
simple functions in an anonymous namespace.

[dpapad, 2011-03-24 21:39:12]

On 2011/03/24 21:21:58, vandebo wrote:
>
http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
> File chrome/renderer/print_web_view_helper_mac.mm (right):
> 
>
http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
> chrome/renderer/print_web_view_helper_mac.mm:23: CHECK(metafile.get());
> Doing the same thing as before is fine here - return if it fails.
> 
>
http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
> chrome/renderer/print_web_view_helper_mac.mm:69: CHECK(metafile.get());
> And here.
> 
>
http://codereview.chromium.org/6696076/diff/5003/printing/native_metafile_fac...
> File printing/native_metafile_factory.cc (right):
> 
>
http://codereview.chromium.org/6696076/diff/5003/printing/native_metafile_fac...
> printing/native_metafile_factory.cc:39: NativeMetafile*
> NativeMetafileFactory::CreateNewMetafile(){
> This doesn't need to be a class member.  We prefer to make these kinds of
things
> simple functions in an anonymous namespace.

I left it as a class member because this class is a friend of all metafiles (in
order to be able to construct them). By having it as a independent function I
will have to make this function friend (and unfriend the factory). Is that
preferable?

[vandebo (ex-Chrome), 2011-03-24 21:46:14]

On 2011/03/24 21:39:12, dpapad wrote:
> On 2011/03/24 21:21:58, vandebo wrote:
http://codereview.chromium.org/6696076/diff/5003/printing/native_metafile_fac...
> > printing/native_metafile_factory.cc:39: NativeMetafile*
> > NativeMetafileFactory::CreateNewMetafile(){
> > This doesn't need to be a class member.  We prefer to make these kinds of
> things
> > simple functions in an anonymous namespace.
> 
> I left it as a class member because this class is a friend of all metafiles
(in
> order to be able to construct them). By having it as a independent function I
> will have to make this function friend (and unfriend the factory). Is that
> preferable?

Fair enough (there is a hidden class member dependency).

[dpapad, 2011-03-24 22:02:37]

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

http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:23: CHECK(metafile.get());
On 2011/03/24 21:21:58, vandebo wrote:
> Doing the same thing as before is fine here - return if it fails.

Done.

http://codereview.chromium.org/6696076/diff/5003/chrome/renderer/print_web_vi...
chrome/renderer/print_web_view_helper_mac.mm:69: CHECK(metafile.get());
On 2011/03/24 21:21:58, vandebo wrote:
> And here.

Done.

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

http://codereview.chromium.org/6696076/diff/5003/printing/native_metafile_fac...
printing/native_metafile_factory.cc:39: NativeMetafile*
NativeMetafileFactory::CreateNewMetafile(){
On 2011/03/24 21:21:58, vandebo wrote:
> This doesn't need to be a class member.  We prefer to make these kinds of
things
> simple functions in an anonymous namespace.

I left it as a class member because this class is a friend of all metafiles (in
order to be able to construct them). By having it as a independent function I
will have to make this function friend (and unfriend the factory). Is that
preferable?

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
File printing/native_metafile_factory.h (right):

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
printing/native_metafile_factory.h:33: 
Ok, so I am leaving this as a class member since it takes advantage of this
class being friend of the metafiles.

[vandebo (ex-Chrome), 2011-03-24 22:15:42]

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
printing/native_metafile_factory.cc:19: NativeMetafile* metafile =
CreateNewMetafile();
You could make metafile a scoped_ptr to make its lifetime more resilient to
changes.

[dpapad, 2011-03-24 22:37:53]

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
File printing/native_metafile_factory.cc (right):

http://codereview.chromium.org/6696076/diff/19/printing/native_metafile_facto...
printing/native_metafile_factory.cc:19: NativeMetafile* metafile =
CreateNewMetafile();
On 2011/03/24 22:15:42, vandebo wrote:
> You could make metafile a scoped_ptr to make its lifetime more resilient to
> changes.

Done.

[vandebo (ex-Chrome), 2011-03-24 22:39:36]

LGTM