content: convert child process notifications to observer usage

content: convert child process notifications to observer usage

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

[jam, 2013-02-08 18:48:07]

https://codereview.chromium.org/12212089/diff/3001/content/public/browser/bro...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/3001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:17: class CONTENT_EXPORT
BrowserChildProcessObserver {
interfaces should be used on only one thread. the derived classes of this class
should also be created and destroyed on the UI thread. otherwise you have
locking that makes UI thread possibly wait on IO thread which is against the
async, message-passing conventions of chrome code

also, all the virtual methods below don't need to be in the cc file, just put an
empty body inline. that's the convention we use in the content api for methods
with no return types

[Paweł Hajdan Jr., 2013-02-14 15:37:42]

PTAL

I'm not entirely sure about this new observer interface - any feedback how to
make it better and cleaner is most welcome.

[jam, 2013-02-15 22:44:00]

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:19: // Called on UI
thread when a child process host has connected
get rid of all the references to UI thread in this file. by default, all content
APIs are used on the UI thread except otherwise noted

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:41:
~BrowserChildProcessObserver() {}
nit: make virtual. also get rid of the constructor.

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:54: const
ChildProcessData& data);
it's awkward to see these methods here. someone looking at this interface just
wants to see what it's used for, not how it's implemented.

first, put the static methods below in this class. just call them Add/Remove.

you can then put these methods in browser_child_process_host_impl.cc. in that
file's header you can expose whatever method that PluginProcessHost needs (I'm
guessing the NotifyProcessInstanceCreated method)

[Paweł Hajdan Jr., 2013-02-18 14:21:07]

PTAL

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:19: // Called on UI
thread when a child process host has connected
On 2013/02/15 22:44:00, jam wrote:
> get rid of all the references to UI thread in this file. by default, all
content
> APIs are used on the UI thread except otherwise noted

Done.

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:41:
~BrowserChildProcessObserver() {}
On 2013/02/15 22:44:00, jam wrote:
> nit: make virtual. also get rid of the constructor.

Virtual done. Getting rid of the ctor would result in an error like:

chrome/browser/metrics/metrics_service.cc: In constructor
‘MetricsService::MetricsService()’:
chrome/browser/metrics/metrics_service.cc:526:49: error: no matching function
for call to
‘content::BrowserChildProcessObserver::BrowserChildProcessObserver()’
chrome/browser/metrics/metrics_service.cc:526:49: note: candidate is:
./content/public/browser/browser_child_process_observer.h:52:3: note:
content::BrowserChildProcessObserver::BrowserChildProcessObserver(const
content::BrowserChildProcessObserver&)
./content/public/browser/browser_child_process_observer.h:52:3: note:  
candidate expects 1 argument, 0 provided
make: ***
[out/Debug/obj.target/browser/chrome/browser/metrics/metrics_service.o] Error 1

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:54: const
ChildProcessData& data);
On 2013/02/15 22:44:00, jam wrote:
> it's awkward to see these methods here. someone looking at this interface just
> wants to see what it's used for, not how it's implemented.
> 
> first, put the static methods below in this class. just call them Add/Remove.
> 
> you can then put these methods in browser_child_process_host_impl.cc. in that
> file's header you can expose whatever method that PluginProcessHost needs (I'm
> guessing the NotifyProcessInstanceCreated method)

Done.

[jam, 2013-02-19 18:07:09]

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:41:
~BrowserChildProcessObserver() {}
On 2013/02/18 14:21:07, Paweł Hajdan Jr. wrote:
> On 2013/02/15 22:44:00, jam wrote:
> > nit: make virtual. also get rid of the constructor.
> 
> Virtual done. Getting rid of the ctor would result in an error like:
> 
> chrome/browser/metrics/metrics_service.cc: In constructor
> ‘MetricsService::MetricsService()’:
> chrome/browser/metrics/metrics_service.cc:526:49: error: no matching function
> for call to
> ‘content::BrowserChildProcessObserver::BrowserChildProcessObserver()’
> chrome/browser/metrics/metrics_service.cc:526:49: note: candidate is:
> ./content/public/browser/browser_child_process_observer.h:52:3: note:
> content::BrowserChildProcessObserver::BrowserChildProcessObserver(const
> content::BrowserChildProcessObserver&)
> ./content/public/browser/browser_child_process_observer.h:52:3: note:  
> candidate expects 1 argument, 0 provided
> make: ***
> [out/Debug/obj.target/browser/chrome/browser/metrics/metrics_service.o] Error
1

