Define the public version of the browser side RenderProcessHost interface. This interface is not ...

Define the public interface for content browser RenderProcessHost. This interface is implemented by the RenderProcessHostImpl class which lives in content\browser\renderer_host\render_process_host_impl.cc/.h. The RenderProcessHostImpl class is a consolidation of the RenderProcessHost and BrowserRenderProcessHost classes.

The RenderProcessHost public interface was created from the now deleted RenderProcessHost class defined in content\browser\renderer_host\render_process_host.h. 
Additional methods have been added to the interface to ensure that it works correctly with the MockRenderProcessHost class used by unit tests. I had to implement a number of overrides in the MockRenderProcessHost  class to ensure that tests work correctly. This was because of assumptions in the tests that the MockRPH class was a real RPH which it was till now.

Added a TODO for the methods which could potentially be removed from this interface. Will revisit that in a subsequent CL.

BUG=98716
TEST=No change in functionality. Hopefully it all compiles and works.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110615

[jam, 2011-11-11 03:24:29]

http://codereview.chromium.org/8515027/diff/4001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/8515027/diff/4001/content/content_browser.gypi...
content/content_browser.gypi:584: 'public/browser/render_process_host.h',
nit: spacing

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:10: #include "base/process.h"
is this really needed?

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:11: #include "base/process_util.h"
not needed?

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:12: #include "base/time.h"
forward declare?

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:14: #include
"ipc/ipc_channel_proxy.h"
nit: not needed

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:24: // Virtual interface that
represents the browser side of the browser <->
nit: "virtual interface" is redundant (sorry I know you just moved this!)

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:47: RenderProcessHost() {}
nit: not needed

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:56: virtual void EnableSendQueue()
= 0;
no one calls this in chrome. why does it need to be on the interface? just for
the test?

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:67: // Called on the UI thread to
cancel any outstanding resource requests for
nit: we should get rid of all the "Called on UI thread" parts of comments, since
this entire class is not thread safe and so should only be called on the UI
thread

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:69: virtual void
CancelResourceRequests(int render_widget_id) = 0;
no one calls this in chrome. why does it need to be on the interface? just for
the test?

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:75: virtual void
CrossSiteSwapOutACK(
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:81: virtual bool
WaitForUpdateMsg(int render_widget_id,
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:86: virtual void
ReceivedBadMessage() = 0;
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:101: virtual void DumpHandles() =
0;
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:119: virtual TransportDIB*
GetTransportDIB(TransportDIB::Id dib_id) = 0;
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:125: virtual void
SetCompositingSurface(
ditto

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:138: return
run_renderer_in_process_;
apart from not compiling, we should avoid member variables in the inteface..

should these really be part of the interface? outside of content, it's called by
InProcessBrowserTest (which should move to BrowserTestBase, since we'd want this
for content_browsretests as well) and FakeExternalTab. I'm not familiar with
this class, is it really needed there?

[ananta, 2011-11-16 19:47:14]

http://codereview.chromium.org/8515027/diff/4001/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/8515027/diff/4001/content/content_browser.gypi...
content/content_browser.gypi:584: 'public/browser/render_process_host.h',
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> nit: spacing

Done.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:10: #include "base/process.h"
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> is this really needed?

Needed for typedefs like ProcessHandle, etc used below.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:11: #include "base/process_util.h"
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> not needed?

We use the TerminationStatus enum defined in this file below:-
See RendererClosedDetails.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:12: #include "base/time.h"
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> forward declare?

Done.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:14: #include
"ipc/ipc_channel_proxy.h"
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> nit: not needed
Replaced with ipc_message.h to get the definition of IPC::Message::Sender

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:24: // Virtual interface that
represents the browser side of the browser <->
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> nit: "virtual interface" is redundant (sorry I know you just moved this!)

Done.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:47: RenderProcessHost() {}
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> nit: not needed

Done.

http://codereview.chromium.org/8515027/diff/4001/content/public/browser/rende...
content/public/browser/render_process_host.h:67: // Called on the UI thread to
cancel any outstanding resource requests for
On 2011/11/11 03:24:29, John Abd-El-Malek wrote:
> nit: we should get rid of all the "Called on UI thread" parts of comments,
since
> this entire class is not thread safe and so should only be called on the UI
> thread

Done.

[jam, 2011-11-16 22:10:19]

lgtm

it would be great if we can reduce the number of functions in RenderProcessHost,
but I agree it's better to get this in first

http://codereview.chromium.org/8515027/diff/15010/chrome/browser/visitedlink/...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

http://codereview.chromium.org/8515027/diff/15010/chrome/browser/visitedlink/...
chrome/browser/visitedlink/visitedlink_unittest.cc:499: class
VisitRelayingRenderProcessHost : public RenderProcessHostImpl {
this will have to change (in the future), since we won't be able to derive from
internal content classes. the only thing allowed via DEPS will be the interfaces
in content/public

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
File content/browser/renderer_host/mock_render_process_host.cc (right):

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/mock_render_process_host.cc:17: // Clean up this
dependency in a subsequent CL.
it's fine to do this in a future change since this has grown so much and I
sympathize with keeping this change alive! for base classes like this that are
interfaces, where as part of the interface we have getters based on IDs and
iterators etc, we've done this work in the interface's constructor/destructor so
that it's only done in one place

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_BROWSER_RENDER_PROCESS_HOST_H_
nit: ifdef guard

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:74: // Returns the user
browser context associated with this renderer process.
nit: don't duplicate comment from interface

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:93: // Try to shutdown
the associated render process as fast as possible, but
nit: ditto

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:97: 
nit: extra line

http://codereview.chromium.org/8515027/diff/15010/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/8515027/diff/15010/content/content_browser.gyp...
content/content_browser.gypi:586: 'public/browser/render_process_host.h',
nit: ordering

[ananta, 2011-11-17 13:34:45]

http://codereview.chromium.org/8515027/diff/15010/chrome/browser/visitedlink/...
File chrome/browser/visitedlink/visitedlink_unittest.cc (right):

http://codereview.chromium.org/8515027/diff/15010/chrome/browser/visitedlink/...
chrome/browser/visitedlink/visitedlink_unittest.cc:499: class
VisitRelayingRenderProcessHost : public RenderProcessHostImpl {
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> this will have to change (in the future), since we won't be able to derive
from
> internal content classes. the only thing allowed via DEPS will be the
interfaces
> in content/public

Yes. We can mock out the remaining RPH methods or perhaps look into moving this
test to content.

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
File content/browser/renderer_host/mock_render_process_host.cc (right):

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/mock_render_process_host.cc:17: // Clean up this
dependency in a subsequent CL.
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> it's fine to do this in a future change since this has grown so much and I
> sympathize with keeping this change alive! for base classes like this that are
> interfaces, where as part of the interface we have getters based on IDs and
> iterators etc, we've done this work in the interface's constructor/destructor
so
> that it's only done in one place

I will do that in a follow up CL.

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.h (right):

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:5: #ifndef
CONTENT_BROWSER_RENDERER_HOST_BROWSER_RENDER_PROCESS_HOST_H_
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> nit: ifdef guard

Done.

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:74: // Returns the user
browser context associated with this renderer process.
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> nit: don't duplicate comment from interface

Done.

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:93: // Try to shutdown
the associated render process as fast as possible, but
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> nit: ditto

Done.

http://codereview.chromium.org/8515027/diff/15010/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.h:97: 
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> nit: extra line

Done.

http://codereview.chromium.org/8515027/diff/15010/content/content_browser.gypi
File content/content_browser.gypi (right):

http://codereview.chromium.org/8515027/diff/15010/content/content_browser.gyp...
content/content_browser.gypi:586: 'public/browser/render_process_host.h',
On 2011/11/16 22:10:19, John Abd-El-Malek wrote:
> nit: ordering

Done.

[Aaron Boodman, 2012-01-24 23:23:21]

Extension parts LGTM