Linux: print page to file rather than using shared memory to send it to the b...

Linux: print page to file rather than using shared memory to send it to the browser.

BUG=9847
adapted from patch by <minyu.huang [at] gmail>


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

[tony, 2009-09-14 23:06:57]

http://codereview.chromium.org/203062/diff/1/10
File base/file_util.h (right):

http://codereview.chromium.org/203062/diff/1/10#newcode372
Line 372: int WriteFile(const int fd, const char* data, int size);
Can we just move this function into pdf_ps_metafile_linux.cc?

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode730
Line 730: (*temp_file_fd).fd = *fd_in_browser = -1;
It's kind of a shame that all this platform specific printing code has to be
pulled into resource message filter.  Marc-Antoine: Is there a better place for
this?  Alternately, maybe we should make a new class for it?

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
I guess by making this a singleton, there can be no other singleton
std::map<int, FilePath>s in the code?  Maybe that's ok?

http://codereview.chromium.org/203062/diff/1/5#newcode751
Line 751: (*temp_file_fd).fd = *fd_in_browser = fd;
Nit: Can you use -> here?

http://codereview.chromium.org/203062/diff/1/5#newcode752
Line 752: (*temp_file_fd).auto_close = true;
Nit: ->?

[Evan Stade, 2009-09-14 23:18:20]

http://codereview.chromium.org/203062/diff/1/10
File base/file_util.h (right):

http://codereview.chromium.org/203062/diff/1/10#newcode372
Line 372: int WriteFile(const int fd, const char* data, int size);
On 2009/09/14 23:06:57, tony wrote:
> Can we just move this function into pdf_ps_metafile_linux.cc?

no, because it provides the implementation for WriteFile(filepath,...)

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode730
Line 730: (*temp_file_fd).fd = *fd_in_browser = -1;
On 2009/09/14 23:06:57, tony wrote:
> It's kind of a shame that all this platform specific printing code has to be
> pulled into resource message filter.  Marc-Antoine: Is there a better place
for
> this?  Alternately, maybe we should make a new class for it?

I suspect it belongs in browser/printing in the long run

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/14 23:06:57, tony wrote:
> I guess by making this a singleton, there can be no other singleton
> std::map<int, FilePath>s in the code?  Maybe that's ok?

ah good point. It would be easy enough to make it a member variable.

http://codereview.chromium.org/203062/diff/1/5#newcode751
Line 751: (*temp_file_fd).fd = *fd_in_browser = fd;
On 2009/09/14 23:06:57, tony wrote:
> Nit: Can you use -> here?

Done.

http://codereview.chromium.org/203062/diff/1/5#newcode752
Line 752: (*temp_file_fd).auto_close = true;
On 2009/09/14 23:06:57, tony wrote:
> Nit: ->?

Done.

[myhuang, 2009-09-14 23:40:37]

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode730
Line 730: (*temp_file_fd).fd = *fd_in_browser = -1;
On 2009/09/14 23:18:20, Evan Stade wrote:
> On 2009/09/14 23:06:57, tony wrote:
> > It's kind of a shame that all this platform specific printing code has to be
> > pulled into resource message filter.  Marc-Antoine: Is there a better place
> for
> > this?  Alternately, maybe we should make a new class for it?
> 
> I suspect it belongs in browser/printing in the long run

We might not have easier solutions right now. I am not very sure if current
printing classes for Windows can fit our Linux printing architecture or not
(maybe not). For example, one big difference is that on Windows, we do printing
page by page (rendering page 1->DidPrintPage->rendering
page2->DidPrintPage->...), while on Linux, we generate all pages at the same
time(AllocateTempFileForPrinting->rendering all
pages->TempFileForPrintingWritten).
Briefly speaking, on Windows, we generate one EMF file for each page, while on
Linux, we generate just one PDF file fore all pages.

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/14 23:18:20, Evan Stade wrote:
> On 2009/09/14 23:06:57, tony wrote:
> > I guess by making this a singleton, there can be no other singleton
> > std::map<int, FilePath>s in the code?  Maybe that's ok?
> 
> ah good point. It would be easy enough to make it a member variable.

