Remove EPM:all_hosts_ and use all_extension_views_ instead.

Remove EPM:all_hosts_ and use all_extension_views_ instead.

EPM:all_hosts_ is used to look for extension render view hosts,
under the assumption it has all of them. This assumption is wrong,
and the name is confusing, so we're removing all_hosts_. A different
container can be used: all_extension_views, which contains
all the RenderViewHosts for extensions.

BUG=102617
TEST=Covered by existing tests


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

[benwells, 2012-04-19 07:36:03]

This is continuing https://chromiumcodereview.appspot.com/10116017/, using
WebContents instead of RenderViewHost.

[benwells, 2012-04-19 11:34:02]

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:23: #include
"content/browser/web_contents/web_contents_impl.h"
This causes a build error due to an illegal dependency.

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:194: return
static_cast<WebContentsImpl*>(rvh->GetDelegate());
How should I do this correctly?

If I use reinterpret_cast  to cast to WebContents it doesn't work, which I
*think* is because the multiple inheritance in the WebContentsImpl making the
WebContents and WebContentsImpl pointers not equal.

As it is it doesn't work either, as evidenced by the badness in the try runs.

[Matt Perry, 2012-04-19 20:53:19]

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:194: return
static_cast<WebContentsImpl*>(rvh->GetDelegate());
On 2012/04/19 11:34:02, benwells wrote:
> How should I do this correctly?
> 
> If I use reinterpret_cast  to cast to WebContents it doesn't work, which I
> *think* is because the multiple inheritance in the WebContentsImpl making the
> WebContents and WebContentsImpl pointers not equal.
> 
> As it is it doesn't work either, as evidenced by the badness in the try runs.

You're having trouble here because you're trying to get around layering
constraints. Since you're changing EPM from registering RenderViewHosts to
registering WebContents, you should move the registration to when the
WebContents is created. There's already a NOTIFICATION_WEB_CONTENTS_DESTROYED,
but it looks like you have to add one for _CREATED. Then have EPM listen to
those, and do registration there.

[Charlie Reis, 2012-04-19 21:42:10]

I don't think this change is safe.  See below for details, but a WebContents can
have many RenderViewHosts, and the visible one changes over time as the tab
navigates.

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:492: RenderViewHost* host =
contents->GetRenderViewHost();
We need to be very careful here.  GetRenderViewHost returns the *current*
RenderViewHost for a WebContents, which changes over time as the tab navigates. 
Are we certain this WebContents hasn't navigated and stopped showing an
extension?

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
File chrome/browser/renderer_host/chrome_render_view_host_observer.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/rend...
chrome/browser/renderer_host/chrome_render_view_host_observer.cc:183:
process_manager->UnregisterWebContents(contents);
I don't think this will get called if you navigate away from an extension while
there are other things keeping the extension process alive.  The RenderViewHost
will just be "swapped out" and the EPM will think this WebContents is showing an
extension when it is not.

[Aaron Boodman, 2012-04-20 01:00:55]

Ben and I looked into a different approach where we leave EPM containing RVH.
This works, except we need to add an accessor to RWH that we weren't sure
content-team would be OK with.

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:489: contents_set.begin(); iter !=
contents_set.end();
indent -= 4

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/8001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:492: RenderViewHost* host =
contents->GetRenderViewHost();
On 2012/04/19 21:42:10, creis wrote:
> We need to be very careful here.  GetRenderViewHost returns the *current*
> RenderViewHost for a WebContents, which changes over time as the tab
navigates. 
> Are we certain this WebContents hasn't navigated and stopped showing an
> extension?

Good point. My bad, I forgot that WebContents encapsulated navigation.

[benwells, 2012-04-20 01:08:50]

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:489: contents_set.begin(); iter !=
contents_set.end();
On 2012/04/20 01:00:55, Aaron Boodman wrote:
> indent -= 4

Oh, I've done that all over the place, I thought it came in because it is in the
middle of the assignment (iter = ...). Although the second iter != statemtn
should drop down a line  and line up with ++iter.

[Aaron Boodman, 2012-04-20 01:15:11]

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:489: contents_set.begin(); iter !=
contents_set.end();
On 2012/04/20 01:08:51, benwells wrote:
> On 2012/04/20 01:00:55, Aaron Boodman wrote:
> > indent -= 4
> 
> Oh, I've done that all over the place, I thought it came in because it is in
the
> middle of the assignment (iter = ...).

Oh, it was right originally.

[Matt Perry, 2012-04-20 01:15:52]

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
File chrome/browser/automation/automation_util.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/2001/chrome/browser/auto...
chrome/browser/automation/automation_util.cc:489: contents_set.begin(); iter !=
contents_set.end();
On 2012/04/20 01:08:51, benwells wrote:
> On 2012/04/20 01:00:55, Aaron Boodman wrote:
> > indent -= 4
> 
> Oh, I've done that all over the place, I thought it came in because it is in
the
> middle of the assignment (iter = ...). Although the second iter != statemtn
> should drop down a line  and line up with ++iter.

