heads-up: message filters in the browser

[John Abd-El-Malek]

I'm splitting off all the classes in ResourceMessageFilter that filter messages, so that they're all separate message filters.  This will hopefully make RMF more maintainable, and make it easier to add new filters.  I wanted to give a heads-up to code reviewers, and a hint to future filter writers, regarding naming:

If a filter in the browser process has a corresponding object in the renderer process, then it should be called FeatureXDispatcherHost.  The object in the renderer is FeatureXDispatcher.  This is consistent with ResourceDispatcherHost/ResourceDispatcher, and a variety of other classes.

If a filter has no corresponding object in the renderer, then it's not a "Host" of anything, and should just be named FeatureXMessageFilter.

After that I will work on making filters work on threads other than UI, to save the manual work that we currently do for UI, WebKit, FILE threads etc.

[oshima]

This may break a couple of valgrind's suppressions. It'd be nice if you can run valgrind trybots before you check in,

or please let valgrind sheriff know when you check it in.

Thanks,

- oshima

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[John Abd-El-Malek]

Thanks for the heads up.

On Fri, Dec 3, 2010 at 5:56 PM, oshima <osh...@chromium.org> wrote:

This may break a couple of valgrind's suppressions. It'd be nice if you can run valgrind trybots before you check in,

or please let valgrind sheriff know when you check it in.

Thanks,

- oshima

On Fri, Dec 3, 2010 at 5:28 PM, John Abd-El-Malek <j...@chromium.org> wrote:

I'm splitting off all the classes in ResourceMessageFilter that filter messages, so that they're all separate message filters.  This will hopefully make RMF more maintainable, and make it easier to add new filters.  I wanted to give a heads-up to code reviewers, and a hint to future filter writers, regarding naming:

If a filter in the browser process has a corresponding object in the renderer process, then it should be called FeatureXDispatcherHost.  The object in the renderer is FeatureXDispatcher.  This is consistent with ResourceDispatcherHost/ResourceDispatcher, and a variety of other classes.

If a filter has no corresponding object in the renderer, then it's not a "Host" of anything, and should just be named FeatureXMessageFilter.

After that I will work on making filters work on threads other than UI,

this should of course read IO :)

[Antoine Labour]

On Fri, Dec 3, 2010 at 5:28 PM, John Abd-El-Malek <j...@chromium.org> wrote:

I'm splitting off all the classes in ResourceMessageFilter that filter messages, so that they're all separate message filters.  This will hopefully make RMF more maintainable, and make it easier to add new filters.

Thanks for doing this !

 

 I wanted to give a heads-up to code reviewers, and a hint to future filter writers, regarding naming:

If a filter in the browser process has a corresponding object in the renderer process, then it should be called FeatureXDispatcherHost.  The object in the renderer is FeatureXDispatcher.  This is consistent with ResourceDispatcherHost/ResourceDispatcher, and a variety of other classes.

If a filter has no corresponding object in the renderer, then it's not a "Host" of anything, and should just be named FeatureXMessageFilter.

After that I will work on making filters work on threads other than UI, to save the manual work that we currently do for UI, WebKit, FILE threads etc.

For that you may want to look at what I did in http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/pepper_file_message_filter.cc?revision=68069&content-type=text/plain

In that class all the thread hopping is factorized in a single place, because all the messages are meant to be handled on the FILE thread. It works pretty well, aside of the fact that messages need to be listed twice (once for the filtering itself which needs to happen on the IO thread, and once on the FILE thread to dispatch to the handler method). Feel free to suggest a better way.

Antoine

[John Abd-El-Malek]

On Sat, Dec 4, 2010 at 10:58 AM, Antoine Labour <pi...@chromium.org> wrote:

On Fri, Dec 3, 2010 at 5:28 PM, John Abd-El-Malek <j...@chromium.org> wrote:

I'm splitting off all the classes in ResourceMessageFilter that filter messages, so that they're all separate message filters.  This will hopefully make RMF more maintainable, and make it easier to add new filters.

