Make BrowserMessageFilter not derive from IPC::ChannelProxy::MessageFilter. This allows us to hide …

Make BrowserMessageFilter not derive from IPC::ChannelProxy::MessageFilter. This allows us to hide the OnMessageReceived which shouldn't be overridden from child classes, and also avoid the pattern of requiring an overridden method to have to call to the base class.

R=scherkus@chromium.org, scherkus

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

[scherkus (not reviewing), 2013-09-26 19:57:37]

I'm amazed at the amount of unnecessary superclass-calling code that was
removed!

few nits & q's

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:28: filter_->internal_ = NULL;
sanity check: this should be fine since GetFilter() should only be called during
creation?

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:76:
base::IgnoreResult(&Internal::DispatchMessage), this, message));
indent

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:95: };
DISALLOW ?

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:98: : internal_(new
Internal(this)), channel_(NULL),
I believe we'll leak internal_ if someone creates a BMF but never adds it
anywhere since the refcount will never get incremented

do we care about that case?

https://codereview.chromium.org/24514003/diff/98001/content/test/webrtc_audio...
File content/test/webrtc_audio_device_test.cc (right):

https://codereview.chromium.org/24514003/diff/98001/content/test/webrtc_audio...
content/test/webrtc_audio_device_test.cc:114: audio_mirroring_manager,
user_input_monitor),
nit: fix indent?

looks off

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h#n...
ipc/ipc_channel_proxy.h:61: : public base::RefCountedThreadSafe<MessageFilter> {
is this cleanup on the side? (e.g., there's only one MessagefilterTraits and
nobody uses OnDestruct()?)

[scherkus (not reviewing), 2013-09-26 19:58:33]

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h#n...
ipc/ipc_channel_proxy.h:61: : public base::RefCountedThreadSafe<MessageFilter> {
On 2013/09/26 19:57:37, scherkus wrote:
> is this cleanup on the side? (e.g., there's only one MessagefilterTraits and
> nobody uses OnDestruct()?)

wait ignore this ... I think the answer is BMF was the only client of the
OnDestruct() traits, right?

[jam, 2013-09-26 20:22:38]

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:28: filter_->internal_ = NULL;
On 2013/09/26 19:57:37, scherkus wrote:
> sanity check: this should be fine since GetFilter() should only be called
during
> creation?

(removed now that I changed the lifetime)

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:76:
base::IgnoreResult(&Internal::DispatchMessage), this, message));
On 2013/09/26 19:57:37, scherkus wrote:
> indent

Done.

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:95: };
On 2013/09/26 19:57:37, scherkus wrote:
> DISALLOW ?

Done.

https://codereview.chromium.org/24514003/diff/98001/content/public/browser/br...
content/public/browser/browser_message_filter.cc:98: : internal_(new
Internal(this)), channel_(NULL),
On 2013/09/26 19:57:37, scherkus wrote:
> I believe we'll leak internal_ if someone creates a BMF but never adds it
> anywhere since the refcount will never get incremented

good point, I didn't consider that.

> 
> do we care about that case?

at least one test turned out to be failing because of this:
ResolveProxyMsgHelperTest.CancelPendingRequests. but also we don't want memory
leaks in unittests. so i've switched ownership to be the other way around, like
you had in your cl. in retrospect this makes more sense, since then we don't
have to worry about the naked pointer ever being null.

https://codereview.chromium.org/24514003/diff/98001/content/test/webrtc_audio...
File content/test/webrtc_audio_device_test.cc (right):

https://codereview.chromium.org/24514003/diff/98001/content/test/webrtc_audio...
content/test/webrtc_audio_device_test.cc:114: audio_mirroring_manager,
user_input_monitor),
On 2013/09/26 19:57:37, scherkus wrote:
> nit: fix indent?
> 
> looks off

Done.

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h
File ipc/ipc_channel_proxy.h (right):

