Skip to content

Commit

Permalink
Remove GetMainRenderFrame from RenderView.
Browse files Browse the repository at this point in the history
The MainFrame can be accessed off of WebView's API so there is no
need to store it in RenderViewImpl. Provide a convenience method on
RenderFrame to get the MainRenderFrame from the WebView.

BUG=1155202

Change-Id: Ibfab7e7a8285f6e275b4b3fd96a06e3c242216cc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2949671
Reviewed-by: Avi Drissman <avi@chromium.org>
Owners-Override: Avi Drissman <avi@chromium.org>
Commit-Queue: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891654}
  • Loading branch information
dtapuska authored and Chromium LUCI CQ committed Jun 11, 2021
1 parent a6e9891 commit a3f2d52
Show file tree
Hide file tree
Showing 19 changed files with 55 additions and 70 deletions.
9 changes: 3 additions & 6 deletions android_webview/renderer/aw_content_renderer_client.cc
Expand Up @@ -151,14 +151,11 @@ void AwContentRendererClient::RenderFrameCreated(
new js_injection::JsCommunication(render_frame);
new AwSafeBrowsingErrorPageControllerDelegateImpl(render_frame);

// TODO(jam): when the frame tree moves into content and parent() works at
// RenderFrame construction, simplify this by just checking parent().
content::RenderFrame* parent_frame =
render_frame->GetRenderView()->GetMainRenderFrame();
if (parent_frame && parent_frame != render_frame) {
content::RenderFrame* main_frame = render_frame->GetMainRenderFrame();
if (main_frame && main_frame != render_frame) {
// Avoid any race conditions from having the browser's UI thread tell the IO
// thread that a subframe was created.
GetRenderMessageFilter()->SubFrameCreated(parent_frame->GetRoutingID(),
GetRenderMessageFilter()->SubFrameCreated(main_frame->GetRoutingID(),
render_frame->GetRoutingID());
}

Expand Down
3 changes: 1 addition & 2 deletions chrome/renderer/chrome_content_renderer_client.cc
Expand Up @@ -119,7 +119,6 @@
#include "content/public/common/webplugininfo.h"
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_frame_visitor.h"
#include "content/public/renderer/render_view.h"
#include "extensions/buildflags/buildflags.h"
#include "ipc/ipc_sync_channel.h"
#include "media/base/media_switches.h"
Expand Down Expand Up @@ -579,7 +578,7 @@ void ChromeContentRendererClient::RenderFrameCreated(
if (!render_frame->IsMainFrame()) {
auto* main_frame_no_state_prefetch_helper =
prerender::NoStatePrefetchHelper::Get(
render_frame->GetRenderView()->GetMainRenderFrame());
render_frame->GetMainRenderFrame());
if (main_frame_no_state_prefetch_helper) {
// Avoid any race conditions from having the browser tell subframes that
// they're no-state prefetching.
Expand Down
4 changes: 1 addition & 3 deletions chrome/renderer/chrome_content_settings_agent_delegate.cc
Expand Up @@ -7,7 +7,6 @@
#include "base/containers/contains.h"
#include "chrome/common/ssl_insecure_content.h"
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_view.h"
#include "third_party/blink/public/web/web_local_frame.h"

#if BUILDFLAG(ENABLE_EXTENSIONS)
Expand All @@ -25,8 +24,7 @@ ChromeContentSettingsAgentDelegate::ChromeContentSettingsAgentDelegate(
RenderFrameObserverTracker<ChromeContentSettingsAgentDelegate>(
render_frame),
render_frame_(render_frame) {
content::RenderFrame* main_frame =
render_frame->GetRenderView()->GetMainRenderFrame();
content::RenderFrame* main_frame = render_frame->GetMainRenderFrame();
// TODO(nasko): The main frame is not guaranteed to be in the same process
// with this frame with --site-per-process. This code needs to be updated
// to handle this case. See https://crbug.com/496670.
Expand Down
Expand Up @@ -20,7 +20,6 @@
#include "content/public/common/url_constants.h"
#include "content/public/renderer/document_state.h"
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_view.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
#include "third_party/blink/public/common/browser_interface_broker_proxy.h"
Expand Down Expand Up @@ -112,8 +111,7 @@ ContentSettingsAgentImpl::ContentSettingsAgentImpl(
&ContentSettingsAgentImpl::OnContentSettingsAgentRequest,
base::Unretained(this)));

content::RenderFrame* main_frame =
render_frame->GetRenderView()->GetMainRenderFrame();
content::RenderFrame* main_frame = render_frame->GetMainRenderFrame();
// TODO(nasko): The main frame is not guaranteed to be in the same process
// with this frame with --site-per-process. This code needs to be updated
// to handle this case. See https://crbug.com/496670.
Expand Down
Expand Up @@ -32,10 +32,10 @@ std::unique_ptr<blink::URLLoaderThrottle>
NoStatePrefetchHelper::MaybeCreateThrottle(int render_frame_id) {
content::RenderFrame* render_frame =
content::RenderFrame::FromRoutingID(render_frame_id);
auto* helper = render_frame
? NoStatePrefetchHelper::Get(
render_frame->GetRenderView()->GetMainRenderFrame())
: nullptr;
auto* helper =
render_frame
? NoStatePrefetchHelper::Get(render_frame->GetMainRenderFrame())
: nullptr;
if (!helper)
return nullptr;

Expand Down
5 changes: 5 additions & 0 deletions content/public/renderer/render_frame.h
Expand Up @@ -140,6 +140,11 @@ class CONTENT_EXPORT RenderFrame : public IPC::Listener,
// Returns the RenderView associated with this frame.
virtual RenderView* GetRenderView() = 0;

// Returns the RenderFrame associated with the main frame of the WebView.
// See `blink::WebView::MainFrame()`. Note that this will be null when
// the main frame in this process is a remote frame.
virtual RenderFrame* GetMainRenderFrame() = 0;

// Return the RenderAccessibility associated with this frame.
virtual RenderAccessibility* GetRenderAccessibility() = 0;

Expand Down
3 changes: 0 additions & 3 deletions content/public/renderer/render_view.h
Expand Up @@ -43,9 +43,6 @@ class CONTENT_EXPORT RenderView {
// been closed but not yet destroyed are excluded).
static void ForEach(RenderViewVisitor* visitor);

// Returns the main RenderFrame.
virtual RenderFrame* GetMainRenderFrame() = 0;

// Get the routing ID of the view.
virtual int GetRoutingID() = 0;

Expand Down
3 changes: 2 additions & 1 deletion content/public/test/render_view_test.cc
Expand Up @@ -533,7 +533,8 @@ void RenderViewTest::SetUp() {
*agent_scheduling_group_, std::move(view_params),
/*was_created_by_renderer=*/false, base::ThreadTaskRunnerHandle::Get());

RenderFrameWasShownWaiter waiter(view->GetMainRenderFrame());
RenderFrameWasShownWaiter waiter(RenderFrame::FromWebFrame(
view->GetWebView()->MainFrame()->ToWebLocalFrame()));
render_widget_host_->widget_remote_for_testing()->WasShown(
{} /* record_tab_switch_time_request */, false /* was_evicted=*/,
blink::mojom::RecordContentToVisibleTimeRequestPtr());
Expand Down
7 changes: 3 additions & 4 deletions content/renderer/pepper/mock_renderer_ppapi_host.cc
Expand Up @@ -11,16 +11,15 @@
namespace content {

MockRendererPpapiHost::MockRendererPpapiHost(RenderView* render_view,
RenderFrame* render_frame,
PP_Instance instance)
: sink_(),
ppapi_host_(&sink_, ppapi::PpapiPermissions()),
render_view_(render_view),
render_frame_(render_frame),
pp_instance_(instance),
has_user_gesture_(false),
plugin_instance_(new FakePepperPluginInstance) {
if (render_view)
render_frame_ = render_view->GetMainRenderFrame();
}
plugin_instance_(new FakePepperPluginInstance) {}

MockRendererPpapiHost::~MockRendererPpapiHost() {}

Expand Down
8 changes: 5 additions & 3 deletions content/renderer/pepper/mock_renderer_ppapi_host.h
Expand Up @@ -20,9 +20,11 @@ class FakePepperPluginInstance;
// resources through this will get added to the test sink.
class MockRendererPpapiHost : public RendererPpapiHost {
public:
// This function takes the RenderView and instance that the mock resource
// host will be associated with.
MockRendererPpapiHost(RenderView* render_view, PP_Instance instance);
// This function takes the RenderView, RenderFrame and instance that the mock
// resource host will be associated with.
MockRendererPpapiHost(RenderView* render_view,
RenderFrame* render_frame,
PP_Instance instance);
~MockRendererPpapiHost() override;

ppapi::proxy::ResourceMessageTestSink& sink() { return sink_; }
Expand Down
4 changes: 2 additions & 2 deletions content/renderer/pepper/pepper_file_chooser_host_unittest.cc
Expand Up @@ -142,7 +142,7 @@ class PepperFileChooserHostTest : public RenderViewTest {
TEST_F(PepperFileChooserHostTest, Show) {
PP_Resource pp_resource = 123;

MockRendererPpapiHost host(view_, pp_instance());
MockRendererPpapiHost host(view_, GetMainRenderFrame(), pp_instance());
PepperFileChooserHost chooser(&host, pp_instance(), pp_resource);

// Say there's a user gesture.
Expand Down Expand Up @@ -199,7 +199,7 @@ TEST_F(PepperFileChooserHostTest, Show) {
TEST_F(PepperFileChooserHostTest, NoUserGesture) {
PP_Resource pp_resource = 123;

MockRendererPpapiHost host(view_, pp_instance());
MockRendererPpapiHost host(view_, GetMainRenderFrame(), pp_instance());
PepperFileChooserHost chooser(&host, pp_instance(), pp_resource);

// Say there's no user gesture.
Expand Down
Expand Up @@ -29,7 +29,7 @@ class PepperGraphics2DHostTest : public testing::Test {
return PepperGraphics2DHost::ConvertToLogicalPixels(scale, op_rect, delta);
}

PepperGraphics2DHostTest() : renderer_ppapi_host_(nullptr, 12345) {}
PepperGraphics2DHostTest() : renderer_ppapi_host_(nullptr, nullptr, 12345) {}

~PepperGraphics2DHostTest() override {
ppapi::ProxyAutoLock proxy_lock;
Expand Down
4 changes: 1 addition & 3 deletions content/renderer/pepper/pepper_plugin_instance_impl.cc
Expand Up @@ -2370,9 +2370,7 @@ void PepperPluginInstanceImpl::SetPluginToHandleFindRequests(
PP_Instance instance) {
if (!LoadFindInterface())
return;
bool is_main_frame =
render_frame_ &&
render_frame_->GetRenderView()->GetMainRenderFrame() == render_frame_;
bool is_main_frame = render_frame_ && render_frame_->IsMainFrame();
if (!is_main_frame)
return;
container_->UsePluginAsFindHandler();
Expand Down
29 changes: 9 additions & 20 deletions content/renderer/render_frame_impl.cc
Expand Up @@ -1874,15 +1874,6 @@ RenderFrameImpl::~RenderFrameImpl() {
web_media_stream_device_observer_.reset();

base::trace_event::TraceLog::GetInstance()->RemoveProcessLabel(routing_id_);

if (is_main_frame_) {
// Ensure the RenderView doesn't point to this object, once it is destroyed.
// TODO(nasko): Add a check that the |main_render_frame_| of |render_view_|
// is |this|, once the object is no longer leaked.
// See https://crbug.com/464764.
render_view_->main_render_frame_ = nullptr;
}

g_routing_id_frame_map.Get().erase(routing_id_);
agent_scheduling_group_.RemoveRoute(routing_id_);
}
Expand Down Expand Up @@ -2121,9 +2112,7 @@ void RenderFrameImpl::Unload(
// TODO(creis): WebFrame::swap() can return false. Most of those cases
// should be due to the frame being detached during unload (in which case
// the necessary cleanup has happened anyway), but it might be possible for
// it to return false without detaching. Catch any cases that the
// RenderView's main_render_frame_ isn't cleared below (whether swap returns
// false or not).
// it to return false without detaching.
//
// This executes the unload handlers on this frame and its local descendants.
bool success = frame_->Swap(proxy->web_frame());
Expand All @@ -2134,10 +2123,6 @@ void RenderFrameImpl::Unload(
// Main frames should always swap successfully because there is no parent
// frame to cause them to become detached.
DCHECK(success);
// For main frames, the swap should have cleared the RenderView's pointer to
// this frame.
CHECK(!render_view->main_render_frame_);

// The RenderFrameProxy being swapped in here has now been attached to the
// Page as its main frame and properly initialized by the WebFrame::Swap()
// call, so we can call WebView's DidAttachRemoteMainFrame().
Expand Down Expand Up @@ -2288,6 +2273,14 @@ RenderView* RenderFrameImpl::GetRenderView() {
return render_view_;
}

RenderFrame* RenderFrameImpl::GetMainRenderFrame() {
WebFrame* main_frame = GetWebView()->MainFrame();
DCHECK(main_frame);
if (!main_frame->IsWebLocalFrame())
return nullptr;
return RenderFrame::FromWebFrame(main_frame->ToWebLocalFrame());
}

RenderAccessibility* RenderFrameImpl::GetRenderAccessibility() {
return render_accessibility_manager_->GetRenderAccessibilityImpl();
}
Expand Down Expand Up @@ -4853,10 +4846,6 @@ bool RenderFrameImpl::SwapIn(WebFrame* previous_web_frame) {
// If this is the main frame going from a remote frame to a local frame,
// it needs to set RenderViewImpl's pointer for the main frame to itself.
if (is_main_frame_) {
// TODO(https://crubg.com/936696): Implement RenderDocument on main frames.
CHECK(!render_view_->main_render_frame_);
render_view_->main_render_frame_ = this;

// The WebFrame being swapped in here has now been attached to the Page as
// its main frame, and the WebFrameWidget was previously initialized when
// the frame was created so we can call WebView's DidAttachLocalMainFrame().
Expand Down
1 change: 1 addition & 0 deletions content/renderer/render_frame_impl.h
Expand Up @@ -334,6 +334,7 @@ class CONTENT_EXPORT RenderFrameImpl

// RenderFrame implementation:
RenderView* GetRenderView() override;
RenderFrame* GetMainRenderFrame() override;
RenderAccessibility* GetRenderAccessibility() override;
std::unique_ptr<AXTreeSnapshotter> CreateAXTreeSnapshotter(
ui::AXMode ax_mode) override;
Expand Down
12 changes: 9 additions & 3 deletions content/renderer/render_view_browsertest_mac.mm
Expand Up @@ -7,6 +7,7 @@
#include "base/strings/utf_string_conversions.h"
#include "content/public/browser/native_web_keyboard_event.h"
#include "content/public/test/render_view_test.h"
#include "content/renderer/render_frame_impl.h"
#include "content/renderer/render_view_impl.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/blink/public/common/web_preferences/web_preferences.h"
Expand Down Expand Up @@ -77,8 +78,11 @@
prefs.enable_scroll_animator = false;

RenderViewImpl* view = static_cast<RenderViewImpl*>(view_);
blink::WebFrameWidget* blink_widget =
view->GetMainRenderFrame()->GetLocalRootWebFrameWidget();
blink::WebFrameWidget* blink_widget = view->GetWebView()
->MainFrame()
->ToWebLocalFrame()
->LocalRoot()
->FrameWidget();

view->GetWebView()->SetWebPreferences(prefs);

Expand All @@ -89,7 +93,9 @@
NSEvent* arrowUpKeyDown = CmdDeadKeyEvent(NSKeyDown, kVK_UpArrow);

// First test when javascript does not eat keypresses -- should scroll.
view->GetMainRenderFrame()->set_send_content_state_immediately(true);
RenderFrameImpl::FromWebFrame(
view->GetWebView()->MainFrame()->ToWebLocalFrame())
->set_send_content_state_immediately(true);
LoadHTML(kRawHtml);
render_thread_->sink().ClearMessages();

Expand Down
13 changes: 6 additions & 7 deletions content/renderer/render_view_impl.cc
Expand Up @@ -130,7 +130,7 @@ void RenderViewImpl::Initialize(
webview_->SetWebPreferences(params->web_preferences);

if (local_main_frame) {
main_render_frame_ = RenderFrameImpl::CreateMainFrame(
RenderFrameImpl::CreateMainFrame(
agent_scheduling_group_, this, opener_frame,
params->type != mojom::ViewWidgetType::kTopLevel,
std::move(params->replication_state), params->devtools_main_frame_token,
Expand Down Expand Up @@ -372,8 +372,11 @@ WebView* RenderViewImpl::CreateView(
creator->GetTaskRunner(blink::TaskType::kInternalDefault));

if (reply->wait_for_debugger) {
blink::WebFrameWidget* frame_widget =
view->GetMainRenderFrame()->GetLocalRootWebFrameWidget();
blink::WebFrameWidget* frame_widget = view->GetWebView()
->MainFrame()
->ToWebLocalFrame()
->LocalRoot()
->FrameWidget();
frame_widget->WaitForDebuggerWhenShown();
}

Expand All @@ -382,10 +385,6 @@ WebView* RenderViewImpl::CreateView(

// RenderView implementation ---------------------------------------------------

RenderFrameImpl* RenderViewImpl::GetMainRenderFrame() {
return main_render_frame_;
}

int RenderViewImpl::GetRoutingID() {
return routing_id_;
}
Expand Down
3 changes: 0 additions & 3 deletions content/renderer/render_view_impl.h
Expand Up @@ -117,7 +117,6 @@ class CONTENT_EXPORT RenderViewImpl : public blink::WebViewClient,

// RenderView implementation -------------------------------------------------

RenderFrameImpl* GetMainRenderFrame() override;
int GetRoutingID() override;
blink::WebView* GetWebView() override;

Expand Down Expand Up @@ -185,8 +184,6 @@ class CONTENT_EXPORT RenderViewImpl : public blink::WebViewClient,
// The `AgentSchedulingGroup` this view is associated with.
AgentSchedulingGroup& agent_scheduling_group_;

RenderFrameImpl* main_render_frame_ = nullptr;

#if defined(OS_ANDROID)
// Android Specific ----------------------------------------------------------

Expand Down
3 changes: 1 addition & 2 deletions weblayer/renderer/content_renderer_client_impl.cc
Expand Up @@ -27,7 +27,6 @@
#include "components/webapps/renderer/web_page_metadata_agent.h"
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_thread.h"
#include "content/public/renderer/render_view.h"
#include "third_party/blink/public/platform/platform.h"
#include "third_party/blink/public/platform/web_runtime_features.h"
#include "ui/base/resource/resource_bundle.h"
Expand Down Expand Up @@ -154,7 +153,7 @@ void ContentRendererClientImpl::RenderFrameCreated(
if (!render_frame->IsMainFrame()) {
auto* main_frame_no_state_prefetch_helper =
prerender::NoStatePrefetchHelper::Get(
render_frame->GetRenderView()->GetMainRenderFrame());
render_frame->GetMainRenderFrame());
if (main_frame_no_state_prefetch_helper) {
// Avoid any race conditions from having the browser tell subframes that
// they're no-state prefetching.
Expand Down

0 comments on commit a3f2d52

Please sign in to comment.