This part should also go into browser/printing/* or printing/* when we design
those new printing related classes on Linux.

[Evan Stade, 2009-09-14 23:48:34]

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/14 23:40:38, myhuang wrote:
> On 2009/09/14 23:18:20, Evan Stade wrote:
> > On 2009/09/14 23:06:57, tony wrote:
> > > I guess by making this a singleton, there can be no other singleton
> > > std::map<int, FilePath>s in the code?  Maybe that's ok?
> > 
> > ah good point. It would be easy enough to make it a member variable.
> 
> This part should also go into browser/printing/* or printing/* when we design
> those new printing related classes on Linux.

agreed. I also think that printing on linux ought to be hidden behind a
commandline switch for now.

[M-A Ruel, 2009-09-15 01:07:17]

http://codereview.chromium.org/203062/diff/1/10
File base/file_util.h (right):

http://codereview.chromium.org/203062/diff/1/10#newcode372
Line 372: int WriteFile(const int fd, const char* data, int size);
On 2009/09/14 23:18:20, Evan Stade wrote:
> no, because it provides the implementation for WriteFile(filepath,...)

Function overloading is frown upon. Please rename.
Also, why doesn't it take a FileDescriptor?

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/14 23:18:20, Evan Stade wrote:
> On 2009/09/14 23:06:57, tony wrote:
> > I guess by making this a singleton, there can be no other singleton
> > std::map<int, FilePath>s in the code?  Maybe that's ok?
> 
> ah good point. It would be easy enough to make it a member variable.

No, you can "taint" the singleton with a type but it is totally static so
putting it a member doesn't change anything.

It could be in PrintJobManager so it wouldn't need to be a singleton.

http://codereview.chromium.org/203062/diff/1/5#newcode756
Line 756: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
This doesn't need to be in ResourceMessageFilter since it's not a sync message.
In theory all that code should be in print_job_manager.

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

http://codereview.chromium.org/203062/diff/1/8#newcode1400
Line 1400: IPC_SYNC_MESSAGE_ROUTED1_0(ViewHostMsg_TempFileForPrintingWritten,
This message doesn't need to be sync.

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
I wonder what would be the final implementation.

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

http://codereview.chromium.org/203062/diff/1/2#newcode92
Line 92: bool SaveTo(const base::FileDescriptor& fd) const;
Function overloading is discouraged.

[myhuang, 2009-09-15 01:19:06]

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> I wonder what would be the final implementation.

My idea is to save the PDF as a temporary file and use the GTK+ APIs to send
this file directly to the printing system (CUPS). Since the API need a File
Name, we have to save the PDF on the disk first. (I knew it sound ugly.)

[M-A Ruel, 2009-09-15 01:28:02]

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
On 2009/09/15 01:19:06, myhuang wrote:
> My idea is to save the PDF as a temporary file and use the GTK+ APIs to send
> this file directly to the printing system (CUPS). Since the API need a File
> Name, we have to save the PDF on the disk first. (I knew it sound ugly.)

So, what's TODO then? Because unless mistaken, that's what is being done right
now.

[myhuang, 2009-09-15 01:35:05]

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
On 2009/09/15 01:28:02, Marc-Antoine Ruel wrote:
> On 2009/09/15 01:19:06, myhuang wrote:
> > My idea is to save the PDF as a temporary file and use the GTK+ APIs to send
> > this file directly to the printing system (CUPS). Since the API need a File
> > Name, we have to save the PDF on the disk first. (I knew it sound ugly.)
> 
> So, what's TODO then? Because unless mistaken, that's what is being done right
> now.

Sorry I did not make it clear. I meant that the use of IPC message
ViewHostMsg_TempFileForPrintingWritten is just for testing purpose, and I was
not very sure if we will keep it in the future.

[Evan Stade, 2009-09-15 01:35:34]

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> I wonder what would be the final implementation.

I have this question too. What are the drawbacks of this implementation?
performance?

[myhuang, 2009-09-15 01:53:46]

http://codereview.chromium.org/203062/diff/1/9
File chrome/renderer/print_web_view_helper_linux.cc (right):

http://codereview.chromium.org/203062/diff/1/9#newcode101
Line 101: // and the browser will save this PDF on the disk.
On 2009/09/15 01:35:34, Evan Stade wrote:
> On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> > I wonder what would be the final implementation.
> 
> I have this question too. What are the drawbacks of this implementation?
> performance?

I was trying to say that ViewHostMsg_AllocateTempFileForPrinting and
ViewHostMsg_TempFileForPrintingWritten are created for temporary testing
purpose, because I was not sure what we are going to use in the future after we
implement printing classes for Linux.
However, I've been thinking the all-page approach vs page-by-page approach here.
Now, we print all pages in the PDF file on linux while on Windows we generate
one EMF for each page. The advantage of the all-page approach is that it's
easier to do "Print to File" since we can hardly merge multiple PDF files (one
for each page) in the browser without introducing additional PDF parser. But in
some rare cases, the user might print a very long web page (let's say more than
1000 pages), then the render will be blocked in
PrintWebViewHelper::PrintPages(). Furthermore, the resulting PDF file might be
very big. In the wiki page, I mentioned that it would be good if we can have our
own PDF related APIs (maybe in skia) so that we can use the page-by-page
approach and reduce the storage we need for memory and disk space.

[Evan Stade, 2009-09-15 03:33:52]

moved resource_message_filter code to resource_message_filter_gtk

[Evan Stade, 2009-09-15 18:57:34]

sorry, somehow didn't see your comments.

http://codereview.chromium.org/203062/diff/1/10
File base/file_util.h (right):

http://codereview.chromium.org/203062/diff/1/10#newcode372
Line 372: int WriteFile(const int fd, const char* data, int size);
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> On 2009/09/14 23:18:20, Evan Stade wrote:
> > no, because it provides the implementation for WriteFile(filepath,...)
> 
> Function overloading is frown upon. Please rename.

done

> Also, why doesn't it take a FileDescriptor?

because we always want to not close it when done

http://codereview.chromium.org/203062/diff/1/5
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/203062/diff/1/5#newcode743
Line 743: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> On 2009/09/14 23:18:20, Evan Stade wrote:
> > On 2009/09/14 23:06:57, tony wrote:
> > > I guess by making this a singleton, there can be no other singleton
> > > std::map<int, FilePath>s in the code?  Maybe that's ok?
> > 
> > ah good point. It would be easy enough to make it a member variable.
> 
> No, you can "taint" the singleton with a type but it is totally static so
> putting it a member doesn't change anything.
> 
> It could be in PrintJobManager so it wouldn't need to be a singleton.

Like Min, I am skeptical that it's worth trying to use PrintJobManager for
Linux. Writing a PrintJobManagerLinux seems overkill for now since the structure
of this code is likely to change a lot.

http://codereview.chromium.org/203062/diff/1/5#newcode756
Line 756: MapFd2FilePath* g_fd2filepath = Singleton<MapFd2FilePath>::get();
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> This doesn't need to be in ResourceMessageFilter since it's not a sync
message.
> In theory all that code should be in print_job_manager.

I could be mistaken, but isn't ResourceMessageFilter for non-routed messages,
not just sync messages?

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

http://codereview.chromium.org/203062/diff/1/2#newcode92
Line 92: bool SaveTo(const base::FileDescriptor& fd) const;
On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> Function overloading is discouraged.

Done.

[M-A Ruel, 2009-09-15 20:28:39]

On 2009/09/15 18:57:34, Evan Stade wrote:
> Like Min, I am skeptical that it's worth trying to use PrintJobManager for
> Linux. Writing a PrintJobManagerLinux seems overkill for now since the
structure
> of this code is likely to change a lot.

Ok.

> On 2009/09/15 01:07:17, Marc-Antoine Ruel wrote:
> > This doesn't need to be in ResourceMessageFilter since it's not a sync
> message.
> > In theory all that code should be in print_job_manager.
> 
> I could be mistaken, but isn't ResourceMessageFilter for non-routed messages,
> not just sync messages?

Yes, but ViewHostMsg_TempFileForPrintingWritten still doesn't need to be SYNC.

http://codereview.chromium.org/203062/diff/22/1024
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/203062/diff/22/1024#newcode34
Line 34: static FdMap map;
What I meant is:
struct PrintingFileDescriptorMap {
 public:
  FdMap map;
};
Singleton<PrintingFileDescriptorMap>::get()->map

[Evan Stade, 2009-09-15 20:46:56]

Changed the types of the messages.

http://codereview.chromium.org/203062/diff/22/1024
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/203062/diff/22/1024#newcode34
Line 34: static FdMap map;
On 2009/09/15 20:28:39, Marc-Antoine Ruel wrote:
> What I meant is:
> struct PrintingFileDescriptorMap {
>  public:
>   FdMap map;
> };
> Singleton<PrintingFileDescriptorMap>::get()->map

ok. I can change it to that if you want, but is there some advantage? The lines
of code are exactly equal :)

[M-A Ruel, 2009-09-15 20:49:25]

On 2009/09/15 20:46:56, Evan Stade wrote:
> ok. I can change it to that if you want, but is there some advantage? The
lines
> of code are exactly equal :)

Better atexit() management. Also, much easier to grep. Multithread safe.
Guaranteed to have no static constructor call.

[myhuang, 2009-09-15 20:54:45]

On 2009/09/15 20:49:25, Marc-Antoine Ruel wrote:
> On 2009/09/15 20:46:56, Evan Stade wrote:
> > ok. I can change it to that if you want, but is there some advantage? The
> lines
> > of code are exactly equal :)
> 
> Better atexit() management. Also, much easier to grep. Multithread safe.
> Guaranteed to have no static constructor call.

In this case, is the Singleton<PrintingFileDescriptorMap> multithread-safe?

[myhuang, 2009-09-15 20:55:40]

On 2009/09/15 20:54:45, myhuang wrote:
> On 2009/09/15 20:49:25, Marc-Antoine Ruel wrote:
> > On 2009/09/15 20:46:56, Evan Stade wrote:
> > > ok. I can change it to that if you want, but is there some advantage? The
> > lines
> > > of code are exactly equal :)
> > 
> > Better atexit() management. Also, much easier to grep. Multithread safe.
> > Guaranteed to have no static constructor call.
> 
> In this case, is the Singleton<PrintingFileDescriptorMap> multithread-safe?

We might need a lock to guard it.

[M-A Ruel, 2009-09-15 20:57:33]

On 2009/09/15 20:55:40, myhuang wrote:
> On 2009/09/15 20:54:45, myhuang wrote:
> > In this case, is the Singleton<PrintingFileDescriptorMap> multithread-safe?
> 
> We might need a lock to guard it.

Singleton<> itself is always thread safe. Your usage is single thread so it's
fine.

[M-A Ruel, 2009-09-15 20:57:50]

and lgtm after this change and Singleton.

http://codereview.chromium.org/203062/diff/6005/6015
File base/file_util_posix.cc (right):

http://codereview.chromium.org/203062/diff/6005/6015#newcode522
Line 522: ssize_t bytes_written_total, bytes_written_partial;
It's rare that we see multiple variables defined on the same line. See style
guide at "Local Variables"

[Evan Stade, 2009-09-15 20:58:33]

On Tue, Sep 15, 2009 at 1:55 PM,  <minyu.huang@gmail.com> wrote:
> On 2009/09/15 20:54:45, myhuang wrote:
>>
>> On 2009/09/15 20:49:25, Marc-Antoine Ruel wrote:
>> > On 2009/09/15 20:46:56, Evan Stade wrote:
>> > > ok. I can change it to that if you want, but is there some advantage?
>> > > The
>> > lines
>> > > of code are exactly equal :)
>> >
>> > Better atexit() management. Also, much easier to grep. Multithread safe.
>> > Guaranteed to have no static constructor call.

Will upload new version after meeting.

>
>> In this case, is the Singleton<PrintingFileDescriptorMap>
>> multithread-safe?
>
> We might need a lock to guard it.
>
> http://codereview.chromium.org/203062
>

GetFdMap() is only called on the IO thread.

[myhuang, 2009-09-15 21:03:24]

On 2009/09/15 20:58:33, Evan Stade wrote:
> On Tue, Sep 15, 2009 at 1:55 PM,  <mailto:minyu.huang@gmail.com> wrote:
> > On 2009/09/15 20:54:45, myhuang wrote:
> >>
> >> On 2009/09/15 20:49:25, Marc-Antoine Ruel wrote:
> >> > On 2009/09/15 20:46:56, Evan Stade wrote:
> >> > > ok. I can change it to that if you want, but is there some advantage?
> >> > > The
> >> > lines
> >> > > of code are exactly equal :)
> >> >
> >> > Better atexit() management. Also, much easier to grep. Multithread safe.
> >> > Guaranteed to have no static constructor call.
> 
> Will upload new version after meeting.
> 
> >
> >> In this case, is the Singleton<PrintingFileDescriptorMap>
> >> multithread-safe?
> >
> > We might need a lock to guard it.
> >
> > http://codereview.chromium.org/203062
> >
> 
> GetFdMap() is only called on the IO thread.

Great!
Evan, Marc-Antoine, and Tony, Thank you very much for fixing/reviewing my patch!

[Evan Stade, 2009-09-15 23:30:36]

sorry, I noticed a unit test needed updating. Take a quick look at
pdf_ps_metafile_linux_unittest.cc ?

[tony, 2009-09-15 23:36:35]

pdf_ps_metafile_linux_unittest.cc LGTM

[M-A Ruel, 2009-09-15 23:42:52]

Please commit before I see more unnecessary changes.

http://codereview.chromium.org/203062/diff/7003/7013
File base/file_util_posix.cc (right):

http://codereview.chromium.org/203062/diff/7003/7013#newcode525
Line 525: for (; bytes_written_total < size;
nit: Ahhh if you had read the style guide correctly, you'd have seen that
bytes_written_partial should be defined local to the for loop.

After that, you know, that code will be mighty. :)

http://codereview.chromium.org/203062/diff/7003/7015
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/203062/diff/7003/7015#newcode33
Line 33: public:
nit: You should paste code I write, the public: was to trick you off. :)

Ok it wasn't to trick you, I just replaced class for struct at the last moment.