New issue
Advanced search Search tips

Issue 727166 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Move frame tree back into core

Project Member Reported by dcheng@chromium.org, May 28 2017

Issue description

It's a bit awkward that it's maintained in web right now: it means we need extra plumbing for a core frame to figure out its parent frame, etc, we need to expose the frame tree to the embedder in a very direct way, maintaining openers needs to be at the web layer, along with the awkward tracing involved (since WebFrame itself inherit from GarbageCollected).

We can simplify this by:
1. Removing AddChild()/RemoveChild() etc from the public API and exposing a WebLocalFrame::CreateChild to match WebRemoteFrame::CreateLocalChild and WebRemoteFrame::CreateRemoteChild.

2. Change frame detach to automatically remove the frame from the frame tree rather than relying on the embedder to do it in WebFrameClient::FrameDetached().

3. Construct blink::Frame at the same time as blink::WebFrame. This will require changing the way we set the main frame: instead of doing it in the Frame constructor, we can have WebView::SetMainFrame() set the main frame, while the various Web*Frame::Create*Child methods will attach the frame to the frame owner.

4. Move the frame tree itself into blink::Frame and remove the plumbing in the Web layer.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01f99abb0f0a8a68fcf01527068d481c16bd70c0

commit 01f99abb0f0a8a68fcf01527068d481c16bd70c0
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon May 29 21:16:26 2017

Remove frame tree management functions from WebFrame public API.

This was originally done in Blink: however, since the frame tree wasn't
updated until WebFrameClient::CreateChildFrame returned, RenderFrameImpl
initialization was tricky since code that just used the WebLocalFrame
directly wouldn't know if the frame was a child frame or not without
plumbing around extra arguments.

To fix this, Blink exposed functions to manage the frame tree to the
embedder. However, this started resulting in a lot of boilerplate code
copied between various WebFrameClient implementations--as well as the
possibility that the embedder could unsync the frame tree from Blink's
internal state.

Bug: 727166
Change-Id: I653354d48232c8f712b3c4e864f0c9783ef63ae7
Reviewed-on: https://chromium-review.googlesource.com/506500
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#475381}
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/content/shell/test_runner/web_frame_test_client.cc
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/LocalFrameClientImpl.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/RemoteFrameClientImpl.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/WebFrame.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/tests/FrameTestHelpers.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/public/web/WebFrame.h
[modify] https://crrev.com/01f99abb0f0a8a68fcf01527068d481c16bd70c0/third_party/WebKit/public/web/WebLocalFrame.h

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3d56220ed9fd697185c345f3047507164ce09b46

commit 3d56220ed9fd697185c345f3047507164ce09b46
Author: Daniel Cheng <dcheng@chromium.org>
Date: Thu Jun 15 16:07:41 2017

Make WebLocalFrame 1:1 with TestWebFrameClient

Several helpers are added to make it easier to write
test code:
- TestWebFrameClient can optionally delete itself on
  frame detach.
- Adds helpers for creating new local frames, so test
  code doesn't have to specify uninteresting arguments.
- Adds LocalMainFrame() and RemoteMainFrame() helpers
  to WebViewHelper to make it easier to get the main
  frame in the appropriate form.

Followup CLs will make remote frames use a similar
pattern, with the eventual goal of having methods to
create main frames and addressing some longstanding
TODOs.

Bug:  361765 , 727166
Change-Id: Ic99508f18336e51bef9fee000c5e2cf919e94c4c
Reviewed-on: https://chromium-review.googlesource.com/521751
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Sasha Morrissey <sashab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479727}
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/DocumentLoaderTest.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/LocalFrameClientImplTest.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp
[modify] https://crrev.com/3d56220ed9fd697185c345f3047507164ce09b46/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65400da28d6fc9221acdd2fffbbf4f459435e44e

commit 65400da28d6fc9221acdd2fffbbf4f459435e44e
Author: Daniel Cheng <dcheng@chromium.org>
Date: Sun Jun 18 23:28:59 2017

Improve test helpers for remote frames.

The test framework now automatically creates and manages
the lifetime of WebRemoteFrameClient and WebWidgetClient
internally when no subclassing is required. Test helpers
for initializing a remote main frame have also been
added.

Bug: 727166
Change-Id: I3f5936c948aa4b712d693d0d0105b388653bf7f8
Reviewed-on: https://chromium-review.googlesource.com/538143
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480322}
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/core/frame/FrameTestHelpers.h
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/web/tests/DocumentLoaderTest.cpp
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/web/tests/WebHelperPluginTest.cpp
[modify] https://crrev.com/65400da28d6fc9221acdd2fffbbf4f459435e44e/third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b81391a1463e01627551df0c4520581aa3118c4e

commit b81391a1463e01627551df0c4520581aa3118c4e
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Jun 19 03:46:44 2017

FrameTestHelpers: eliminate explicit clients that are redundant

