Prepare for remote->local frame swap

Prepare for remote->local frame swap

The plan is to create a LocalFrame that is loosely tied to an existing RemoteFrame but is not really attached to the FrameTree. The LocalFrame will begin loading, and when it commits, it will be swapped into where the RemotFrame was. This adds sufficient logic to actually support that swap (and to maintain the intermediate semi-attached state while the LocalFrame is provisional).

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

[Nate Chapin, 2014-10-15 19:30:14]

japhet@chromium.org changed reviewers:
+ dcheng@chromium.org

[Nate Chapin, 2014-10-15 19:30:15]

https://codereview.chromium.org/600553003 has my current theory on what the
chromium-side will look like, but I haven't attempted to polish it.

[dcheng, 2014-10-15 21:14:35]

This is complicated. If I understand correctly, the general flow is:

- content might create a WebLocalFrame for provisional navigations.
- It initializes it using initializeToReplaceRemoteFrame().
- If the navigation commits, we then swap() it in?

I thought we were working towards a state where we know for sure that a
navigation will commit. Would it be possible to defer creating a WebLocalFrame
until then to eliminate a lot of the complexity?

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1810: m_frame =
LocalFrame::create(&m_frameLoaderClientImpl, oldFrame->host(), new
PlaceholderFrameOwner());
Is PlaceholderFrameOwner the right one? Do we not bother enforcing any sandbox
flags, etc while the load is in progress?

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

https://codereview.chromium.org/643333002/diff/290001/public/web/WebFrame.h#n...
public/web/WebFrame.h:211: void setParent(WebFrame*);
Nit: maybe call it setParentForProvisionalNavigation() or something? Also, this
doesn't need to be publicly exposed, right? It looks like only Blink calls it.

[Nate Chapin, 2014-10-15 21:27:31]

My understanding based on a conversation with nasko a week or two ago was that
waiting to create the frame until commit was dependent on PlzNavigate. I haven't
been following PlzNavigate progress too closely, so I don't know how long we'd
have to wait in order to depend on it. I don't like this intermediate state, and
I'm more than happy to discuss alternatives to it if they are viable :)

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1810: m_frame =
LocalFrame::create(&m_frameLoaderClientImpl, oldFrame->host(), new
PlaceholderFrameOwner());
On 2014/10/15 21:14:35, dcheng wrote:
> Is PlaceholderFrameOwner the right one? Do we not bother enforcing any sandbox
> flags, etc while the load is in progress?

I don't think we enforce them in the provisional state of the first real load,
but I'm not certian. Will look.

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

https://codereview.chromium.org/643333002/diff/290001/public/web/WebFrame.h#n...
public/web/WebFrame.h:211: void setParent(WebFrame*);
On 2014/10/15 21:14:35, dcheng wrote:
> Nit: maybe call it setParentForProvisionalNavigation() or something? Also,
this
> doesn't need to be publicly exposed, right? It looks like only Blink calls it.

I only did it this way because m_parent is private, I guess I could make it
protected?

[dcheng, 2014-10-16 00:39:07]

I talked to nasko@ and he estimates the PlzNavigate stuff won't come any sooner
than 2 quarters from now. Oh well.

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

https://codereview.chromium.org/643333002/diff/290001/public/web/WebFrame.h#n...
public/web/WebFrame.h:211: void setParent(WebFrame*);
On 2014/10/15 21:27:31, Nate Chapin wrote:
> On 2014/10/15 21:14:35, dcheng wrote:
> > Nit: maybe call it setParentForProvisionalNavigation() or something? Also,
> this
> > doesn't need to be publicly exposed, right? It looks like only Blink calls
it.
> 
> I only did it this way because m_parent is private, I guess I could make it
> protected?

Yeah, I think making it protected would be better.

[Nate Chapin, 2014-10-16 21:45:56]

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
File Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/643333002/diff/290001/Source/web/WebLocalFram...
Source/web/WebLocalFrameImpl.cpp:1810: m_frame =
LocalFrame::create(&m_frameLoaderClientImpl, oldFrame->host(), new
PlaceholderFrameOwner());
On 2014/10/15 21:27:31, Nate Chapin wrote:
> On 2014/10/15 21:14:35, dcheng wrote:
> > Is PlaceholderFrameOwner the right one? Do we not bother enforcing any
sandbox
> > flags, etc while the load is in progress?
> 
> I don't think we enforce them in the provisional state of the first real load,
> but I'm not certian. Will look.

This function now creates the LocalFrame with a PlaceholderFrameOwner, then
immediately replaces it with the actual owner. This is because we can't create
the LocalFrame with a null FrameOwner (which would cause this to be the main
frame), nor the real frame owner (which would cause this to become the frame
owner's contentFrame() and perform the swap too soon).

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

https://codereview.chromium.org/643333002/diff/290001/public/web/WebFrame.h#n...
public/web/WebFrame.h:211: void setParent(WebFrame*);
On 2014/10/16 00:39:07, dcheng wrote:
> On 2014/10/15 21:27:31, Nate Chapin wrote:
> > On 2014/10/15 21:14:35, dcheng wrote:
> > > Nit: maybe call it setParentForProvisionalNavigation() or something? Also,
> > this
> > > doesn't need to be publicly exposed, right? It looks like only Blink calls
> it.
> > 
> > I only did it this way because m_parent is private, I guess I could make it
> > protected?
> 
> Yeah, I think making it protected would be better.

Done.

[dcheng, 2014-10-16 22:17:40]

The changes LGTM.

Can we file a bug to cleanup the unnecessary bits once we have PlzNavigate (and
maybe mark it as blocked on PlzNavigate?)

[Nate Chapin, 2014-10-16 22:18:39]

On 2014/10/16 22:17:40, dcheng wrote:
> The changes LGTM.
> 
> Can we file a bug to cleanup the unnecessary bits once we have PlzNavigate
(and
> maybe mark it as blocked on PlzNavigate?)

Will do.

[Nate Chapin, 2014-10-16 22:19:42]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-10-16 22:20:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643333002/330001

[commit-bot: I haz the power, 2014-10-17 00:21:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-17 00:21:21]

Exceeded time limit waiting for builds to trigger.

[Nate Chapin, 2014-10-17 17:40:42]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-10-17 17:41:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/643333002/330001

[commit-bot: I haz the power, 2014-10-17 17:42:37]

Committed patchset #7 (id:330001) as 183908