Tie extension/app initialization to RenderView creation, not RenderViewHost creation

InitRenderViewHostForExtensions would only be called when a new RenderViewHost
was created, but when reloading a crashed tab we would be reusing the RVH and just
making a new RenderView, the process would not be sent the ActivateExtension/Application
IPCs.

Move the process-specific initialization to ChromeRenderViewHostObserver::RenderViewHostInitialized,
which is called every time a new renderer process is created.

BUG=89607
TEST=no
R=creis@chromium.org,jam@chromium.org

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

[Charlie Reis, 2011-08-11 00:35:53]

This seems kind of indirect.  Should we be doing the extension-specific
RenderViewHostCreated logic as part of InitRenderView (or something similar)
instead?  Or at least the IPC part of it?

Seems like we'd always want that to happen when a new renderer for an app was
created.  I don't see why we need to create a new SiteInstance or RenderViewHost
for it.  In fact, that might confuse the BrowsingInstance, which expects to only
have one SiteInstance per site.

[Mihai Parparita -not on Chrome, 2011-08-11 01:33:10]

+jam since this now touches content/browser/*

On 2011/08/11 00:35:53, creis wrote:
> This seems kind of indirect.  Should we be doing the extension-specific
> RenderViewHostCreated logic as part of InitRenderView (or something similar)
> instead?  Or at least the IPC part of it?

That makes sense. RenderViewHostManager::InitRenderView isn't quite the right
place (because it's in content/, so it doesn't know anything about extensions).
Instead I added a new RenderViewCreated method to ContentBrowserClient.

[Charlie Reis, 2011-08-11 16:53:43]

This looks great.  If we find a way around the AllowBindings issue, I think
we'll be set.

http://codereview.chromium.org/7612016/diff/2002/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/7612016/diff/2002/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:143:
render_view_host->AllowBindings(BindingsPolicy::EXTENSION);
Looks like this is causing test failures because the RenderViewHost is already
initialized.

http://codereview.chromium.org/7612016/diff/2002/content/browser/content_brow...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7612016/diff/2002/content/browser/content_brow...
content/browser/content_browser_client.h:71: // Notifies that a new RenderView
has been created.
Nit: "for the given RenderViewHost."

[jam, 2011-08-11 18:13:36]

http://codereview.chromium.org/7612016/diff/2002/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/7612016/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:195:
content::GetContentClient()->browser()->RenderViewCreated(this);
It's best to avoid adding methods to ContentBrowserClient unless there's no
other way (i.e. at the point that content needs to tell the embedder, there's no
other context). The reason is that ContentBrowserClient is already pretty large,
and the smaller we can keep it the easier it'll be for embedders.

In this case, we already tell the RenderViewHostObservers. Chrome implements one
in chrome/browser/renderer_host/chrome_render_view_host_observer.cc, so how
about moving the code there?

[Mihai Parparita -not on Chrome, 2011-08-11 18:46:53]

On 2011/08/11 16:53:43, creis wrote:
> Looks like this is causing test failures because the RenderViewHost is already
> initialized.

Should be fixed, now that I've kept part of InitRenderViewHostForExtensions in
ChromeContentBrowserClient for stuff like this, and moved the process-specific
parts to ChromeRenderViewHostObserver::InitRenderViewHostForExtensions.

> In this case, we already tell the RenderViewHostObservers. Chrome implements
one
> in chrome/browser/renderer_host/chrome_render_view_host_observer.cc, so how
> about moving the code there?

Makes sense, done.

Mihai

[jam, 2011-08-11 18:52:56]

lgtm

[Charlie Reis, 2011-08-11 19:10:55]

Thanks-- just one more question inline.

http://codereview.chromium.org/7612016/diff/4009/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (left):

http://codereview.chromium.org/7612016/diff/4009/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:108:
site_instance->GetProcess()->mark_is_extension_process();
Ah yes, this only needs to be done once, because the RPH stays constant even
when the process dies and is recreated.

http://codereview.chromium.org/7612016/diff/4009/chrome/browser/renderer_host...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/7612016/diff/4009/chrome/browser/renderer_host...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:38: // Note
that due to GetEffectiveURL(), even hosted apps will have a
A little unfortunate that we're duplicating the code from here to line 55, plus
some of the "if" blocks below.  We've had bugs in this before and it'd be nice
not to track down new ones in both places.

I wonder if it's better to move it all here and out of
ChromeContentBrowserClient, and then only do the three one-time-init actions
(mark_is_extension_process, RegisterExtensionSiteInstance, and AllowBindings) if
the RenderViewHost isn't yet initialized.  Or do those actions need to be done
earlier?

[Mihai Parparita -not on Chrome, 2011-08-11 20:40:38]

On Thu, Aug 11, 2011 at 12:10 PM, <creis@chromium.org> wrote:

> I wonder if it's better to move it all here and out of
> ChromeContentBrowserClient, and then only do the three one-time-init
> actions (mark_is_extension_process, RegisterExtensionSiteInstance, and
> AllowBindings) if the RenderViewHost isn't yet initialized.  Or do those
> actions need to be done earlier?


By the time RenderViewHostInitialized is called, the RVH is always
initialized. However, that early init work can be done in
the ChromeRenderViewHostObserver constructor, so I've moved it to there (and
the extension-getting code is now
in ChromeRenderViewHostObserver::GetExtension, so that it can be used in
both places).

Mihai

[Charlie Reis, 2011-08-11 21:02:23]

Great-- LGTM!

http://codereview.chromium.org/7612016/diff/12001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

http://codereview.chromium.org/7612016/diff/12001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:59: const
Extension* extension = GetExtension();
Please add a comment saying this is the one-time initialization for a
RenderViewHost, and a similar comment below saying it's for initialization
needed each time the RenderViewHost creates a RenderView in the renderer
process.

[Mihai Parparita -not on Chrome, 2011-08-11 21:40:34]

On 2011/08/11 21:02:23, creis wrote:
> chrome/browser/renderer_host/chrome_render_view_host_observer.cc:59: const
> Extension* extension = GetExtension();
> Please add a comment saying this is the one-time initialization for a
> RenderViewHost, and a similar comment below saying it's for initialization
> needed each time the RenderViewHost creates a RenderView in the renderer
> process.

I already have those comments in the header file, do you want me to repeat them?

New version of the patch fixes IsolatedAppApiTest.CookieIsolation by calling
SetInstalledAppForRenderer earlier.

[Charlie Reis, 2011-08-11 22:24:58]

On 2011/08/11 21:40:34, Mihai Parparita wrote:
> I already have those comments in the header file, do you want me to repeat
them?

Ah, that works.  No need to repeat.

LGTM.

[commit-bot: I haz the power, 2011-08-12 03:59:26]

Change committed as 96505