Remove notifications from ContentViewCore.

Remove notifications from ContentViewCore.

BUG=170921
TEST=everything still works

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

[Avi (use Gerrit), 2013-11-21 21:28:25]

ppi: review
joth: fyi

This is in response to https://codereview.chromium.org/62203016/

https://codereview.chromium.org/81243003/diff/50001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/81243003/diff/50001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:217: void
ContentViewCoreImpl::RenderViewReady() {
== NOTIFICATION_WEB_CONTENTS_CONNECTED

https://codereview.chromium.org/81243003/diff/50001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:233: RenderViewHost* new_host)
{
== NOTIFICATION_RENDER_VIEW_HOST_CHANGED

[ppi, 2013-11-26 17:25:29]

+cc Grace

First of all - this overall looks great and thank you for looking into this!

After giving this a try locally, I see one change in behavior that I would like
to point out

When a new tab is created in current ToT:
- first we receive RENDER_VIEW_HOST_CHANGED, the renderer is not ready yet, so
we do nothing
- then we receive RENDER_PROCESS_CREATED and we call onRenderProcessSwap(0, pid)

With the patch, when a new tab is created:
- first we receive RenderViewHostChanged() call, the new renderer *is* ready, so
we call onRenderProcessSwap(0, pid)
- then we receive RenderViewReady() call and we do onRenderProcessSwap(0, pid)
again

I don't know at all if this is at all - perhaps it is possible to get duplicate
calls to onRenderProcessSwap under current implementation as well; this is just
what I observe locally.

This is harmless when the CVC is not considering itself "in foreground" (so
perhaps we would be safe...) but otherwise we may leak strong bindings, that are
created for each onRenderProcessSwap() call.

It would be great to be sure that when a render process is created for the CVC
we get 1 and only one call to onRenderProcessSwap() - please let me think about
this for a day and please let me know if you have any thoughts on all of the
above.

[ppi, 2013-11-26 17:28:38]

The confusing sentence ("I don't know at all ...") should read: "I don't know it
this is at all well-defined."

Apologies!

[Avi (use Gerrit), 2013-11-27 16:58:36]

On 2013/11/26 17:25:29, ppi wrote:
> When a new tab is created in current ToT:
> - first we receive RENDER_VIEW_HOST_CHANGED, the renderer is not ready yet, so
> we do nothing
> [...]
> With the patch, when a new tab is created:
> - first we receive RenderViewHostChanged() call, the new renderer *is* ready,
so
> we call onRenderProcessSwap(0, pid)

This is very much a surprise to me. If we look at WebContentsImpl,
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
, those two calls (RENDER_VIEW_HOST_CHANGED and RenderViewHostChanged) are
literally fired at exactly the same time. I don't understand why this would
change.

> It would be great to be sure that when a render process is created for the CVC
> we get 1 and only one call to onRenderProcessSwap() - please let me think
about
> this for a day and please let me know if you have any thoughts on all of the
> above.

The callback behavior we have now is pretty much all historical accident. What
you describe is a request that the callback API surface act in a sane way, and I
agree. I would want to have the callbacks be at a time where they work well and
have all the info you want. I'm willing to rework the guts of content to make
that happen. I'm just surprised that we're getting the result you want even
without change.

[ppi, 2013-11-27 18:50:41]

Thanks for following up!

I think that yesterday I misinterpreted the results I was seeing - apologies for
the confusion! But I think there is at least one real issue here - please let me
elaborate (and hopefully this time I got this right).

If I see this correctly, RenderViewReady() is called every time a RVH connects
with a renderer, including connecting to a renderer that's already well and
running (i.e. when we're sharing renderer processes between tabs). This is
different from the NOTIFICATION_RENDERER_PROCESS_CREATED notification, that we
get only when a truly new renderer is created.

That breaks us in the "shared renderers" scenario:
a) Tab A is created and renderer process R is created for it.
b) Tab B is created and for whatever reason (e.g. both are NTPs) it will reuse
the already running process R. The relevant parts of starting the tab run as
follows:
- - RenderViewHostManager::CommitPending() swaps in the new RVH. This doesn't
matter much (for the ContentView), because the CV is not considering itself in
foreground so we won't alter any bindings. But then...
- - We call onShow() on our CV, at this point we will grab the pid of the
renderer of our RVH and put a binding on it.
- - And *after* that we get RenderViewReady(). The CV considers itself in
foreground, so another binding will be placed on the renderer, which should
*not* happen.

Without the patch, the (equivalent of the) last point just doesn't happen. Right
now I don't see how we can fix this nicely, but maybe I've just been staring at
it for too long.

What do you think?

[Avi (use Gerrit), 2014-01-23 20:52:04]

I'm going to close this.

It looks like you guys are switching to a simpler and easier-to-understand
scheme with bug http://crbug.com/325896 which will eventually replace this code.
I will keep an eye on that.

[Avi (use Gerrit), 2014-05-22 14:58:37]

OK, Przemysław, WDYT?

[ppi, 2014-05-22 15:58:44]

Thanks, this LGTM!

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:307:
view->SetContentViewCore(this);
Looking at what attachImeAdapter() cares about (attaching to entity that is
specific to RWHV), we probably should be comparing the old RWHVA with the new
one and we would call something like onRenderWidgetHostViewChanged().

This is independent of this change, though, probably better to address that
separately.

[Ted C, 2014-05-22 16:29:24]

lgtm (espeically with the patch set two title)

Adding a bunch of other people to make sure all areas are covered, but to me
this all looks good.

sievers, powei - re-assignment of CVC on the RWHVA
aurimas, boliu - ime attachment

[boliu, 2014-05-22 17:18:41]

On 2014/05/22 16:29:24, Ted C wrote:
> lgtm (espeically with the patch set two title)
> 
> Adding a bunch of other people to make sure all areas are covered, but to me
> this all looks good.
> 
> sievers, powei - re-assignment of CVC on the RWHVA
> aurimas, boliu - ime attachment

Looks find to me, but I don't really know much about ime...

[powei, 2014-05-22 17:52:42]

lgtm on the CVC/RWHVA part

[Avi (use Gerrit), 2014-05-22 20:32:35]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-05-22 20:34:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/81243003/200001

[no sievers, 2014-05-22 20:44:57]

LGTM

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:307:
view->SetContentViewCore(this);
On 2014/05/22 15:58:44, ppi wrote:
> Looking at what attachImeAdapter() cares about (attaching to entity that is
> specific to RWHV), we probably should be comparing the old RWHVA with the new
> one and we would call something like onRenderWidgetHostViewChanged().
> 
> This is independent of this change, though, probably better to address that
> separately.

I don't think the patch is changing the behavior in that regard, but now I'm
wondering if it's correct that lie 305-307 is inside |if (old_host)|. In
web_contents_observer.h it says that old_host maybe be 'NULL if the old RVH was
shut down.'

[Avi (use Gerrit), 2014-05-22 21:14:26]

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
content/browser/android/content_view_core_impl.cc:307:
view->SetContentViewCore(this);
This is exactly the analog to NOTIFICATION_RENDER_VIEW_HOST_CHANGED, and lines
319-323 were inside the if(). Not to say that that was right, but I'm not the
expert here, so I was following the existing code.

[no sievers, 2014-05-22 21:26:19]

On 2014/05/22 21:14:26, Avi wrote:
>
https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
> 
>
https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:307:
> view->SetContentViewCore(this);
> This is exactly the analog to NOTIFICATION_RENDER_VIEW_HOST_CHANGED, and lines
> 319-323 were inside the if(). Not to say that that was right, but I'm not the
> expert here, so I was following the existing code.

Yes, definitely not your fault :)