https://codereview.chromium.org/24514003/diff/98001/ipc/ipc_channel_proxy.h#n...
ipc/ipc_channel_proxy.h:61: : public base::RefCountedThreadSafe<MessageFilter> {
On 2013/09/26 19:58:34, scherkus wrote:
> On 2013/09/26 19:57:37, scherkus wrote:
> > is this cleanup on the side? (e.g., there's only one MessagefilterTraits and
> > nobody uses OnDestruct()?)
> 
> wait ignore this ... I think the answer is BMF was the only client of the
> OnDestruct() traits, right?

exactly

[michaeln, 2013-09-26 20:30:15]

https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
File content/browser/dom_storage/dom_storage_message_filter.cc (left):

https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
content/browser/dom_storage/dom_storage_message_filter.cc:49:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
is this no longer executed on the IO thread or are you just cleaning up the
dcheck for other reasons?

[jam, 2013-09-26 20:33:17]

https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
File content/browser/dom_storage/dom_storage_message_filter.cc (left):

https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
content/browser/dom_storage/dom_storage_message_filter.cc:49:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2013/09/26 20:30:16, michaeln wrote:
> is this no longer executed on the IO thread or are you just cleaning up the
> dcheck for other reasons?

these methods are all called on the IO thread. i'm just removing this while
going through the callers. the comment that i say in reviews when i see people
add these is that this is testing the behavior of the core class
(BrowserMessageFilter), and we don't need to do that in every child class.

if you feel strongly about it i can add it back

[michaeln, 2013-09-26 21:16:34]

On 2013/09/26 20:33:17, jam wrote:
>
https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
> File content/browser/dom_storage/dom_storage_message_filter.cc (left):
> 
>
https://codereview.chromium.org/24514003/diff/98001/content/browser/dom_stora...
> content/browser/dom_storage/dom_storage_message_filter.cc:49:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> On 2013/09/26 20:30:16, michaeln wrote:
> > is this no longer executed on the IO thread or are you just cleaning up the
> > dcheck for other reasons?
> 
> these methods are all called on the IO thread. i'm just removing this while
> going through the callers. the comment that i say in reviews when i see people
> add these is that this is testing the behavior of the core class
> (BrowserMessageFilter), and we don't need to do that in every child class.
> 
> if you feel strongly about it i can add it back

I like these as a form of documentation in these derived classes.

Its not always obvious to folks coming thru making changes what thread things
are running on. In the DOMStorage class i guess it doesn't really help much
since nothing in that method depends on running on the IO thread, but in the
FileAPIMessageFilter class i think its helpful documentation. Can you add them
back for the FileAPIMessageFilter class? I can't say i feel strongly about this,
but enough so to notice the changes and check.

I primarily asked to make sure the threading wasn't changing. If the dchecks had
not be removed, i wouldn't have asked :)

[scherkus (not reviewing), 2013-09-26 21:48:27]

lgtm

[scherkus (not reviewing), 2013-09-26 21:49:06]

On 2013/09/26 21:48:27, scherkus wrote:
> lgtm

(pending michaeln's concerns)

[scherkus (not reviewing), 2013-09-26 21:58:14]

wait ......... does this work as intended?

For example:

AddFilter(new OrientationMessageFilter());

OMF will have a refcount of zero and never get collected because AddFilter()
only refs OMF::internal_ ... but who is holding onto the ref to OMF itself?

[jam, 2013-09-27 20:57:23]

I ran into a number of issues switching the ownership, so I switched it to the
original code. To avoid leaks in unittests, I went through a variety of
approaches but all had problems depending on if the message loop didn't get
pumped which doesn't always happen. At the end, I realized that we can just
create Internal on demand.

I synced before uploading, but you can just diff BMF's files from ps 8 to 9

https://codereview.chromium.org/24514003/diff/192001/content/public/browser/b...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/24514003/diff/192001/content/public/browser/b...
content/public/browser/browser_message_filter.cc:193: // never attached to a
channel, we don't leak Internal;
(also added "and this" at the end, but didn't upload)

[scherkus (not reviewing), 2013-09-27 21:18:42]

lgtm

https://codereview.chromium.org/24514003/diff/192001/content/public/browser/b...
File content/public/browser/browser_message_filter.cc (right):

https://codereview.chromium.org/24514003/diff/192001/content/public/browser/b...
content/public/browser/browser_message_filter.cc:192: // We create this on
demand so that if a filter is used in a unit test but
I suppose we can still leak the filter in the DoSomething(new
MyBrowserMessageFilter()) case when the parameter type is BrowserMessageFilter*
(as opposed to scoped_refptr<T>), but that's more of an issue w/ the code and
refcounting and not really BMF's fault

https://codereview.chromium.org/24514003/diff/192001/content/public/browser/b...
content/public/browser/browser_message_filter.cc:194: DCHECK(!internal_) <<
"Should only be called once.";
it's funny that internal_'s only purpose now is to be a debug-only bool...

we could go down the path of actually using a bool, but I'm OK with this

[scherkus (not reviewing), 2013-09-27 21:19:31]

you've got some compile failures too:

../../content/browser/resolve_proxy_msg_helper_unittest.cc:30:7: error:
[chromium-style] Classes that are ref-counted should have explicit destructors
that are declared protected or private.
class TestResolveProxyMsgHelper : public ResolveProxyMsgHelper {