Re: design changes for https://codereview.chromium.org/284193007

Elly Jones

unread,

May 22, 2014, 5:58:19 PM5/22/14

to Nasko Oskov, site-isol...@chromium.org

+site-isolation-dev

On Thu, May 22, 2014 at 01:39:13PM -0700, Nasko Oskov wrote:
> On Thu, May 22, 2014 at 10:39 AM, Elly Jones <elly...@google.com> wrote:
>
> > So I'm considering how to bring my CL in line with jam's wishes. As I
> > understand it, the
> > current situation is that the RenderView receives an IPC from the
> > RenderWidgetHostImpl
> > (since RenderView is a RenderWidget), which it propagates to its WebView
> > and observers
> > as needed.
>
>
> Correct.
>
>
> > I think jam wants me to change the interaction between browser and
> > renderer such
> > that the IPC is sent by RenderFrameHostManager
>
>
> Most likely WebContentsImpl, not RenderFrameHostManager.
>
>
> > instead to its corresponding RenderFrame,
> > and also to all child RenderFrames. Does that sound right?
>
>
> Yes.
>
>
> > Do you think it's safe to reuse the same IPC?
>
>
> I think the model going forward should be along those lines: WebContents is
> hidden, therefore it needs to notify its widgets that it was hidden. It
> used to be only one before, it will be potentially multiple in the future.
> In general, if WebContents is hidden, it should be that all of its frames
> are hidden as well, so it is safe to just call WebContents::ForEachFrame
> with a callback that sends an IPC to each frame.
> I'm not sure whether hiding originates at the WebContents layer and
> propagates to RWH and codesearch references don't work for me to check
> easily. If that turns out to be incorrect, the above idea might not be
> quite the same in implementation, but the spirit will likely remain.

Unfortunately I don't think this is true. I added a base::debug::StackTrace to
content::RenderWidgetHostImpl::WasShown(), and:

#1 0x7f1a3fc6bc76 content::RenderWidgetHostImpl::WasShown()
#2 0x7f1a3fc83bc2 content::RenderWidgetHostViewAura::WasShown()
#3 0x7f1a3fc840d8 content::RenderWidgetHostViewAura::Show()
#4 0x7f1a3fab5ae2 content::RenderFrameHostManager::CommitPending()
#5 0x7f1a3fab689c content::RenderFrameHostManager::UpdateStateForNavigate()
#6 0x7f1a3fab2042 content::RenderFrameHostManager::Navigate()
#7 0x7f1a3fe8905a content::NavigatorImpl::NavigateToEntry()
#8 0x7f1a3fe894d6 content::NavigatorImpl::NavigateToPendingEntry()
#9 0x7f1a3fd470b5 content::WebContentsImpl::NavigateToPendingEntry()

So it looks like the decision to show the RenderWidget comes from RenderFrameHostManager.

Would it make sense to send the IPC from RenderFrameHostManager?

>
>
> > I'm kind of worried because RenderView is a RenderWidget but RenderFrame
> > is not, and I don't understand the difference.
> >
>
> RenderWidget(Host) is the object that deals with rendering pixels and
> routing input events. RenderView(Host) is the extension of that which adds
> navigation and other stuff on top of the widget. Context menus are an
> example of a RenderWidget(Host) that is *not* a RenderView(Host).
>
> In the future, RenderFrameHost will not extend RenderWidgetHost, rather it
> will own it. Additionally, there won't be 1-to-1 relationship between RFH
> and RWH, but rather there will be one RWH for a set of directly connected
> frames in the same process. For example, if we have A->B->A->A nested
> frames, there will be 2 RWHs - 1 for top level A, one for B, one for the
> other two As.
> In that world, it won't make as much sense for RWH to route the
> hidden/shown IPCs if we want the frames to take action on those events. It
> should move to the frame, which will then let the widget know and can take
> action or dispatch to observers.
>
> Does that make sense or did I manage to confuse you further?

I'm a little further confused :).

-- elly

Nasko Oskov

unread,

May 22, 2014, 7:25:52 PM5/22/14

to Elly Jones, site-isol...@chromium.org

I don't think this is the only stack trace that will hit WasShown on the widget. WebContentsImpl also has WasShown/WasHidden that call into the view.

John, does it make sense going forward to have Show/Hide be WebContents level concept and for RFHM to avoid calling this on subframes? It makes some sense for the show/hide should be on the RFH, which will then forward it to the associated view, though I'm not sure what the conventions between the RVH and its view are. Can you share how you see this working?

Nick Carter

unread,

May 22, 2014, 7:39:24 PM5/22/14

to Nasko Oskov, Elly Jones, site-isol...@chromium.org, Stefan Kuhne

+skuhne who was looking at this in https://codereview.chromium.org/286943005/

I was puzzling over this earlier in the week, and came to the following conclusion. As I understand it, the Show/Hide in CommitPending isn't really changing the visibility of the active widget -- instead what it's doing is: the widget is being swapped, and so the the visibility state from the old widget needs to be propagated to the new widget. Widgets that are not current (e.g. pending ones) are kept in the hidden state.

So it looks like the decision to show the RenderWidget comes from RenderFrameHostManager.

Would it make sense to send the IPC from RenderFrameHostManager?

I don't think this is the only stack trace that will hit WasShown on the widget. WebContentsImpl also has WasShown/WasHidden that call into the view.

Because I had it handy, here's another stack trace showing how we can wind up in WasShown:

content::RenderWidgetHostImpl::WasShown()

content::RenderWidgetHostViewAura::WasShown()

content::RenderWidgetHostViewAura::Show()

content::WebContentsImpl::WasShown()

content::WebContentsViewAura::OnWindowTargetVisibilityChanged(bool visible)

aura::Window::SetVisible(bool visible)

aura::Window::Show()

views::NativeViewHostAura::ShowWidget(int x, int y, int w, int h)

views::NativeViewHost::Layout()

views::View::BoundsChanged(const gfx::Rect & previous_bounds)