Add Blink APIs for frame tree mirroring.

Add Blink APIs for frame tree mirroring.

Traditionally, frame tree mutation has been driven from inside the renderer
itself. However, for out-of-process iframes, the embedder needs to mirror the
same frame tree structure between several different renderers.

BUG=346764
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176159

[dcheng, 2014-06-11 23:15:37]

I'm not wholly certain of the approach I've taken for FrameOwner ownership in
this patch. In particular, it feels kind of ugly that clearContentFrame()
results in self deletion when the parent is a remote frame. However, I
considered several other alternatives, and I considered this the least bad one:
1. Adding a RefPtr<> on WebFrame in case the frame needs a remote parent proxy.
2. Adding a wrapper WebFrameClient to listen for frame detach.
3. The current implementation.

In the usual case, this is a lot easier--the parent frame itself retains a
reference to the FrameOwner via DOM. But we don't have that if the parent is
remote =(

[dcheng, 2014-06-13 16:47:43]

On 2014/06/11 at 23:15:37, dcheng wrote:
> I'm not wholly certain of the approach I've taken for FrameOwner ownership in
this patch. In particular, it feels kind of ugly that clearContentFrame()
results in self deletion when the parent is a remote frame. However, I
considered several other alternatives, and I considered this the least bad one:
> 1. Adding a RefPtr<> on WebFrame in case the frame needs a remote parent
proxy.
> 2. Adding a wrapper WebFrameClient to listen for frame detach.
> 3. The current implementation.
> 
> In the usual case, this is a lot easier--the parent frame itself retains a
reference to the FrameOwner via DOM. But we don't have that if the parent is
remote =(

Ping?

[abarth-chromium, 2014-06-13 18:06:46]

I feel a bit like I'm walking into the middle of a movie.  The code generally
makes sense, but I'm worried I don't have enough context to evaluate it
properly.

For example,

1) Is RemoteBridgeFrameOwner meant to be a transition concept or is that
something that's part of the final design?

2) Why are the frame owner objects self-owning?  I would have expected them to
be owned by the frame tree, perhaps on the embedder side of the API if the
embedder owns the frame tree.  Is this a transitional phase, or is this meant to
be the final design?

https://codereview.chromium.org/334483002/diff/20001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/334483002/diff/20001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.h:43: virtual void clearContentFrame()
OVERRIDE;
This can still be private.  That will force callers to use FrameOwner's
interface.

https://codereview.chromium.org/334483002/diff/20001/Source/web/WebRemoteFram...
File Source/web/WebRemoteFrameImpl.cpp (right):

https://codereview.chromium.org/334483002/diff/20001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:54: delete this;
delete this -> :(

Is there really no object that owns this object?

https://codereview.chromium.org/334483002/diff/20001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:88: delete this;
ditto

https://codereview.chromium.org/334483002/diff/20001/Source/web/WebRemoteFram...
Source/web/WebRemoteFrameImpl.cpp:825:
child->initializeAsChildFrame(frame()->host(), new
RemoteBridgeFrameOwner(child), name, AtomicString());
adoptPtr(new RemoteBridgeFrameOwner(...    ?

https://codereview.chromium.org/334483002/diff/20001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/334483002/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:205: virtual void initializeAsMainFrame(WebView*) = 0;
Why is this API public?  If it's called by the WebView, it can be private,
right?

[dcheng, 2014-06-13 23:56:12]

On 2014/06/13 18:06:46, abarth wrote:
> I feel a bit like I'm walking into the middle of a movie.  The code generally
> makes sense, but I'm worried I don't have enough context to evaluate it
> properly.
> 
> For example,
> 
> 1) Is RemoteBridgeFrameOwner meant to be a transition concept or is that
> something that's part of the final design?
> 

This is meant to be part of the final design. It's meant to provide a way for a
frame to get info about its remote parent (sandbox flags set on the owner
element) and signal back to its remote parent (triggering a load event on the
owner element when loading is complete). I've updated the class comments to try
to better explain why we're using them.

> 2) Why are the frame owner objects self-owning?  I would have expected them to
> be owned by the frame tree, perhaps on the embedder side of the API if the
> embedder owns the frame tree.  Is this a transitional phase, or is this meant
to
> be the final design?
> 

I've updated them so they no longer rely on self-deletion.

https://codereview.chromium.org/334483002/diff/20001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.h (right):

https://codereview.chromium.org/334483002/diff/20001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.h:43: virtual void clearContentFrame()
OVERRIDE;
On 2014/06/13 18:06:46, abarth wrote:
> This can still be private.  That will force callers to use FrameOwner's
> interface.

Done.

https://codereview.chromium.org/334483002/diff/20001/public/web/WebFrame.h
File public/web/WebFrame.h (right):

https://codereview.chromium.org/334483002/diff/20001/public/web/WebFrame.h#ne...
public/web/WebFrame.h:205: virtual void initializeAsMainFrame(WebView*) = 0;
On 2014/06/13 18:06:46, abarth wrote:
> Why is this API public?  If it's called by the WebView, it can be private,
> right?

I'd like to avoid friending WebViewImpl if possible--it seems a bit out of place
here.

How would you feel about de-virtualizing this, and updating
WebViewImpl::setMainFrame to dispatch like this:
    if (f->isLocal())
        toWebLocalFrameImpl(f)->initializeAsMainFrame(page());
    else
        toWebRemoteFrameImpl(f)->initializeAsMainFrame(page());

[abarth-chromium, 2014-06-14 00:44:49]

On 2014/06/13 at 23:56:12, dcheng wrote:
> How would you feel about de-virtualizing this, and updating
WebViewImpl::setMainFrame to dispatch like this:
>     if (f->isLocal())
>         toWebLocalFrameImpl(f)->initializeAsMainFrame(page());
>     else
>         toWebRemoteFrameImpl(f)->initializeAsMainFrame(page());

That seems better than adding a public API that's only for internal use.

[dcheng, 2014-06-14 21:08:48]

On 2014/06/14 00:44:49, abarth wrote:
> On 2014/06/13 at 23:56:12, dcheng wrote:
> > How would you feel about de-virtualizing this, and updating
> WebViewImpl::setMainFrame to dispatch like this:
> >     if (f->isLocal())
> >         toWebLocalFrameImpl(f)->initializeAsMainFrame(page());
> >     else
> >         toWebRemoteFrameImpl(f)->initializeAsMainFrame(page());
> 
> That seems better than adding a public API that's only for internal use.

Done.

[abarth-chromium, 2014-06-14 21:46:34]

lgtm

[dcheng, 2014-06-14 22:22:44]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2014-06-14 22:23:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dcheng@chromium.org/334483002/100001

[commit-bot: I haz the power, 2014-06-14 23:27:44]

Change committed as 176159

[Stephen Chennney, 2014-06-16 11:10:35]

This change is causing failures on the linux Asan bots.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

Bug will be filed.

[Stephen Chennney, 2014-06-16 11:15:18]

On 2014/06/16 11:10:35, Stephen Chenney wrote:
> This change is causing failures on the linux Asan bots.
> 
> http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN
> 
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> Bug will be filed.

https://code.google.com/p/chromium/issues/detail?id=385015