Refactor RenderProcessHost to use IPC::Listener instead of RenderWidgetHost

Refactor RenderProcessHost to use IPC::Listener instead of RenderWidgetHost

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

[dcheng, 2013-06-11 00:34:58]

Mostly a drive by. Generic comment about removing out commented-out code as
well.

https://codereview.chromium.org/16431010/diff/3001/content/browser/devtools/r...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/16431010/diff/3001/content/browser/devtools/r...
content/browser/devtools/render_view_devtools_agent_host.cc:154: /*
Nit: Delete this block.

https://codereview.chromium.org/16431010/diff/3001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:93: return (process_id_
<< sizeof(int32)) | routing_id_;
Is it intentional that you're only left shifting by 4?

https://codereview.chromium.org/16431010/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:101: int32 process_id_;
Nit: No _ when naming structs.
Also, I might personally just use a union here but that's just preference (I
don't know what convention is).

https://codereview.chromium.org/16431010/diff/3001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:301:
RenderWidgetHost::GetRenderWidgetHosts() {
From what I can see, it's fine to just use return by value here (or pass a
pointer to the output hash_map). Note that a lot of code in Chrome already
depends on RVO though; for example,
render_view_devtools_agent_host.cc:DevToolsAgentHost::GetValidRenderViewHosts,
which your patch touches, depends on RVO.

Note another advantage to using RVO is you could make the returned value const
if you want to insure that it is not accidentally modified, which you can't do
if you pass it in by pointer.

https://codereview.chromium.org/16431010/diff/3001/content/public/browser/ren...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/16431010/diff/3001/content/public/browser/ren...
content/public/browser/render_widget_host.h:42: typedef
std::vector<RenderWidgetHost*> RenderWidgetHostList;
I think this typedef is unused.

[nasko, 2013-06-12 13:45:37]

Hey John,
Can you review this CL? It is the refactor we discussed in email to allow for
non-RWH IPC listeners to register in the browser process.
Thanks,
Nasko

[jam, 2013-06-12 19:59:59]

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/timezone_settings.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/timezone_settings.cc:357: for
(content::RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/processes/processes_api.cc:117: for
(content::RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:219: it != widgets.end(); ++it) {
ditto

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/performanc...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/performanc...
chrome/browser/performance_monitor/performance_monitor.cc:630: it !=
widgets.end(); ++it) {
ditto

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/renderer_h...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:40: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/task_manag...
File chrome/browser/task_manager/guest_resource_provider.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/task_manag...
chrome/browser/task_manager/guest_resource_provider.cc:136: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/accessibility_ui.cc:99: it != widgets.end(); ++it)
{
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_state_impl.cc:133: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_state_impl.cc:143:
RenderWidgetHostImpl::From(const_cast<RenderWidgetHost*>(widget));
you'd get rid of the const cast after switching to size_t iterating

https://codereview.chromium.org/16431010/diff/24001/content/browser/devtools/...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/devtools/...
content/browser/devtools/render_view_devtools_agent_host.cc:125: it !=
widgets.end(); ++it) {
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:879:
rwh = NULL;
nit: skip the else, it's not needed since rwh is initialized to NULL

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1619: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
dito

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1642: it !=
widgets.end(); ++it) {
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1795: it !=
widgets.end(); ++it) {
ditto

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:492: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
ditto re size_t

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:21: #include
"cc/base/hash_pair.h"
it seems wrong that content uses cc's base (or that cc has its own base, now
that it's in the same repo).

if this is base-type code that is used by multiple modules, it should be in
src/base

https://codereview.chromium.org/16431010/diff/24001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:114: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
since this is a vector,

for (size_t i = 0; i < widgets.size(); ++i)

is a more concise and readable

https://codereview.chromium.org/16431010/diff/24001/content/public/test/mock_...
File content/public/test/mock_render_process_host.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/public/test/mock_...
content/public/test/mock_render_process_host.cc:218: if (!widget)
when can this happen?

[nasko, 2013-06-12 21:18:59]

Fixes for all the issues raised.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/chromeos/s...
File chrome/browser/chromeos/system/timezone_settings.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/chromeos/s...
chrome/browser/chromeos/system/timezone_settings.cc:357: for
(content::RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/extensions...
File chrome/browser/extensions/api/processes/processes_api.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/extensions...
chrome/browser/extensions/api/processes/processes_api.cc:117: for
(content::RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/memory_det...
File chrome/browser/memory_details.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/memory_det...
chrome/browser/memory_details.cc:219: it != widgets.end(); ++it) {
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/performanc...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/performanc...
chrome/browser/performance_monitor/performance_monitor.cc:630: it !=
widgets.end(); ++it) {
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/renderer_h...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:40: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/task_manag...
File chrome/browser/task_manager/guest_resource_provider.cc (right):

https://codereview.chromium.org/16431010/diff/24001/chrome/browser/task_manag...
chrome/browser/task_manager/guest_resource_provider.cc:136: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
File content/browser/accessibility/accessibility_ui.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/accessibility_ui.cc:99: it != widgets.end(); ++it)
{
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
File content/browser/accessibility/browser_accessibility_state_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_state_impl.cc:133: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/accessibi...
content/browser/accessibility/browser_accessibility_state_impl.cc:143:
RenderWidgetHostImpl::From(const_cast<RenderWidgetHost*>(widget));
On 2013/06/12 19:59:59, jam wrote:
> you'd get rid of the const cast after switching to size_t iterating

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/devtools/...
File content/browser/devtools/render_view_devtools_agent_host.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/devtools/...
content/browser/devtools/render_view_devtools_agent_host.cc:125: it !=
widgets.end(); ++it) {
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/media/web_contents_video_capture_device.cc
(right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/media/web_contents_video_capture_device.cc:879:
rwh = NULL;
On 2013/06/12 19:59:59, jam wrote:
> nit: skip the else, it's not needed since rwh is initialized to NULL

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1619: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> dito

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1642: it !=
widgets.end(); ++it) {
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_process_host_impl.cc:1795: it !=
widgets.end(); ++it) {
On 2013/06/12 19:59:59, jam wrote:
> ditto

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:492: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> ditto re size_t

Done.

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:21: #include
"cc/base/hash_pair.h"
On 2013/06/12 19:59:59, jam wrote:
> it seems wrong that content uses cc's base (or that cc has its own base, now
> that it's in the same repo).
> 
> if this is base-type code that is used by multiple modules, it should be in
> src/base

Agreed. ajwong and danakj have taken on moving this code in base. I don't
believe this CL should be blocked on that move. Let me know if you disagree.

https://codereview.chromium.org/16431010/diff/24001/content/browser/worker_ho...
File content/browser/worker_host/worker_service_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/worker_ho...
content/browser/worker_host/worker_service_impl.cc:114: for
(RenderWidgetHost::List::const_iterator it = widgets.begin();
On 2013/06/12 19:59:59, jam wrote:
> since this is a vector,
> 
> for (size_t i = 0; i < widgets.size(); ++i)
> 
> is a more concise and readable

Done.

https://codereview.chromium.org/16431010/diff/24001/content/public/test/mock_...
File content/public/test/mock_render_process_host.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/public/test/mock_...
content/public/test/mock_render_process_host.cc:218: if (!widget)
On 2013/06/12 19:59:59, jam wrote:
> when can this happen?

Missed that, fixed now.

[jam, 2013-06-12 22:59:27]

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:21: #include
"cc/base/hash_pair.h"
On 2013/06/12 21:18:59, nasko wrote:
> On 2013/06/12 19:59:59, jam wrote:
> > it seems wrong that content uses cc's base (or that cc has its own base, now
> > that it's in the same repo).
> > 
> > if this is base-type code that is used by multiple modules, it should be in
> > src/base
> 
> Agreed. ajwong and danakj have taken on moving this code in base. I don't
> believe this CL should be blocked on that move. Let me know if you disagree.

Yes, please let's wait on that.

[nasko, 2013-06-20 23:25:26]

jam@, can you take a look? The move of the pair hashing code is now complete and
I've rebased on top of it.
Thanks!

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/16431010/diff/24001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_impl.cc:21: #include
"cc/base/hash_pair.h"
On 2013/06/12 22:59:27, jam wrote:
> On 2013/06/12 21:18:59, nasko wrote:
> > On 2013/06/12 19:59:59, jam wrote:
> > > it seems wrong that content uses cc's base (or that cc has its own base,
now
> > > that it's in the same repo).
> > > 
> > > if this is base-type code that is used by multiple modules, it should be
in
> > > src/base
> > 
> > Agreed. ajwong and danakj have taken on moving this code in base. I don't
> > believe this CL should be blocked on that move. Let me know if you disagree.
> 
> Yes, please let's wait on that.

I've moved the hash_pair into base/containers/hash_tables.

[jam, 2013-06-21 17:07:42]

lgtm

[commit-bot: I haz the power, 2013-06-21 18:33:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/16431010/43001

[commit-bot: I haz the power, 2013-06-21 20:23:42]

Change committed as 207913