There's usually no need to explicitly create a TestWebFrameClient,
TestWebRemoteFrameClient, TestWebWidgetClient, or TestWebViewClient: the
test framework can handle creation of these internally. It's only when a
test needs to extend the behavior that it's useful to create an explicit
client; otherwise, it just makes the test more complex for no benefit..

Bug: 727166
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ia31996abd43c19edc752cc4a42d636b1a81a09a7
Reviewed-on: https://chromium-review.googlesource.com/538381
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480339}
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/core/paint/LinkHighlightImplTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/ExternalPopupMenuTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/CompositorWorkerTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/LayoutGeometryMapTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/ProgrammaticScrollTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/ScrollingCoordinatorTest.cpp
[modify] https://crrev.com/b81391a1463e01627551df0c4520581aa3118c4e/third_party/WebKit/Source/web/tests/WebFrameTest.cpp

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bd3794204f2267a5a16f95912f405ae8360d24bc

commit bd3794204f2267a5a16f95912f405ae8360d24bc
Author: Daniel Cheng <dcheng@chromium.org>
Date: Wed Jun 21 19:00:28 2017

Add WebFrameClient::BindToFrame() to fix RenderFrameImpl association.

WebLocalFrame initialization fires a few WebFrameclient callbacks before
WebLocalFrame creation has returned control to the caller. In that case,
the caller's WebFrameClient won't know its associated WebFrameClient
yet.

BindToFrame() is called after WebLocalFrame is constructed, but before
any other WebFrameClient callbacks are invoked, guaranteeing that the
two objects will be associated before any other callbacks.

Bug:  361765 , 727166
Change-Id: Ib23bd3fa3f73d9b1229a7d0394bc538f22a65861
Reviewed-on: https://chromium-review.googlesource.com/542655
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481263}
[modify] https://crrev.com/bd3794204f2267a5a16f95912f405ae8360d24bc/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/bd3794204f2267a5a16f95912f405ae8360d24bc/content/renderer/render_frame_impl.h
[modify] https://crrev.com/bd3794204f2267a5a16f95912f405ae8360d24bc/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/bd3794204f2267a5a16f95912f405ae8360d24bc/third_party/WebKit/public/web/WebFrameClient.h

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8bec1b8074a2e8c2cdfd530981fca8224ea3614

commit c8bec1b8074a2e8c2cdfd530981fca8224ea3614
Author: Daniel Cheng <dcheng@chromium.org>
Date: Fri Jun 23 07:59:47 2017

Move WebViewFrameWidget to the Oilpan heap.

A followup patch will move lifetime management of WebFrame /
WebFrameWidget into Blink. Using a common cleanup path will make things
simpler.

Bug: 727166
Change-Id: I2f0a583ec47542e2d355d93db1cf79d651b04d3f
Reviewed-on: https://chromium-review.googlesource.com/544585
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481825}
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/core/frame/WebFrameWidgetBase.h
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/core/frame/WebViewFrameWidget.cpp
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/core/frame/WebViewFrameWidget.h
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/web/WebFrameWidgetImpl.h
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/c8bec1b8074a2e8c2cdfd530981fca8224ea3614/third_party/WebKit/Source/web/WebLocalFrameImpl.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d5e56fff672298da16ffa16e6eeda1cbc61531e0

commit d5e56fff672298da16ffa16e6eeda1cbc61531e0
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Jun 26 07:37:13 2017

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.cronin@chromium.org,thestig@chromium.org,tommycli@chromium.org,watk@chromium.org

Bug: 727166
Change-Id: I1322e2757ef01a4210cf3668b24164f793a64061
Reviewed-on: https://chromium-review.googlesource.com/542658
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482211}
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/components/plugins/renderer/webview_plugin.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/components/printing/renderer/print_web_view_helper.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/media/android/media_info_loader_unittest.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_frame_impl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_view_browsertest.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_view_impl.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/renderer/render_view_impl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/shell/test_runner/web_view_test_client.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/shell/test_runner/web_view_test_client.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/content/shell/test_runner/web_view_test_proxy.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/extensions/renderer/scoped_web_frame.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/media/blink/multibuffer_data_source_unittest.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/media/blink/resource_multibuffer_data_provider_unittest.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebFactory.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebFrame.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebRemoteFrameImpl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/exported/WebViewTest.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/frame/FrameTestHelpers.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/loader/EmptyClients.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/page/ChromeClient.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/page/CreateWindow.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/testing/sim/SimWebViewClient.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/core/testing/sim/SimWebViewClient.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/ChromeClientImpl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebFactoryImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebFactoryImpl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebLocalFrameImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebLocalFrameImpl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/WebViewImpl.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/public/web/WebFrame.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/public/web/WebLocalFrame.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/public/web/WebRemoteFrame.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/public/web/WebView.h
[modify] https://crrev.com/d5e56fff672298da16ffa16e6eeda1cbc61531e0/third_party/WebKit/public/web/WebViewClient.h

Cc: -sashab@chromium.org

Sign in to add a comment