Add factory creation methods for creating local and remote main frames. [chromium/src : master]
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 1:Commit-Queue +1
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 2:Commit-Queue +1
mark a. foltz (Gerrit)
mark a. foltz removed mfoltz...@chromium.org from this change.
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 5:Commit-Queue +1
Daniel Cheng (Gerrit)
Daniel Cheng uploaded patch set #6 to this change.
Add factory creation methods for creating local and remote main frames.
- Creating a main frame no longer requires passing a pointless tree
scope type, since the main frame is never in the shadow tree.
- Consistently set name / sandbox flags in Blink via CreateMainFrame().
This makes the browser and renderer-driven main frame creation paths
more similar.
- Remove WebLocalFrame::Create() as it no longer needs to be exposed to
the embedder.
Bug: 727166
Change-Id: I1322e2757ef01a4210cf3668b24164f793a64061
---
M components/plugins/renderer/webview_plugin.cc
M components/printing/renderer/print_web_view_helper.cc
M content/renderer/media/android/media_info_loader_unittest.cc
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/renderer/render_frame_proxy.cc
M content/renderer/render_view_browsertest.cc
M content/renderer/render_view_impl.cc
M content/renderer/render_view_impl.h
M content/shell/test_runner/web_view_test_client.cc
M content/shell/test_runner/web_view_test_client.h
M content/shell/test_runner/web_view_test_proxy.h
M extensions/renderer/scoped_web_frame.cc
M media/blink/multibuffer_data_source_unittest.cc
M media/blink/resource_multibuffer_data_provider_unittest.cc
M media/blink/webmediaplayer_impl_unittest.cc
M third_party/WebKit/Source/core/exported/WebFactory.h
M third_party/WebKit/Source/core/exported/WebFrame.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.h
M third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
M third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/page/ChromeClient.h
M third_party/WebKit/Source/core/page/CreateWindow.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.h
M third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.h
M third_party/WebKit/Source/web/WebFactoryImpl.cpp
M third_party/WebKit/Source/web/WebFactoryImpl.h
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
M third_party/WebKit/Source/web/WebLocalFrameImpl.h
M third_party/WebKit/Source/web/WebViewImpl.cpp
M third_party/WebKit/Source/web/WebViewImpl.h
M third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
M third_party/WebKit/Source/web/tests/WebViewTest.cpp
M third_party/WebKit/public/web/WebFrame.h
M third_party/WebKit/public/web/WebLocalFrame.h
M third_party/WebKit/public/web/WebRemoteFrame.h
M third_party/WebKit/public/web/WebView.h
M third_party/WebKit/public/web/WebViewClient.h
44 files changed, 256 insertions(+), 197 deletions(-)
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 6:
+alexmos for content
+japhet for blink
This CL lets Blink manage more state internally, rather than exposing it all to the renderer. This makes it easier to refactor Blink-internal things in a followup CL.
(7 comments)
File content/renderer/render_frame_impl.cc:
// This conversion is a little sad, as this often comes from a
// WebString...We can't pass WebString in FrameReplicationState either. Meh. We could add another parameter, but it'd look weird...
File content/renderer/render_view_impl.cc:
I changed this to be an argument to CreateMainFrame(), so Blink uniformly sets this during frame creation now. This gets rid of the need to coordinate between CreateWindow.cpp and here.
Ditto.
Blink won't plumb up _blank anymore. Things like _BLANK probably had minor issues: Blink does a case-insensitive compare, while content didn't...
I /think/ this shouldn't be web-visible, as we force an empty unique name for the main frame anyway... and Blink internally treats _blank fairly consistently.
File third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp:
// It would be nice to DCHECK that the main frame is not set yet here.
// Unfortunately, there is an edge case with a pending RenderFrameHost that
// violates this: the embedder may create a pending RenderFrameHost for
// navigating to a new page in a popup. If the navigation ends up redirecting
// to a site that requires a process swap, it doesn't go through the standard
// swapping path and instead directly overwrites the main frame.
// TODO(dcheng): Remove the need for this and strongly enforce this condition
// with a DCHECK.This is the case we discussed yesterday with the pending RenderFrameHost.
File third_party/WebKit/public/web/WebLocalFrame.h:
// TODO(dcheng): The argument order should be more consistent with
// CreateLocalChild() and CreateRemoteChild() in WebRemoteFrame... but it's so
// painful...Maybe I'll try this in a future patch. I put the new parameters at the end, since callers outside content/renderer generally don't actually care about them...
File third_party/WebKit/public/web/WebRemoteFrame.h:
This argument isn't interesting for callers of WebRemoteFrame::Create(): this is creating a frame to be swapped in (rather than replicated) and will always take the opener from the frame being replaced.
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng would like Alex Moshchuk and Nate Chapin to review this change.
To view, visit change 542658. To unsubscribe, visit settings.
Alex Moshchuk (Gerrit)
Alex Moshchuk posted comments on this change.
Patch set 6:Code-Review +1
Very cool, content/ LGTM with some minor notes. Thanks for the helpful comments!
(8 comments)
I changed this to be an argument to CreateMainFrame(), so Blink uniformly s
Ack
Blink won't plumb up _blank anymore. Things like _BLANK probably had minor
Yes, it's good to remove that inconsistency, especially since the spec says the comparison should be case-insensitive. (https://html.spec.whatwg.org/multipage/browsers.html#browsing-context-name)
So, do I understand correctly that previously, _BLANK was sent to the browser process (and stored as the FTN name, etc.), but Blink would keep the new frame's name empty? On the other hand, trying this on stable, window.open("about:blank","_BLANK") ends up with the window.name being "_BLANK", which is surprising given the case-insensitive comparison to _blank in CreateWindow (that you're moving), so I don't think I completely understand how the name gets set in that case...
Patch Set #6, Line 637:
// TODO(dcheng): Shouldn't these be mutually exclusive at this point?It might be worth referencing 720116 here, the bug with the browser sending us both routing IDs, so we don't forget to make this mutually exclusive once that bug is fixed.
File third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp:
// It would be nice to DCHECK that the main frame is not set yet here.
// Unfortunately, there is an edge case with a pending RenderFrameHost that
// violates this: the embedder may create a pending RenderFrameHost for
// navigating to a new page in a popup. If the navigation ends up redirecting
// to a site that requires a process swap, it doesn't go through the standard
// swapping path and instead directly overwrites the main frame.
// TODO(dcheng): Remove the need for this and strongly enforce this condition
// with a DCHECK.This is the case we discussed yesterday with the pending RenderFrameHost.
Ack
File third_party/WebKit/Source/web/WebFactoryImpl.h:
Patch Set #6, Line 27:
InterfaceRegistry*) const override;Seems weird that this doesn't take an opener but the one below does. Is it also just because this arg isn't interesting for the current callers of this, or is there another reason we don't want the opener here?
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:
Patch Set #6, Line 1545:
// Maybe to make sure the security origin is initialized correctly?If a.com window.opens a new popup and then does a cross-process navigation to b.com, how else would we set the opener for the new main frame for b.com?
File third_party/WebKit/public/web/WebRemoteFrame.h:
This argument isn't interesting for callers of WebRemoteFrame::Create(): th
Ack
This argument isn't interesting for callers of WebRemoteFrame::Create(): th
Ack
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 6:
(4 comments)
Yes, it's good to remove that inconsistency, especially since the spec says
I don't see that locally. Maybe I'm missing something in the repro steps (I'm also trying this on dev channel).
I think where it comes in is for replicated frame names: I think the replicated named will show up as _blank (confusingly enough) rather than the empty string.
Patch Set #6, Line 637:
// TODO(dcheng): Shouldn't these be mutually exclusive at this point?It might be worth referencing 720116 here, the bug with the browser sending
Patch Set #6, Line 27:
InterfaceRegistry*) const override;Seems weird that this doesn't take an opener but the one below does. Is it
It's only used for the worker shadow page, which doesn't need this: thus I figured it's not worth the trouble plumbing it through.
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:
Patch Set #6, Line 1545:
// Maybe to make sure the security origin is initialized correctly?If a.com window.opens a new popup and then does a cross-process navigation
I guess what I mean is the caller could set it too. I'll delete this comment, since it's not very useful.
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 9:Commit-Queue +1
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 10:Commit-Queue +1
Daniel Cheng (Gerrit)
+haraken, mind taking a look at this CL?
Kentaro Hara (Gerrit)
Kentaro Hara posted comments on this change.
Patch set 10:Code-Review +1
WebKit LGTM
Daniel Cheng (Gerrit)
Daniel Cheng uploaded patch set #11 to this change.
Add factory creation methods for creating local and remote main frames.
- Creating a main frame no longer requires passing a pointless tree
scope type, since the main frame is never in the shadow tree.
- Consistently set name / sandbox flags in Blink via CreateMainFrame().
This makes the browser and renderer-driven main frame creation paths
more similar.
- Remove WebLocalFrame::Create() as it no longer needs to be exposed to
the embedder.
TBR=rdevlin...@chromium.org,the...@chromium.org,tomm...@chromium.org,wa...@chromium.org
Bug: 727166
Change-Id: I1322e2757ef01a4210cf3668b24164f793a64061
---
M components/plugins/renderer/webview_plugin.cc
M components/printing/renderer/print_web_view_helper.cc
M content/renderer/media/android/media_info_loader_unittest.cc
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/renderer/render_frame_proxy.cc
M content/renderer/render_view_browsertest.cc
M content/renderer/render_view_impl.cc
M content/renderer/render_view_impl.h
M content/shell/test_runner/web_view_test_client.cc
M content/shell/test_runner/web_view_test_client.h
M content/shell/test_runner/web_view_test_proxy.h
M extensions/renderer/scoped_web_frame.cc
M media/blink/multibuffer_data_source_unittest.cc
M media/blink/resource_multibuffer_data_provider_unittest.cc
M media/blink/webmediaplayer_impl_unittest.cc
M third_party/WebKit/Source/core/exported/WebFactory.h
M third_party/WebKit/Source/core/exported/WebFrame.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.h
M third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
M third_party/WebKit/Source/core/exported/WebViewTest.cpp
M third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/page/ChromeClient.h
M third_party/WebKit/Source/core/page/CreateWindow.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.h
M third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.h
M third_party/WebKit/Source/web/WebFactoryImpl.cpp
M third_party/WebKit/Source/web/WebFactoryImpl.h
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
M third_party/WebKit/Source/web/WebLocalFrameImpl.h
M third_party/WebKit/Source/web/WebViewImpl.cpp
M third_party/WebKit/Source/web/WebViewImpl.h
M third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
M third_party/WebKit/public/web/WebFrame.h
M third_party/WebKit/public/web/WebLocalFrame.h
M third_party/WebKit/public/web/WebRemoteFrame.h
M third_party/WebKit/public/web/WebView.h
M third_party/WebKit/public/web/WebViewClient.h
44 files changed, 256 insertions(+), 197 deletions(-)
To view, visit change 542658. To unsubscribe, visit settings.
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 11:Commit-Queue +2
TBRing trivial Blink API usage changes:
tommycli@ for //components/plugins
thestig@ for //components/printing
rdevlin.cronin@ for //extensions
watk@ for //media
Commit Bot (Gerrit)
Commit Bot posted comments on this change.
Patch set 11:
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Edit commit message" https://chromium-review.googlesource.com/c/542658/11
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/542658/11
Bot data: {"action": "start", "triggered_at": "2017-06-26T05:12:37.0Z", "cq_cfg_revision": "e12d437dc7f395d72995b548c9dacf21b0b1526e", "revision": "4975d190b8d1fba7d65d74306884a84988257b1c"}
Daniel Cheng (Gerrit)
Daniel Cheng posted comments on this change.
Patch set 11:
Oops. Really adding TBRs as reviewers this time:
TBRing trivial Blink API usage changes:
tommycli@ for //components/plugins
thestig@ for //components/printing
rdevlin.cronin@ for //extensions
watk@ for //media
To view, visit change 542658. To unsubscribe, visit settings.
Commit Bot (Gerrit)
Commit Bot merged this change.
Add factory creation methods for creating local and remote main frames.
- Creating a main frame no longer requires passing a pointless tree
scope type, since the main frame is never in the shadow tree.
- Consistently set name / sandbox flags in Blink via CreateMainFrame().
This makes the browser and renderer-driven main frame creation paths
more similar.
- Remove WebLocalFrame::Create() as it no longer needs to be exposed to
the embedder.
TBR=rdevlin...@chromium.org,the...@chromium.org,tomm...@chromium.org,wa...@chromium.org
Bug: 727166
Change-Id: I1322e2757ef01a4210cf3668b24164f793a64061
Reviewed-on: https://chromium-review.googlesource.com/542658
Commit-Queue: Daniel Cheng <dch...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Alex Moshchuk <ale...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482211}
---
M components/plugins/renderer/webview_plugin.cc
M components/printing/renderer/print_web_view_helper.cc
M content/renderer/media/android/media_info_loader_unittest.cc
M content/renderer/render_frame_impl.cc
M content/renderer/render_frame_impl.h
M content/renderer/render_frame_proxy.cc
M content/renderer/render_view_browsertest.cc
M content/renderer/render_view_impl.cc
M content/renderer/render_view_impl.h
M content/shell/test_runner/web_view_test_client.cc
M content/shell/test_runner/web_view_test_client.h
M content/shell/test_runner/web_view_test_proxy.h
M extensions/renderer/scoped_web_frame.cc
M media/blink/multibuffer_data_source_unittest.cc
M media/blink/resource_multibuffer_data_provider_unittest.cc
M media/blink/webmediaplayer_impl_unittest.cc
M third_party/WebKit/Source/core/exported/WebFactory.h
M third_party/WebKit/Source/core/exported/WebFrame.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
M third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.h
M third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
M third_party/WebKit/Source/core/exported/WebViewTest.cpp
M third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
M third_party/WebKit/Source/core/loader/EmptyClients.h
M third_party/WebKit/Source/core/page/ChromeClient.h
M third_party/WebKit/Source/core/page/CreateWindow.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.cpp
M third_party/WebKit/Source/core/testing/sim/SimWebViewClient.h
M third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.cpp
M third_party/WebKit/Source/web/ChromeClientImpl.h
M third_party/WebKit/Source/web/WebFactoryImpl.cpp
M third_party/WebKit/Source/web/WebFactoryImpl.h
M third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
M third_party/WebKit/Source/web/WebLocalFrameImpl.h
M third_party/WebKit/Source/web/WebViewImpl.cpp
M third_party/WebKit/Source/web/WebViewImpl.h
M third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp
M third_party/WebKit/public/web/WebFrame.h
M third_party/WebKit/public/web/WebLocalFrame.h
M third_party/WebKit/public/web/WebRemoteFrame.h
M third_party/WebKit/public/web/WebView.h
M third_party/WebKit/public/web/WebViewClient.h
44 files changed, 256 insertions(+), 197 deletions(-)
Lei Zhang (Gerrit)
Lei Zhang posted comments on this change.
Patch set 12:Code-Review +1