Thanks for doing this !

 

 I wanted to give a heads-up to code reviewers, and a hint to future filter writers, regarding naming:

If a filter in the browser process has a corresponding object in the renderer process, then it should be called FeatureXDispatcherHost.  The object in the renderer is FeatureXDispatcher.  This is consistent with ResourceDispatcherHost/ResourceDispatcher, and a variety of other classes.

If a filter has no corresponding object in the renderer, then it's not a "Host" of anything, and should just be named FeatureXMessageFilter.

After that I will work on making filters work on threads other than UI, to save the manual work that we currently do for UI, WebKit, FILE threads etc.

For that you may want to look at what I did in http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/pepper_file_message_filter.cc?revision=68069&content-type=text/plain

In that class all the thread hopping is factorized in a single place, because all the messages are meant to be handled on the FILE thread. It works pretty well, aside of the fact that messages need to be listed twice (once for the filtering itself which needs to happen on the IO thread, and once on the FILE thread to dispatch to the handler method). Feel free to suggest a better way.

I actually decided to do this first, after seeing how many classes need it and figuring it'll be less work this way :)  The approach I took is similar to the above (there's no efficient way to avoid listing the messages twice), here it is with one class changed as an example: http://codereview.chromium.org/5541005/

[John Abd-El-Malek]

This is now done (pending one last code review).  If you're adding more than a few IPCs to the browser, consider putting the message be in a separate file, so don't further bloat render_messages, and having a separate IPC message filter to respond to the IPCs.  Adding new IPC files is now simpler, only one header is included and the only other file that needs to be touched is an enum in ipc_message_utils.h.  BrowserMessageFilter is the name of the new base class for filters in the browser process, and allows for dispatching messages on any thread.

I wanted to bring attention to a few incorrect patterns that I saw in a bunch of places.  Please watch out for this in code you write or review.

-using IPC_MESSAGE_HANDLER_DELAY_REPLY and then sending the reply in the handler

  -IPC_MESSAGE_HANDLER_DELAY_REPLY is meant to be used when the handler won't have the output parameters ready (i.e. because it has to wait for another operation to finish asynchronously).  If the result is available though,  IPC_MESSAGE_HANDLE should be used directly, which is much more convenient to use.

  -note that IPC_MESSAGE_HANDLER_DELAY_REPLY isn't needed even if you're dispatching on a different thread, as long as you have a Send() function that will work on whatever thread you're dispatching to

-using CallRenderViewHost and friends to call RenderViewHost::Send to send an IPC to the renderer

  -this is a big no-no, since it's equivalent to the renderer waiting on the UI thread.  In cases of sync IPCs from the renderer, this can lead to deadlock.  Since all the code that filters messages should be using BrowserMessageFilter as a base class, just call Send() directly from any thread.

On Fri, Dec 3, 2010 at 5:28 PM, John Abd-El-Malek <j...@chromium.org> wrote:

[Paweł Hajdan, Jr.]

On Sat, Dec 18, 2010 at 00:50, John Abd-El-Malek <j...@chromium.org> wrote:

-using IPC_MESSAGE_HANDLER_DELAY_REPLY and then sending the reply in the handler

  -IPC_MESSAGE_HANDLER_DELAY_REPLY is meant to be used when the handler won't have the output parameters ready (i.e. because it has to wait for another operation to finish asynchronously).  If the result is available though,  IPC_MESSAGE_HANDLE should be used directly, which is much more convenient to use.

John, I wonder if automation was one of those places. In automation, we frequently have the following pattern:

We perform some action. If the result is ready, send the reply immediately (in the handler). Otherwise, set up an observer which will send the reply message when notified.

Is the pattern above (which you an probably see in testing_automation_provider.cc and automation_provider_observers.cc) correct, or can it be improved? What do you think about it?

[John Abd-El-Malek]

If the reply is sometimes not available, then the _DELAY_REPLY is needed.  If the normal macro is used, and the dispatcher doesn't have the result ready, then there's no way for it to get the reply message at that point.