Refactor notification provider in renderer process to not use a message filte...

Refactor notification provider in renderer process to not use a message filter.

BUG=24241
TEST=not crashing


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

[jabdelmalek, 2009-10-14 21:48:10]

http://codereview.chromium.org/276032/diff/2005/2008
File chrome/renderer/notification_provider.cc (right):

http://codereview.chromium.org/276032/diff/2005/2008#newcode80
Line 80: 
nit: extra line

http://codereview.chromium.org/276032/diff/2005/2008#newcode82
Line 82: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
Have you tested this on Windows or Linux?  Because all platforms except Mac have
TYPE_DEFAULT as the message loop type for the renderer.

http://codereview.chromium.org/276032/diff/2005/2009
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/276032/diff/2005/2009#newcode497
Line 497: OnNotificationPermissionRequestComplete);
If you have a separate class for handling these messages, it's cleaner to have
it dispatch the messages itself.  That way you avoid having two message handlers
for each message.  There are examples above for devtools_client_ and
devtools_agent_.

[John Gregg, 2009-10-14 22:08:01]

New code posted.

http://codereview.chromium.org/276032/diff/2005/2008
File chrome/renderer/notification_provider.cc (right):

http://codereview.chromium.org/276032/diff/2005/2008#newcode80
Line 80: 
On 2009/10/14 21:48:10, jabdelmalek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/276032/diff/2005/2008#newcode82
Line 82: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
Really?  I'll change it to != TYPE_IO.  [And I will have unit & layout tests for
this as soon as the browser side is checked in]

http://codereview.chromium.org/276032/diff/2005/2009
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/276032/diff/2005/2009#newcode497
Line 497: OnNotificationPermissionRequestComplete);
On 2009/10/14 21:48:10, jabdelmalek wrote:
> If you have a separate class for handling these messages, it's cleaner to have
> it dispatch the messages itself.  That way you avoid having two message
handlers
> for each message.  There are examples above for devtools_client_ and
> devtools_agent_.

Thanks, I thought this was what you meant by "manually", but of course what you
suggest is much cleaner.

[jam, 2009-10-14 22:15:47]

http://codereview.chromium.org/276032/diff/2005/2008
File chrome/renderer/notification_provider.cc (right):

http://codereview.chromium.org/276032/diff/2005/2008#newcode82
Line 82: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
On 2009/10/14 22:08:01, John Gregg wrote:
> Really?  I'll change it to != TYPE_IO.  [And I will have unit & layout tests
for
> this as soon as the browser side is checked in]

I think these DCHECKs are gratuitous, i.e. we don't check on each message
handler in all of our classes that we're on the right thread.  We depend on
IPC::ChannelProxy to do the right thing.

http://codereview.chromium.org/276032/diff/1006/2014
File chrome/renderer/notification_provider.h (right):

http://codereview.chromium.org/276032/diff/1006/2014#newcode36
Line 36: // IPC handlers.
nit: these can be private

http://codereview.chromium.org/276032/diff/1006/2017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/276032/diff/1006/2017#newcode390
Line 390: if (notification_provider_.get() &&
why not just create notification_provider_ in the constructor, and that way this
check isn't needed?

[John Gregg, 2009-10-14 22:48:31]

code revised again.

http://codereview.chromium.org/276032/diff/2005/2008
File chrome/renderer/notification_provider.cc (right):

http://codereview.chromium.org/276032/diff/2005/2008#newcode82
Line 82: DCHECK(MessageLoop::current()->type() == MessageLoop::TYPE_UI);
Okay, they're gone.

http://codereview.chromium.org/276032/diff/1006/2014
File chrome/renderer/notification_provider.h (right):

http://codereview.chromium.org/276032/diff/1006/2014#newcode36
Line 36: // IPC handlers.
On 2009/10/14 22:15:47, John Abd-El-Malek wrote:
> nit: these can be private

Done.

http://codereview.chromium.org/276032/diff/1006/2017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/276032/diff/1006/2017#newcode390
Line 390: if (notification_provider_.get() &&
I prefer that too, so I'll change it.  But devtools_agent is initialized and
checked in exactly the same way.

[jam, 2009-10-14 23:00:23]

lgtm, thanks for the quick turnaround.

http://codereview.chromium.org/276032/diff/1006/2017
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/276032/diff/1006/2017#newcode390
Line 390: if (notification_provider_.get() &&
On 2009/10/14 22:48:31, John Gregg wrote:
> I prefer that too, so I'll change it.  But devtools_agent is initialized and
> checked in exactly the same way.

Actually, looking at devtools_agent doesn't need a check either, since AddRoute
is called in Init(), so there's no chance of a message coming before it's
created.  Up to you where you want to create notification_provider_.