Do you happen to know how I could force a case where old_host is NULL so that I
can see what happens?

Either way, not saying you should hold your patch or anything. This is a
separate problem.

[Avi (use Gerrit), 2014-05-22 21:32:13]

On 2014/05/22 21:26:19, sievers wrote:
> On 2014/05/22 21:14:26, Avi wrote:
> >
>
https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
> > File content/browser/android/content_view_core_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/81243003/diff/200001/content/browser/android/...
> > content/browser/android/content_view_core_impl.cc:307:
> > view->SetContentViewCore(this);
> > This is exactly the analog to NOTIFICATION_RENDER_VIEW_HOST_CHANGED, and
lines
> > 319-323 were inside the if(). Not to say that that was right, but I'm not
the
> > expert here, so I was following the existing code.
> 
> Yes, definitely not your fault :)
> 
> Do you happen to know how I could force a case where old_host is NULL so that
I
> can see what happens?
> 
> Either way, not saying you should hold your patch or anything. This is a
> separate problem.

Not a problem.

old_host is NULL in a very specific case. Check out
RenderFrameHostManager::Navigate, specifically the call to
NotifySwappedFromRenderManager (which ends up calling the observer's
RenderViewHostChanged) that has NULL as the first argument. I'm tracing through
the code trying to work out the exact scenario, and it has something to do with
crashed tabs. I'm not super familiar with this.

[commit-bot: I haz the power, 2014-05-23 00:11:13]

Change committed as 272377