Pass printing result to the browser....

Pass printing result to the browser.

The resulting PDF file will now be passed to the browser and be saved as "chromium_printing_test.pdf" under current directory.

BUG=9847
TEST=printing on linux should now generate chromium_printing_test.pdf in download directory. Printing on Windows should still work.

Patch contributed by minyu.huang@gmail.com

Committed as revision 25615.

[M-A Ruel, 2009-09-02 23:45:01]

http://codereview.chromium.org/172115/diff/1/3
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/172115/diff/1/3#newcode195
Line 195: // Used to pass resulting EMF from renderer to browser in printing.
Please merge both functions

http://codereview.chromium.org/172115/diff/1/4
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/172115/diff/1/4#newcode1344
Line 1344: IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_DuplicateSection,
Please destroy this message.

http://codereview.chromium.org/172115/diff/1/5
File chrome/common/temp_scaffolding_stubs.cc (right):

http://codereview.chromium.org/172115/diff/1/5#newcode333
Line 333: // NOTE: This is a quick hack for testing purpose.
TODO?

http://codereview.chromium.org/172115/diff/1/5#newcode338
Line 338: 
no line here

http://codereview.chromium.org/172115/diff/1/5#newcode361
Line 361: }  // namespace printing
one empty line here

http://codereview.chromium.org/172115/diff/1/12
File skia/ext/vector_canvas.h (right):

http://codereview.chromium.org/172115/diff/1/12#newcode11
Line 11: typedef struct _cairo cairo_t;
#if defined(__linux__)

http://codereview.chromium.org/172115/diff/1/12#newcode47
Line 47: // |is_opaque| is unused.
??

[tony, 2009-09-03 19:31:19]

http://codereview.chromium.org/172115/diff/1/5
File chrome/common/temp_scaffolding_stubs.cc (right):

http://codereview.chromium.org/172115/diff/1/5#newcode358
Line 358: metafile->SaveTo(FilePath("chromium_printing_test.pdf"));
Nit: I think you should put this in Downloads for now.  You can use
base/path_service.h to get DIR_DEFAULT_DOWNLOADS.

http://codereview.chromium.org/172115/diff/1/6
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/172115/diff/1/6#newcode60
Line 60: class TabContents;
Isn't this already forward declared above?

http://codereview.chromium.org/172115/diff/1/6#newcode71
Line 71: #if defined(OS_LINUX)
I think you can use the same impl between linux/mac so I would just remove the
#ifs here.

[myhuang, 2009-09-03 23:08:28]

Path set #2 and #3 are almost the same. I just forgot to remove one comment in
#2.

Please check these for me if possible. Thank you!
1. The usage of SharedMemory. (e.g. Any leak?)
2. Download the patch set, build it on windows, print a long web page to make
sure it really does print.

http://codereview.chromium.org/172115/diff/1/3
File chrome/browser/renderer_host/resource_message_filter.h (right):

http://codereview.chromium.org/172115/diff/1/3#newcode195
Line 195: // Used to pass resulting EMF from renderer to browser in printing.
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> Please merge both functions

Done.

http://codereview.chromium.org/172115/diff/1/4
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/172115/diff/1/4#newcode1344
Line 1344: IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_DuplicateSection,
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> Please destroy this message.

Done.

http://codereview.chromium.org/172115/diff/1/5
File chrome/common/temp_scaffolding_stubs.cc (right):

http://codereview.chromium.org/172115/diff/1/5#newcode333
Line 333: // NOTE: This is a quick hack for testing purpose.
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> TODO?

Done.

http://codereview.chromium.org/172115/diff/1/5#newcode338
Line 338: 
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> no line here

Done.

http://codereview.chromium.org/172115/diff/1/5#newcode358
Line 358: metafile->SaveTo(FilePath("chromium_printing_test.pdf"));
On 2009/09/03 19:31:19, tony wrote:
> Nit: I think you should put this in Downloads for now.  You can use
> base/path_service.h to get DIR_DEFAULT_DOWNLOADS.

Done.

http://codereview.chromium.org/172115/diff/1/5#newcode361
Line 361: }  // namespace printing
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> one empty line here

Done.

http://codereview.chromium.org/172115/diff/1/6
File chrome/common/temp_scaffolding_stubs.h (right):

http://codereview.chromium.org/172115/diff/1/6#newcode60
Line 60: class TabContents;
On 2009/09/03 19:31:19, tony wrote:
> Isn't this already forward declared above?

Done.

http://codereview.chromium.org/172115/diff/1/6#newcode71
Line 71: #if defined(OS_LINUX)
On 2009/09/03 19:31:19, tony wrote:
> I think you can use the same impl between linux/mac so I would just remove the
> #ifs here.

Done.

http://codereview.chromium.org/172115/diff/1/12
File skia/ext/vector_canvas.h (right):

http://codereview.chromium.org/172115/diff/1/12#newcode11
Line 11: typedef struct _cairo cairo_t;
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> #if defined(__linux__)

Done.