I agree with Ben here. I'd format it:
for (iterator iter =
         begin();
     iter != end(); ++iter) {
}

[benwells, 2012-04-20 03:55:14]

Back to RVH, running try jobs now....

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/aut...
File chrome/browser/automation/automation_provider_observers.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/aut...
chrome/browser/automation/automation_provider_observers.cc:511:
manager->background_hosts().begin();
This logic has changed, its now only background hosts. aa: is this intentional /
correct?

[Aaron Boodman, 2012-04-20 06:34:56]

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/aut...
File chrome/browser/automation/automation_provider_observers.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/aut...
chrome/browser/automation/automation_provider_observers.cc:511:
manager->background_hosts().begin();
On 2012/04/20 03:55:14, benwells wrote:
> This logic has changed, its now only background hosts. aa: is this intentional
/
> correct?

It depends on what the scripts that call this automation code are using it for.

To be conservative, let's just wait until all the RVH in EPM stop.

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
File chrome/browser/extensions/extension_devtools_browsertests.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
chrome/browser/extensions/extension_devtools_browsertests.cc:39:
ExtensionProcessManager::ExtensionHostSet background_hosts =
This doesn't do the same thing, but I guess all the tests only cared about
background pages.

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:41: using
content::WebContents;
not needed

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.h (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.h:28: class WebContents;
You can remove this.

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.h:46: const
ExtensionHostSet& platform_app_hosts() const {
It seems like this is only needed for one test. Can we implement that test
differently? Like maybe by counting all the views that have a certain view type?

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ren...
File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/ren...
chrome/browser/renderer_host/chrome_render_view_host_observer.h:15: namespace
content {
?

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/tas...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/chrome/browser/tas...
chrome/browser/task_manager/task_manager_resource_providers.cc:1265:
content::VIEW_TYPE_WEB_CONTENTS) {
TYPE_WEB_CONTENTS is actually the default type. It would be good to have a
specific TYPE_TAB_CONTENTS in chrome_view_type.h. Can you add a TODO?

https://chromiumcodereview.appspot.com/10113005/diff/22001/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:246: bool
RenderWidgetHostImpl::IsLoading() const {
Style should be: is_loading()

[benwells, 2012-04-23 05:00:26]

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/automation/...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/automation/...
chrome/browser/automation/automation_provider_observers.cc:511:
manager->background_hosts().begin();
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> On 2012/04/20 03:55:14, benwells wrote:
> > This logic has changed, its now only background hosts. aa: is this
intentional
> /
> > correct?
> 
> It depends on what the scripts that call this automation code are using it
for.
> 
> To be conservative, let's just wait until all the RVH in EPM stop.

Done.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_devtools_browsertests.cc (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/extension_devtools_browsertests.cc:39:
ExtensionProcessManager::ExtensionHostSet background_hosts =
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> This doesn't do the same thing, but I guess all the tests only cared about
> background pages.

Renamed to be clear its background pages.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:41: using
content::WebContents;
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> not needed

Done.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.h:28: class WebContents;
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> You can remove this.

Done.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.h:46: const
ExtensionHostSet& platform_app_hosts() const {
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> It seems like this is only needed for one test. Can we implement that test
> differently? Like maybe by counting all the views that have a certain view
type?

They can removed for all platform app tests, except the context menu tests. They
initialize a render view context menu using the WebContents from the
ExtensionHost.

I've been thinking with the split of PlatformShellHost out from ExtensionHost it
might be useful to have a list (for each profile, not static) of the
PlatformShellHost's or shell windows created. Perhaps it would be for each
profile and each app. It would be useful for tests and also platform app APIs
(like the new windows api).

Can I leave this list in until that refactor, during which it would be removed
from ExtensionProcessManager?

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/chrome_render_view_host_observer.h (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/chrome_render_view_host_observer.h:15: namespace
content {
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> ?

Done.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/task_manage...
File chrome/browser/task_manager/task_manager_resource_providers.cc (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/task_manage...
chrome/browser/task_manager/task_manager_resource_providers.cc:1265:
content::VIEW_TYPE_WEB_CONTENTS) {
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> TYPE_WEB_CONTENTS is actually the default type. It would be good to have a
> specific TYPE_TAB_CONTENTS in chrome_view_type.h. Can you add a TODO?

Done.

http://codereview.chromium.org/10113005/diff/22001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

http://codereview.chromium.org/10113005/diff/22001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host_impl.cc:246: bool
RenderWidgetHostImpl::IsLoading() const {
On 2012/04/20 06:34:56, Aaron Boodman wrote:
> Style should be: is_loading()

I think its like this as its a private impl of an abstract class, see GetView
and GetRoutingId above.

[Charlie Reis, 2012-04-23 20:21:49]

I like this approach much better!  One minor comment below.

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:325:
render_view_host->GetSiteInstance()->GetSite().host());
I think this is correct for extensions.  A SiteInstance's site isn't always the
same as the URL being shown, but that only really applies to normal pages.  For
extensions, the SiteInstance's site is always valid.

I'm not sure how careful callers of this are, but it is possible for a
RenderViewHost to have a null SiteInstance, and for a SiteInstance to not yet
have a site (see HasSite()).  Only really matters if you pass in RenderViewHosts
that might not be showing extensions.

[Matt Perry, 2012-04-23 21:53:04]

https://chromiumcodereview.appspot.com/10113005/diff/22001/content/browser/re...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/22001/content/browser/re...
content/browser/renderer_host/render_widget_host_impl.cc:246: bool
RenderWidgetHostImpl::IsLoading() const {
On 2012/04/23 05:00:26, benwells wrote:
> On 2012/04/20 06:34:56, Aaron Boodman wrote:
> > Style should be: is_loading()
> 
> I think its like this as its a private impl of an abstract class, see GetView
> and GetRoutingId above.

Yeah, is_loading() style is only used for cheap non-virtual inline methods.

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:410: iter =
all_views.begin();
Given the comment, this should be:
all_views = manager->GetAllViews();
iter = all_views.begin();

[benwells, 2012-04-24 04:31:26]

Comments addressed.

+avi for contents/ changes

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/extension_browsertest.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
chrome/browser/extensions/extension_browsertest.cc:410: iter =
all_views.begin();
On 2012/04/23 21:53:04, Matt Perry wrote:
> Given the comment, this should be:
> all_views = manager->GetAllViews();
> iter = all_views.begin();

Done.

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://chromiumcodereview.appspot.com/10113005/diff/45001/chrome/browser/ext...
chrome/browser/extensions/extension_process_manager.cc:325:
render_view_host->GetSiteInstance()->GetSite().host());
On 2012/04/23 20:21:49, creis wrote:
> I think this is correct for extensions.  A SiteInstance's site isn't always
the
> same as the URL being shown, but that only really applies to normal pages. 
For
> extensions, the SiteInstance's site is always valid.
> 
> I'm not sure how careful callers of this are, but it is possible for a
> RenderViewHost to have a null SiteInstance, and for a SiteInstance to not yet
> have a site (see HasSite()).  Only really matters if you pass in
RenderViewHosts
> that might not be showing extensions.

OK, SiteInstance is checked here and in GetExtensionID above. Note that HasSite
is private. It shouldn't matter, if HasSite is false GetSite will return an
empty URL which won't resolve to any extension.

[Matt Perry, 2012-04-24 23:22:59]

lgtm

[Charlie Reis, 2012-04-25 00:04:12]

LGTM

http://codereview.chromium.org/10113005/diff/53001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_devtools_browsertests.cc (right):

http://codereview.chromium.org/10113005/diff/53001/chrome/browser/extensions/...
chrome/browser/extensions/extension_devtools_browsertests.cc:32: // Looks for an
background ExtensionHost whose URL has the given path component
nit: a background

[benwells, 2012-04-25 01:42:52]

-avi, +jam for content/

[jam, 2012-04-25 01:52:36]

content lgtm

[Aaron Boodman, 2012-04-25 21:27:05]

LGTM

Up to you as to whether to get rid of platform_hosts_ now or in a subsequent
change.

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/10113005/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.h:46: const
ExtensionHostSet& platform_app_hosts() const {
On 2012/04/23 05:00:26, benwells wrote:
> On 2012/04/20 06:34:56, Aaron Boodman wrote:
> > It seems like this is only needed for one test. Can we implement that test
> > differently? Like maybe by counting all the views that have a certain view
> type?
> 
> They can removed for all platform app tests, except the context menu tests.
They
> initialize a render view context menu using the WebContents from the
> ExtensionHost.

I see. Since you know the view type, perhaps you could downcast from
RVH->delegate() to WebContents in that test.

> I've been thinking with the split of PlatformShellHost out from ExtensionHost
it
> might be useful to have a list (for each profile, not static) of the
> PlatformShellHost's or shell windows created. Perhaps it would be for each
> profile and each app. It would be useful for tests and also platform app APIs
> (like the new windows api).

Yeah, like you mentioned before, I think that if there is going to be any
registering-in-big-lists like all_views_ it should be the top-level classes that
do it.

The way things get registered implicitly when WebContents is created seems
destined to lead to pain.

> Can I leave this list in until that refactor, during which it would be removed
> from ExtensionProcessManager?

OK.

http://codereview.chromium.org/10113005/diff/53001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/10113005/diff/53001/chrome/browser/extensions/...
chrome/browser/extensions/extension_process_manager.cc:555: const Extension*
extension = GetExtensionForRenderViewHost(
Thanks for cleaning all these up!

[commit-bot: I haz the power, 2012-04-25 23:08:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10113005/53001

[commit-bot: I haz the power, 2012-04-26 00:47:37]

Change committed as 134028