Created a new BrowserMessageFilter to handle print related IPC messages.

Created a new BrowserMessageFilter to handle print related IPC messages.

Move print related IPC message handlers from render_message_filter to printing_message_filter.

BUG=none
TEST=printing works after code changes.

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

[kmadhusu, 2011-02-21 21:30:04]

Please review and let me know your comments.

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:91: #include
"content/browser/renderer_host/printing_message_filter.h"
jam: Do I need a dummy file
"chrome/browser/renderer_host/printing_message_filter.h" to include
"content/browser/renderer_host/printing_message_filter.h" or is this ok?

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:86:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_AllocateTempFileForPrinting,
jam: Although we are dispatching on a different thread, the handler has a Send()
function. Can I change this to "IPC_MESSAGE_HANDLER"?

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.h (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:51: void
DoOnAllocateTempFileForPrinting(IPC::Message* reply_msg);
Previously, the following functions are declared under the preprocessor USE_X11
but defined under the preprocessor OS_CHROMEOS. Therefore, I declared the
function under "OS_CHROMEOS"
 
  void DoOnAllocateTempFileForPrinting(IPC::Message* reply_msg);
  void DoOnTempFileForPrintingWritten(int sequence_number);

[Lei Zhang, 2011-02-22 19:37:42]

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:91: #include
"content/browser/renderer_host/printing_message_filter.h"
On 2011/02/21 21:30:04, kmadhusu wrote:
> jam: Do I need a dummy file
> "chrome/browser/renderer_host/printing_message_filter.h" to include
> "content/browser/renderer_host/printing_message_filter.h" or is this ok?

This should be fine. Nobody is referring to the file at the old location since
this is a new file.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:29: struct
PrintingSequencePathMap {
this and |g_printing_file_descriptor_map| are CrOS only.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.h (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:17: struct
ViewHostMsg_ScriptedPrint_Params;
nit: this forward decl. should be above the forward decl. in namespaces.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:51: void
DoOnAllocateTempFileForPrinting(IPC::Message* reply_msg);
On 2011/02/21 21:30:04, kmadhusu wrote:
> Previously, the following functions are declared under the preprocessor
USE_X11
> but defined under the preprocessor OS_CHROMEOS. Therefore, I declared the
> function under "OS_CHROMEOS"
>  
>   void DoOnAllocateTempFileForPrinting(IPC::Message* reply_msg);
>   void DoOnTempFileForPrintingWritten(int sequence_number);

SGTM

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter.cc:13: #include
"base/shared_memory.h"
remove.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter_gtk.cc (left):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter_gtk.cc:12: #include
"base/path_service.h"
remove

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter_gtk.cc:14: #include
"chrome/common/chrome_paths.h"
remove

[jam, 2011-02-22 20:29:40]

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:91: #include
"content/browser/renderer_host/printing_message_filter.h"
On 2011/02/22 19:37:42, Lei Zhang wrote:
> On 2011/02/21 21:30:04, kmadhusu wrote:
> > jam: Do I need a dummy file
> > "chrome/browser/renderer_host/printing_message_filter.h" to include
> > "content/browser/renderer_host/printing_message_filter.h" or is this ok?
> 
> This should be fine. Nobody is referring to the file at the old location since
> this is a new file.

actually, the printing code will remain in src\chrome, and not src\content.  The
latter is the minimum required to render a page in a multi-process browser. 
Features like printing, autofill etc are what's provided by the embedder.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:19: #include
"ipc/ipc_message_macros.h"
i don't think this is needed

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:29: struct
PrintingSequencePathMap {
On 2011/02/22 19:37:42, Lei Zhang wrote:
> this and |g_printing_file_descriptor_map| are CrOS only.

also fcntl.h should only be included on gtk builds?  not sure if that's
controlled by OS_LINUX?

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.h (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
all these files should be in chrome\browser\printing

[jam, 2011-02-22 23:39:51]

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:86:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_AllocateTempFileForPrinting,
On 2011/02/21 21:30:04, kmadhusu wrote:
> jam: Although we are dispatching on a different thread, the handler has a
Send()
> function. Can I change this to "IPC_MESSAGE_HANDLER"? 

I'm not quite sure what this question is.  But actually, you don't to have
manually dispatch the message to the file thread.  You can override
BrowserMessageFilter::OverrideThreadForMessage and return FILE for
ViewHostMsg_AllocateTempFileForPrinting and
ViewHostMsg_TempFileForPrintingWritten.  Then you can use IPC_MESSAGE_HANDLER
for them, and the dispatcher functions will be run on the FILE thread.

[kmadhusu, 2011-02-23 02:27:49]

Thanks for the comments. Made code changes accordingly. Please review.

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6546051/diff/1/chrome/browser/renderer_host/br...
chrome/browser/renderer_host/browser_render_process_host.cc:91: #include
"content/browser/renderer_host/printing_message_filter.h"
On 2011/02/22 20:29:41, John Abd-El-Malek wrote:
> On 2011/02/22 19:37:42, Lei Zhang wrote:
> > On 2011/02/21 21:30:04, kmadhusu wrote:
> > > jam: Do I need a dummy file
> > > "chrome/browser/renderer_host/printing_message_filter.h" to include
> > > "content/browser/renderer_host/printing_message_filter.h" or is this ok?
> > 
> > This should be fine. Nobody is referring to the file at the old location
since
> > this is a new file.
> 
> actually, the printing code will remain in src\chrome, and not src\content. 
The
> latter is the minimum required to render a page in a multi-process browser. 
> Features like printing, autofill etc are what's provided by the embedder.

Done. Moved printing_message_filter from src/content to
src/chrome/browser/printing.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:19: #include
"ipc/ipc_message_macros.h"
On 2011/02/22 20:29:41, John Abd-El-Malek wrote:
> i don't think this is needed

Removed.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:29: struct
PrintingSequencePathMap {
On 2011/02/22 19:37:42, Lei Zhang wrote:
> this and |g_printing_file_descriptor_map| are CrOS only.

Done.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:29: struct
PrintingSequencePathMap {
On 2011/02/22 20:29:41, John Abd-El-Malek wrote:
> On 2011/02/22 19:37:42, Lei Zhang wrote:
> > this and |g_printing_file_descriptor_map| are CrOS only.
> 
> also fcntl.h should only be included on gtk builds?  not sure if that's
> controlled by OS_LINUX? 

Fixed. It is required only for CrOS builds.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.cc:86:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_AllocateTempFileForPrinting,
On 2011/02/22 23:39:51, John Abd-El-Malek wrote:
> On 2011/02/21 21:30:04, kmadhusu wrote:
> > jam: Although we are dispatching on a different thread, the handler has a
> Send()
> > function. Can I change this to "IPC_MESSAGE_HANDLER"? 
> 
> I'm not quite sure what this question is.  But actually, you don't to have
> manually dispatch the message to the file thread.  You can override
> BrowserMessageFilter::OverrideThreadForMessage and return FILE for
> ViewHostMsg_AllocateTempFileForPrinting and
> ViewHostMsg_TempFileForPrintingWritten.  Then you can use IPC_MESSAGE_HANDLER
> for them, and the dispatcher functions will be run on the FILE thread.

Fixed. Implemented OverrideThreadForMessage() function to return FILE for
ViewHostMsg_AllocateTempFileForPrinting and
ViewHostMsg_TempFileForPrintingWritten. 

Used IPC_MESSAGE_HANDLER for "ViewHostMsg_AllocateTempFileForPrinting".

Modified OnAllocateTempFileForPrinting() and OnTempFileForPrintingWritten()
accordingly.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
File content/browser/renderer_host/printing_message_filter.h (right):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:1: // Copyright (c) 2011
The Chromium Authors. All rights reserved.
On 2011/02/22 20:29:41, John Abd-El-Malek wrote:
> all these files should be in chrome\browser\printing

Done.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/p...
content/browser/renderer_host/printing_message_filter.h:17: struct
ViewHostMsg_ScriptedPrint_Params;
On 2011/02/22 19:37:42, Lei Zhang wrote:
> nit: this forward decl. should be above the forward decl. in namespaces.

Done.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter.cc:13: #include
"base/shared_memory.h"
On 2011/02/22 19:37:42, Lei Zhang wrote:
> remove.

Done.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_message_filter_gtk.cc (left):

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter_gtk.cc:12: #include
"base/path_service.h"
On 2011/02/22 19:37:42, Lei Zhang wrote:
> remove

Done.

http://codereview.chromium.org/6546051/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_message_filter_gtk.cc:14: #include
"chrome/common/chrome_paths.h"
On 2011/02/22 19:37:42, Lei Zhang wrote:
> remove

Done.

[Lei Zhang, 2011-02-23 06:18:50]

LGTM with nits below.

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:7: #include
"base/command_line.h"
!cros only

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:8: #include
"base/file_util.h"
cros only.

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:9: #include
"base/lazy_instance.h"
cros only

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:14: #include
"chrome/common/chrome_switches.h"
!cros only

http://codereview.chromium.org/6546051/diff/12001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6546051/diff/12001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1779: 'browser/printing/printing_message_filter.cc',
nit: alphabetical order - this goes after printer_query.*

[kmadhusu, 2011-02-23 17:31:53]

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:7: #include
"base/command_line.h"
On 2011/02/23 06:18:50, Lei Zhang wrote:
> !cros only

Done.

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:8: #include
"base/file_util.h"
On 2011/02/23 06:18:50, Lei Zhang wrote:
> cros only.

Done.

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:9: #include
"base/lazy_instance.h"
On 2011/02/23 06:18:50, Lei Zhang wrote:
> cros only

Done.

http://codereview.chromium.org/6546051/diff/12001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:14: #include
"chrome/common/chrome_switches.h"
On 2011/02/23 06:18:50, Lei Zhang wrote:
> !cros only

Done.

http://codereview.chromium.org/6546051/diff/12001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/6546051/diff/12001/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:1779: 'browser/printing/printing_message_filter.cc',
On 2011/02/23 06:18:50, Lei Zhang wrote:
> nit: alphabetical order - this goes after printer_query.*

Done.

[jam, 2011-02-23 17:58:35]

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:42: DCHECK(params);
nit: the usual style is to avoid DCHECKs for parameters being not null.  in
release, it's compiled away anyways and we'll get a crash dump.  in debug, the
next line would crash and the developer would get the same information.  so they
don't add much value.

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:74:
PrintingMessageFilter::~PrintingMessageFilter() {}
nit: closing brace bracket on next line

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:81:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
this DCHECK isn't needed, i.e. it's testing that BrowserMessageFilter is working
correctly.  that should be added in a test for BrowserMessageFilter, but we
don't need every base derived to test this.

http://codereview.chromium.org/6546051/diff/19001/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/6546051/diff/19001/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:450:
IPC_MESSAGE_HANDLER(ViewHostMsg_AllocateSharedMemoryBuffer,
this message is both sent by the printing code, and also the clipboard code.
i.e. see ScopedClipboardWriterGlue::WriteBitmapFromPixels in renderer_glue.cc. 
As such, it shouldn't go into the printing message filter.  the reason is we'll
keep the printing message filter in src\chrome, but the basic IPC messages that
are used for the core multi-process browser will move to src\content.

[kmadhusu, 2011-02-23 19:36:42]

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
File chrome/browser/printing/printing_message_filter.cc (right):

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:42: DCHECK(params);
On 2011/02/23 17:58:35, John Abd-El-Malek wrote:
> nit: the usual style is to avoid DCHECKs for parameters being not null.  in
> release, it's compiled away anyways and we'll get a crash dump.  in debug, the
> next line would crash and the developer would get the same information.  so
they
> don't add much value.

Fixed.

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:74:
PrintingMessageFilter::~PrintingMessageFilter() {}
On 2011/02/23 17:58:35, John Abd-El-Malek wrote:
> nit: closing brace bracket on next line

Done.

http://codereview.chromium.org/6546051/diff/19001/chrome/browser/printing/pri...
chrome/browser/printing/printing_message_filter.cc:81:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2011/02/23 17:58:35, John Abd-El-Malek wrote:
> this DCHECK isn't needed, i.e. it's testing that BrowserMessageFilter is
working
> correctly.  that should be added in a test for BrowserMessageFilter, but we
> don't need every base derived to test this.

Done.

http://codereview.chromium.org/6546051/diff/19001/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/6546051/diff/19001/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:450:
IPC_MESSAGE_HANDLER(ViewHostMsg_AllocateSharedMemoryBuffer,
On 2011/02/23 17:58:35, John Abd-El-Malek wrote:
> this message is both sent by the printing code, and also the clipboard code.
> i.e. see ScopedClipboardWriterGlue::WriteBitmapFromPixels in renderer_glue.cc.

> As such, it shouldn't go into the printing message filter.  the reason is
we'll
> keep the printing message filter in src\chrome, but the basic IPC messages
that
> are used for the core multi-process browser will move to src\content.

Added back to render_message_filter.cc

[jam, 2011-02-24 18:38:07]

lgtm

[Lei Zhang, 2011-02-24 20:07:28]

LGTM