that's because of the DISALLOW_COPY_AND_ASSIGN i believe, which we don't put on
interfaces

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
content/browser/media/webrtc_internals.h:68: // BrowserChildProcessObserver
implementation.
nit: if the methods to implement an interface can be made private, better to do
so so that the public parts of the class is smaller. also move after the
NotificationObserver method

https://codereview.chromium.org/12212089/diff/19001/content/browser/plugin_pr...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/plugin_pr...
content/browser/plugin_process_host.cc:31: #include
"content/public/browser/browser_child_process_observer.h"
nit: not needed?

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:40: void
StartObservingBrowserChildProcesses();
nit: make these be static methods (see content API wiki), and just call them
Add/Remove

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:44:
DISALLOW_COPY_AND_ASSIGN(BrowserChildProcessObserver);
nit: get rid of

[Paweł Hajdan Jr., 2013-02-20 12:11:42]

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/8001/content/public/browser/bro...
content/public/browser/browser_child_process_observer.h:41:
~BrowserChildProcessObserver() {}
On 2013/02/19 18:07:09, jam wrote:
> On 2013/02/18 14:21:07, Paweł Hajdan Jr. wrote:
> > On 2013/02/15 22:44:00, jam wrote:
> > > nit: make virtual. also get rid of the constructor.
> > 
> > Virtual done. Getting rid of the ctor would result in an error like:
> > 
> > chrome/browser/metrics/metrics_service.cc: In constructor
> > ‘MetricsService::MetricsService()’:
> > chrome/browser/metrics/metrics_service.cc:526:49: error: no matching
function
> > for call to
> > ‘content::BrowserChildProcessObserver::BrowserChildProcessObserver()’
> > chrome/browser/metrics/metrics_service.cc:526:49: note: candidate is:
> > ./content/public/browser/browser_child_process_observer.h:52:3: note:
> > content::BrowserChildProcessObserver::BrowserChildProcessObserver(const
> > content::BrowserChildProcessObserver&)
> > ./content/public/browser/browser_child_process_observer.h:52:3: note:  
> > candidate expects 1 argument, 0 provided
> > make: ***
> > [out/Debug/obj.target/browser/chrome/browser/metrics/metrics_service.o]
Error
> 1
> 
> that's because of the DISALLOW_COPY_AND_ASSIGN i believe, which we don't put
on
> interfaces

Done.

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
content/browser/media/webrtc_internals.h:68: // BrowserChildProcessObserver
implementation.
On 2013/02/19 18:07:09, jam wrote:
> nit: if the methods to implement an interface can be made private, better to
do
> so so that the public parts of the class is smaller. also move after the
> NotificationObserver method

Even though I've seen it being done in some parts of the codebase, I don't
remember it being discussed on chromium-dev, and most places don't do that.

Additionally, an "evil" caller could cast to the parent type anyway. See the
following links:
http://stackoverflow.com/questions/484592/overriding-public-virtual-functions...
http://www.parashift.com/c++-faq-lite/hiding-inherited-public.html

If you still consider the above to be blocking this CL, please raise this on
chromium-dev.

