Modify BrowserMessageFilter::OverrideThreadForMessage to allow the subclasses to specificy either a…

Modify BrowserMessageFilter to allow subclasses to specificy either a BrowserThread::ID or a TaskRunner when overriding the target thread for message dispatching.

BUG=114474
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126292

[michaeln, 2012-03-09 20:46:46]

hi john, please take a look...

I'm working on a would be consumer of this interface.

I haven't updated the 10'ish overrides of this method yet because i want to run
the BrowserMessageFilter interface changes by you first.

This is a half-measure to fully resolving 114474 since only a sequence_token can
be returned. The special-case handing for the destination being the UI thread in
BrowserMessageFilter's OnMessageReceived() method got in the way of just
returning a TaskRunner.

[michaeln, 2012-03-09 23:15:31]

I've updated the overrides now, so i'm commensurately more attached to this
interface change now :)

> I haven't updated the 10'ish overrides of this method yet because i want to
run
> the BrowserMessageFilter interface changes by you first.

[jam, 2012-03-10 00:19:08]

http://codereview.chromium.org/9665014/diff/7001/content/public/browser/brows...
File content/public/browser/browser_message_filter.h (right):

http://codereview.chromium.org/9665014/diff/7001/content/public/browser/brows...
content/public/browser/browser_message_filter.h:44: virtual void
OverrideThreadForMessage(
why not have another method "OverrideWorkerPoolForMessage"? that way you don't
need to change existing code, and it becomes easier to understand. i.e. override
the thread version if you want to pass a BrowserThread::ID, or the worker pool
one if you have a sequence worker pool.

[michaeln, 2012-03-10 00:53:33]

https://chromiumcodereview.appspot.com/9665014/diff/7001/content/public/brows...
File content/public/browser/browser_message_filter.h (right):

https://chromiumcodereview.appspot.com/9665014/diff/7001/content/public/brows...
content/public/browser/browser_message_filter.h:44: virtual void
OverrideThreadForMessage(
That would be fine by me too. What would you think of...

TaskRunner* OverrideTaskRunnerForMessage(...);

With the caveat that the 'runner' should not target the UI thread.

Providing the 'runner' is more valuable than a sequence_id since the runner
could post unsequenced tasks or tasks with different shutdown semantics if the
provider desired. Only thing is that it bugs me that there's no good way to
assert that the runner isn't tied to the UI thread (which requires the
special-case stuff).

> why not have another method "OverrideWorkerPoolForMessage"? that way you don't
> need to change existing code, and it becomes easier to understand. i.e.
override
> the thread version if you want to pass a BrowserThread::ID, or the worker pool
> one if you have a sequence worker pool.

[jam, 2012-03-10 01:10:12]

https://chromiumcodereview.appspot.com/9665014/diff/7001/content/public/brows...
File content/public/browser/browser_message_filter.h (right):

https://chromiumcodereview.appspot.com/9665014/diff/7001/content/public/brows...
content/public/browser/browser_message_filter.h:44: virtual void
OverrideThreadForMessage(
On 2012/03/10 00:53:33, michaeln wrote:
> That would be fine by me too. What would you think of...
> 
> TaskRunner* OverrideTaskRunnerForMessage(...);
> 
> With the caveat that the 'runner' should not target the UI thread.
> 
> Providing the 'runner' is more valuable than a sequence_id since the runner
> could post unsequenced tasks or tasks with different shutdown semantics if the
> provider desired.

Yep, agreed that TaskRunner would be better.

> Only thing is that it bugs me that there's no good way to
> assert that the runner isn't tied to the UI thread (which requires the
> special-case stuff).

yeah agreed that this is unfortunate, but i wouldn't let it block you. i wonder
if it would be possible to have some debug checks for this, i.e. maybe in debug
mode keep a map of all taskrunners for UI thread?
> 
> > why not have another method "OverrideWorkerPoolForMessage"? that way you
don't
> > need to change existing code, and it becomes easier to understand. i.e.
> override
> > the thread version if you want to pass a BrowserThread::ID, or the worker
pool
> > one if you have a sequence worker pool.
>

[michaeln, 2012-03-10 01:46:33]

thnx i'll fixup the CL and ping you again next week, and look into some runtime
checks in dbg builds (maybe)

[michaeln, 2012-03-12 19:05:12]

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
File content/public/browser/browser_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
content/public/browser/browser_message_filter.cc:86: if (runner) {
I don't see a reasonable way to compose a debug check that the 'runner' will not
post to the UI thread.

Keeping a map of all instances for that test is too expensive and ugly. RTTI to
determine if the runner ISA BrowserThreadMessageLoopProxy and then a further
check to determine if it's 'id_' data member is UI would be closer to an
appropriate amount of effort to put into performing this check, but we don't
have RTTI and that class is hidden in a .cc file.

So, I'm punting on that runtime check.

[jam, 2012-03-12 21:54:05]

lgtm

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
File content/public/browser/browser_message_filter.cc (right):

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
content/public/browser/browser_message_filter.cc:69: void
BrowserMessageFilter::OverrideThreadForMessage(
nit: no need to change

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
File content/public/browser/browser_message_filter.h (right):

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
content/public/browser/browser_message_filter.h:51: content::BrowserThread::ID*
thread);
nit: spacing

also while you're at it, remove the content:: since it's redundant

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
content/public/browser/browser_message_filter.h:55: // Note: To target the UI
thread, please use OverrideThreadForMessage.
nit: add "since that has extra checks to avoid deadlocks."

http://codereview.chromium.org/9665014/diff/3005/content/public/browser/brows...
content/public/browser/browser_message_filter.h:61: // OverrideThreadForMessage
modifies the thread used to dispatch the message,
nit: OverrideXForMessage

[michaeln, 2012-03-12 23:18:10]

thnx

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
File content/public/browser/browser_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
content/public/browser/browser_message_filter.cc:69: void
BrowserMessageFilter::OverrideThreadForMessage(
On 2012/03/12 21:54:05, John Abd-El-Malek wrote:
> nit: no need to change

Done.

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
File content/public/browser/browser_message_filter.h (right):

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
content/public/browser/browser_message_filter.h:51: content::BrowserThread::ID*
thread);
On 2012/03/12 21:54:05, John Abd-El-Malek wrote:
> nit: spacing
> 
> also while you're at it, remove the content:: since it's redundant

Done.

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
content/public/browser/browser_message_filter.h:55: // Note: To target the UI
thread, please use OverrideThreadForMessage.
On 2012/03/12 21:54:05, John Abd-El-Malek wrote:
> nit: add "since that has extra checks to avoid deadlocks."

Done.

https://chromiumcodereview.appspot.com/9665014/diff/3005/content/public/brows...
content/public/browser/browser_message_filter.h:61: // OverrideThreadForMessage
modifies the thread used to dispatch the message,
On 2012/03/12 21:54:05, John Abd-El-Malek wrote:
> nit: OverrideXForMessage

Done.