Allow RemoteFrames to be navigated through targeted navigation.

Allow RemoteFrames to be navigated through targeted navigation.

Currently, FrameLoader assumes the target frame of a navigation is always
LocalFrame. This is not the case once we have out-of-process iframes.
This patch allows navigation to be sent to RemoteFrame if the frame is in
a different process.

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

[nasko, 2015-06-05 23:30:43]

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

[nasko, 2015-06-05 23:30:44]

Hey Nate,
Can you review this CL for me? We have test coverage for that in chromium
content_browsertests, but ideas on how to test this in blink itself will be
great!
Thanks in advance!
Nasko

[Nate Chapin, 2015-06-05 23:34:47]

https://codereview.chromium.org/1152053006/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1152053006/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoader.cpp:788:
toLocalFrame(targetFrame.get())->loader().load(request);
This feels like the wrong place for this logic. Could we have a function on
Frame that can be called here and implemented by LocalFrame/RemoteFrame? Maybe a
variant of navigate() that takes a FrameLoadRequest?

[nasko, 2015-06-08 15:57:10]

https://codereview.chromium.org/1152053006/diff/1/Source/core/loader/FrameLoa...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1152053006/diff/1/Source/core/loader/FrameLoa...
Source/core/loader/FrameLoader.cpp:788:
toLocalFrame(targetFrame.get())->loader().load(request);
On 2015/06/05 23:34:47, Nate Chapin wrote:
> This feels like the wrong place for this logic. Could we have a function on
> Frame that can be called here and implemented by LocalFrame/RemoteFrame? Maybe
a
> variant of navigate() that takes a FrameLoadRequest?

Done.

[Nate Chapin, 2015-06-08 16:56:23]

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:256: // TODO(dcheng): Special case for
window.open("about:blank") to ensure it loads synchronously into
I don't love the duplication here. Perhaps we should specify that this version
of navigate() assumes you don't need to start the navigation on a timer, and
just call m_loader.load() unconditionally? We don't need a timer for the sole
callsite you're adding, and we've long had this distinction in blink (i.e.,
things that call FrameLoader directly vs. things that call NavigationScheduler
to start a load).

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:77: navigate(*passedRequest.originDocument(),
passedRequest.resourceRequest().url(), passedRequest. lockBackForwardList(),
gesture);
Spurious whitespace after "passedRequest."

https://codereview.chromium.org/1152053006/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:58: #include "core/frame/RemoteFrame.h"
Remove this?

[nasko, 2015-06-08 17:27:04]

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:256: // TODO(dcheng): Special case for
window.open("about:blank") to ensure it loads synchronously into
On 2015/06/08 16:56:22, Nate Chapin wrote:
> I don't love the duplication here. Perhaps we should specify that this version
> of navigate() assumes you don't need to start the navigation on a timer, and
> just call m_loader.load() unconditionally? We don't need a timer for the sole
> callsite you're adding, and we've long had this distinction in blink (i.e.,
> things that call FrameLoader directly vs. things that call NavigationScheduler
> to start a load).

Yeah, not a big fan either. I've modified it as suggested.

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/frame/Remot...
Source/core/frame/RemoteFrame.cpp:77: navigate(*passedRequest.originDocument(),
passedRequest.resourceRequest().url(), passedRequest. lockBackForwardList(),
gesture);
On 2015/06/08 16:56:22, Nate Chapin wrote:
> Spurious whitespace after "passedRequest."

Done.

https://codereview.chromium.org/1152053006/diff/20001/Source/core/loader/Fram...
File Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1152053006/diff/20001/Source/core/loader/Fram...
Source/core/loader/FrameLoader.cpp:58: #include "core/frame/RemoteFrame.h"
On 2015/06/08 16:56:22, Nate Chapin wrote:
> Remove this?

Done.

[Nate Chapin, 2015-06-08 17:28:32]

lgtm

[nasko, 2015-06-08 18:27:42]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-06-08 18:28:29]

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

[commit-bot: I haz the power, 2015-06-08 18:46:18]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196689