http://codereview.chromium.org/172115/diff/1/12#newcode47
Line 47: // |is_opaque| is unused.
On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> ??

Done.

[myhuang, 2009-09-03 23:10:26]

Wait... I forgot to change some other files so this won't compile. Sorry!!

On 2009/09/03 23:08:28, myhuang wrote:
> Path set #2 and #3 are almost the same. I just forgot to remove one comment in
> #2.
> 
> Please check these for me if possible. Thank you!
> 1. The usage of SharedMemory. (e.g. Any leak?)
> 2. Download the patch set, build it on windows, print a long web page to make
> sure it really does print.
> 
> http://codereview.chromium.org/172115/diff/1/3
> File chrome/browser/renderer_host/resource_message_filter.h (right):
> 
> http://codereview.chromium.org/172115/diff/1/3#newcode195
> Line 195: // Used to pass resulting EMF from renderer to browser in printing.
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > Please merge both functions
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/4
> File chrome/common/render_messages_internal.h (right):
> 
> http://codereview.chromium.org/172115/diff/1/4#newcode1344
> Line 1344: IPC_SYNC_MESSAGE_ROUTED1_1(ViewHostMsg_DuplicateSection,
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > Please destroy this message.
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/5
> File chrome/common/temp_scaffolding_stubs.cc (right):
> 
> http://codereview.chromium.org/172115/diff/1/5#newcode333
> Line 333: // NOTE: This is a quick hack for testing purpose.
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > TODO?
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/5#newcode338
> Line 338: 
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > no line here
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/5#newcode358
> Line 358: metafile->SaveTo(FilePath("chromium_printing_test.pdf"));
> On 2009/09/03 19:31:19, tony wrote:
> > Nit: I think you should put this in Downloads for now.  You can use
> > base/path_service.h to get DIR_DEFAULT_DOWNLOADS.
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/5#newcode361
> Line 361: }  // namespace printing
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > one empty line here
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/6
> File chrome/common/temp_scaffolding_stubs.h (right):
> 
> http://codereview.chromium.org/172115/diff/1/6#newcode60
> Line 60: class TabContents;
> On 2009/09/03 19:31:19, tony wrote:
> > Isn't this already forward declared above?
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/6#newcode71
> Line 71: #if defined(OS_LINUX)
> On 2009/09/03 19:31:19, tony wrote:
> > I think you can use the same impl between linux/mac so I would just remove
the
> > #ifs here.
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/12
> File skia/ext/vector_canvas.h (right):
> 
> http://codereview.chromium.org/172115/diff/1/12#newcode11
> Line 11: typedef struct _cairo cairo_t;
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > #if defined(__linux__)
> 
> Done.
> 
> http://codereview.chromium.org/172115/diff/1/12#newcode47
> Line 47: // |is_opaque| is unused.
> On 2009/09/02 23:45:01, Marc-Antoine Ruel wrote:
> > ??
> 
> Done.

[M-A Ruel, 2009-09-04 00:42:44]

A few comment before committing.

http://codereview.chromium.org/172115/diff/9001/10001
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/172115/diff/9001/10001#newcode327
Line 327: IPC_MESSAGE_HANDLER(ViewHostMsg_DuplicateSection, OnDuplicateSection)
delete that

http://codereview.chromium.org/172115/diff/9001/10010
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/172115/diff/9001/10010#newcode262
Line 262: routing_id(),
alignment +4

http://codereview.chromium.org/172115/diff/9001/10014
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/172115/diff/9001/10014#newcode31
Line 31: 
nit: I'm not a fan of empty lines

[myhuang, 2009-09-08 03:08:31]

http://codereview.chromium.org/172115/diff/9001/10010
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/172115/diff/9001/10010#newcode262
Line 262: routing_id(),
On 2009/09/04 00:42:44, Marc-Antoine Ruel wrote:
> alignment +4

Done.

http://codereview.chromium.org/172115/diff/9001/10014
File skia/ext/vector_platform_device_linux.cc (right):

http://codereview.chromium.org/172115/diff/9001/10014#newcode31
Line 31: 
On 2009/09/04 00:42:44, Marc-Antoine Ruel wrote:
> nit: I'm not a fan of empty lines

Done.

[M-A Ruel, 2009-09-08 14:38:51]

Committed with changes (except the DCHECK_GT I forgot to remove).

http://codereview.chromium.org/172115/diff/8039/8042
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/172115/diff/8039/8042#newcode1345
Line 1345: base::SharedMemoryHandle /* renderer handle */,
broke alignment

http://codereview.chromium.org/172115/diff/8039/8047
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/172115/diff/8039/8047#newcode81
Line 81: DCHECK_GT(buf_size, 0u);
uh

http://codereview.chromium.org/172115/diff/8039/8049
File chrome/renderer/print_web_view_helper_win.cc (right):

http://codereview.chromium.org/172115/diff/8039/8049#newcode206
Line 206: if (shrink <= 0) {
Fix this one too for consistency.

http://codereview.chromium.org/172115/diff/8039/8049#newcode233
Line 233: if (webkit_shrink <= 0) {
Please test both.