https://codereview.chromium.org/12212089/diff/19001/content/browser/plugin_pr...
File content/browser/plugin_process_host.cc (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/plugin_pr...
content/browser/plugin_process_host.cc:31: #include
"content/public/browser/browser_child_process_observer.h"
On 2013/02/19 18:07:09, jam wrote:
> nit: not needed?

Done.

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:40: void
StartObservingBrowserChildProcesses();
On 2013/02/19 18:07:09, jam wrote:
> nit: make these be static methods (see content API wiki), and just call them
> Add/Remove

Done. It wasn't obvious to me what the document was - could you explicitly link
to it? I've read http://www.chromium.org/developers/content-module/content-api
and http://www.chromium.org/developers/content-module . None of them seems to
mention static methods vs. methods like here.

https://codereview.chromium.org/12212089/diff/19001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:44:
DISALLOW_COPY_AND_ASSIGN(BrowserChildProcessObserver);
On 2013/02/19 18:07:09, jam wrote:
> nit: get rid of

Done.

[jam, 2013-02-20 19:26:30]

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
content/browser/media/webrtc_internals.h:68: // BrowserChildProcessObserver
implementation.
On 2013/02/20 12:11:42, Paweł Hajdan Jr. wrote:
> On 2013/02/19 18:07:09, jam wrote:
> > nit: if the methods to implement an interface can be made private, better to
> do
> > so so that the public parts of the class is smaller. also move after the
> > NotificationObserver method
> 
> Even though I've seen it being done in some parts of the codebase, I don't
> remember it being discussed on chromium-dev, and most places don't do that.
> 
> Additionally, an "evil" caller could cast to the parent type anyway. See the
> following links:
>
http://stackoverflow.com/questions/484592/overriding-public-virtual-functions...
> http://www.parashift.com/c++-faq-lite/hiding-inherited-public.html
> 
> If you still consider the above to be blocking this CL, please raise this on
> chromium-dev.

I realize the methods are still reachable through casting. But it makes it more
obvious to developers/reviewers that something weird is happening. These methods
are really implementation details of this class, and as such there's no reason
to expose them in the public section of the class. I consider this good hygiene
(exposing only what's needed).

[tfarina, 2013-02-26 19:47:24]

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:8: #include
"base/basictypes.h"
can you remove this now?

[Paweł Hajdan Jr., 2013-02-27 19:13:23]

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
File content/browser/media/webrtc_internals.h (right):

https://codereview.chromium.org/12212089/diff/19001/content/browser/media/web...
content/browser/media/webrtc_internals.h:68: // BrowserChildProcessObserver
implementation.
On 2013/02/20 19:26:31, jam wrote:
> I realize the methods are still reachable through casting. But it makes it
more
> obvious to developers/reviewers that something weird is happening. These
methods
> are really implementation details of this class, and as such there's no reason
> to expose them in the public section of the class. I consider this good
hygiene
> (exposing only what's needed).

Alright, I've moved things to private when consistent with existing code. I've
tried to make the order of declarations also match the order of inheritance.

Please let me know if there are any remaining issues. For trivial ones, ideally
please give a positive review conditional on fixing these issues.

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/02/26 19:47:25, tfarina wrote:
> no (c)

Done.

Note that I see little point for drive-bys just for this. Just convert
everything, modify the presubmit check and get it done all at once.

People will get pretty annoyed if you do drive-bys for something that can and
should be done once and mechanically.

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:8: #include
"base/basictypes.h"
On 2013/02/26 19:47:25, tfarina wrote:
> can you remove this now?

Done.

[tfarina, 2013-02-27 19:17:13]

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
File content/public/browser/browser_child_process_observer.h (right):

https://codereview.chromium.org/12212089/diff/23001/content/public/browser/br...
content/public/browser/browser_child_process_observer.h:1: // Copyright (c) 2013
The Chromium Authors. All rights reserved.
On 2013/02/27 19:13:23, Paweł Hajdan Jr. wrote:
> On 2013/02/26 19:47:25, tfarina wrote:
> > no (c)
> 
> Done.
> 
> Note that I see little point for drive-bys just for this.
If no one points, people will keep using it.

> Just convert
> everything, modify the presubmit check and get it done all at once.
>
As you saw in chromium-dev that isn't going to happen, at least I won't do it
myself.

sorry.
 
> People will get pretty annoyed if you do drive-bys for something that can and
> should be done once and mechanically.
probably...but that isn't going to happen I guess.

[jam, 2013-02-28 21:43:26]

lgtm

[Paweł Hajdan Jr., 2013-02-28 22:00:45]

Committed patchset #7 manually as r185337 (presubmit successful).