Skip to content
Permalink
Browse files

[m79] Remove code duplication for copying OpenURLParams into LoadURLP…

…arams.

There was quite a bit of code duplication related to copying
OpenURLParams into LoadURLParams.  Some callsites didn't copy all the
navigation properties (leading to the DCHECK reported in
https://crbug.com/1007041 and possibly also to some of renderer
kills reported in https://crbug.com/1014483).

This CL tries to improve the situation, by moving the copying code into
a new constructor of LoadURLParams and reusing the constructor from as
many places as possible.

TBR=lukasza@chromium.org

(cherry picked from commit 82a5ca9)

Bug: 1007041, 1014483
Change-Id: Ibeefd97118a78f5a2b3779e51d83731c546fd113
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1853910
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#709132}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1883617
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/branch-heads/3945@{#231}
Cr-Branched-From: e4635ff-refs/heads/master@{#706915}
  • Loading branch information
anforowicz committed Oct 28, 2019
1 parent 6d07216 commit fdc83ed2db852f4d4061b70182a44c0f187312db
@@ -5,6 +5,7 @@
#include "chrome/browser/devtools/devtools_window.h"

#include <algorithm>
#include <set>
#include <utility>

#include "base/base64.h"
@@ -170,8 +171,8 @@ content::WebContents* DevToolsToolboxDelegate::OpenURLFromTab(
DCHECK(source == web_contents());
if (!params.url.SchemeIs(content::kChromeDevToolsScheme))
return NULL;
content::NavigationController::LoadURLParams load_url_params(params.url);
source->GetController().LoadURLWithParams(load_url_params);
source->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(params));
return source;
}

@@ -218,7 +219,7 @@ GURL DecorateFrontendURL(const GURL& base_url) {
std::string url_string(
frontend_url +
((frontend_url.find("?") == std::string::npos) ? "?" : "&") +
"dockSide=undocked"); // TODO(dgozman): remove this support in M38.
"dockSide=undocked"); // TODO(dgozman): remove this support in M38.
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(switches::kEnableDevToolsExperiments))
url_string += "&experiments=true";
@@ -810,7 +811,7 @@ void DevToolsWindow::Show(const DevToolsToggleAction& action) {
&inspected_browser,
&inspected_tab_index);
DCHECK(inspected_browser);
DCHECK(inspected_tab_index != -1);
DCHECK_NE(-1, inspected_tab_index);

RegisterModalDialogManager(inspected_browser);

@@ -5,6 +5,9 @@
#ifndef CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
#define CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_

#include <memory>
#include <string>

#include "base/macros.h"
#include "chrome/browser/devtools/devtools_contents_resizing_strategy.h"
#include "chrome/browser/devtools/devtools_toggle_action.h"
@@ -15,6 +15,7 @@
#include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
#include "chrome/browser/ui/ash/keyboard/chrome_keyboard_bounds_observer.h"
#include "content/public/browser/host_zoom_map.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
@@ -47,8 +48,8 @@ class ChromeKeyboardContentsDelegate : public content::WebContentsDelegate,
content::WebContents* OpenURLFromTab(
content::WebContents* source,
const content::OpenURLParams& params) override {
source->GetController().LoadURL(params.url, params.referrer,
params.transition, params.extra_headers);
source->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(params));
Observe(source);
return source;
}
@@ -10,6 +10,7 @@
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/ui/input_method/ime_native_window.h"
#include "chrome/browser/ui/input_method/ime_window_observer.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/notification_service.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
@@ -164,8 +165,8 @@ void ImeWindow::Observe(int type,
content::WebContents* ImeWindow::OpenURLFromTab(
content::WebContents* source,
const content::OpenURLParams& params) {
source->GetController().LoadURL(params.url, params.referrer,
params.transition, params.extra_headers);
source->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(params));
return source;
}

@@ -101,25 +101,8 @@ WebContents* WebContentsDelegateAndroid::OpenURLFromTab(
return NULL;
}

// content::OpenURLParams -> content::NavigationController::LoadURLParams
content::NavigationController::LoadURLParams load_params(url);
load_params.referrer = params.referrer;
load_params.frame_tree_node_id = params.frame_tree_node_id;
load_params.redirect_chain = params.redirect_chain;
load_params.transition_type = params.transition;
load_params.extra_headers = params.extra_headers;
load_params.should_replace_current_entry =
params.should_replace_current_entry;
load_params.is_renderer_initiated = params.is_renderer_initiated;
load_params.has_user_gesture = params.user_gesture;
load_params.initiator_origin = params.initiator_origin;

if (params.uses_post) {
load_params.load_type = content::NavigationController::LOAD_TYPE_HTTP_POST;
load_params.post_data = params.post_data;
}

source->GetController().LoadURLWithParams(load_params);
source->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(params));

return source;
}
@@ -6,28 +6,68 @@

#include "base/memory/ref_counted_memory.h"
#include "build/build_config.h"
#include "content/public/browser/page_navigator.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/common/was_activated_option.mojom.h"

namespace content {

NavigationController::LoadURLParams::LoadURLParams(const GURL& url)
: url(url),
load_type(LOAD_TYPE_DEFAULT),
transition_type(ui::PAGE_TRANSITION_LINK),
frame_tree_node_id(RenderFrameHost::kNoFrameTreeNodeId),
is_renderer_initiated(false),
override_user_agent(UA_OVERRIDE_INHERIT),
post_data(nullptr),
can_load_local_resources(false),
should_replace_current_entry(false),
has_user_gesture(false),
should_clear_history_list(false),
started_from_context_menu(false),
navigation_ui_data(nullptr),
from_download_cross_origin_redirect(false),
was_activated(mojom::WasActivatedOption::kUnknown),
reload_type(ReloadType::NONE) {}
: url(url), is_renderer_initiated(false) {}

NavigationController::LoadURLParams::LoadURLParams(const OpenURLParams& input)
: url(input.url),
initiator_origin(input.initiator_origin),
source_site_instance(input.source_site_instance),
load_type(input.uses_post ? LOAD_TYPE_HTTP_POST : LOAD_TYPE_DEFAULT),
transition_type(input.transition),
frame_tree_node_id(input.frame_tree_node_id),
referrer(input.referrer),
redirect_chain(input.redirect_chain),
extra_headers(input.extra_headers),
is_renderer_initiated(input.is_renderer_initiated),
post_data(input.post_data),
should_replace_current_entry(input.should_replace_current_entry),
has_user_gesture(input.user_gesture),
started_from_context_menu(input.started_from_context_menu),
blob_url_loader_factory(input.blob_url_loader_factory),
href_translate(input.href_translate),
reload_type(input.reload_type) {
// |post_data| should be present iff |uses_post| is true.
//
// TODO(lukasza): Consider removing |uses_post| (redundant with |post_data|).
DCHECK_EQ(input.uses_post, !!input.post_data);

// A non-null |source_site_instance| is important for picking the right
// renderer process for hosting about:blank and/or data: URLs (their origin's
// precursor is based on |initiator_origin|).
if (url.IsAboutBlank() || url.SchemeIs(url::kDataScheme)) {
DCHECK_EQ(initiator_origin.has_value(),
static_cast<bool>(source_site_instance));
}

// Implementation notes:
// The following LoadURLParams don't have an equivalent in OpenURLParams:
// base_url_for_data_url
// virtual_url_for_data_url
// data_url_as_string
//
// can_load_local_resources
// frame_name
// from_download_cross_origin_redirect
// input_start
// navigation_ui_data
// override_user_agent
// should_clear_history_list
// was_activated
//
// The following OpenURLParams don't have an equivalent in LoadURLParams:
// disposition
// open_app_window_if_possible
// source_render_frame_id
// source_render_process_id
// triggering_event_info
}

NavigationController::LoadURLParams::~LoadURLParams() {
}
@@ -20,6 +20,7 @@
#include "content/public/browser/global_request_id.h"
#include "content/public/browser/navigation_ui_data.h"
#include "content/public/browser/reload_type.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/restore_type.h"
#include "content/public/browser/session_storage_namespace.h"
#include "content/public/browser/site_instance.h"
@@ -44,6 +45,7 @@ class BrowserContext;
class NavigationEntry;
class RenderFrameHost;
class WebContents;
struct OpenURLParams;

// A NavigationController maintains the back-forward list for a WebContents and
// manages all navigation within that list.
@@ -132,15 +134,15 @@ class NavigationController {
scoped_refptr<SiteInstance> source_site_instance;

// See LoadURLType comments above.
LoadURLType load_type;
LoadURLType load_type = LOAD_TYPE_DEFAULT;

// PageTransition for this load. See PageTransition for details.
// Note the default value in constructor below.
ui::PageTransition transition_type;
ui::PageTransition transition_type = ui::PAGE_TRANSITION_LINK;

// The browser-global FrameTreeNode ID for the frame to navigate, or
// RenderFrameHost::kNoFrameTreeNodeId for the main frame.
int frame_tree_node_id;
int frame_tree_node_id = RenderFrameHost::kNoFrameTreeNodeId;

// Referrer for this load. Empty if none.
Referrer referrer;
@@ -158,7 +160,7 @@ class NavigationController {

// User agent override for this load. See comments in
// UserAgentOverrideOption definition.
UserAgentOverrideOption override_user_agent;
UserAgentOverrideOption override_user_agent = UA_OVERRIDE_INHERIT;

// Used in LOAD_TYPE_DATA loads only. Used for specifying a base URL
// for pages loaded via data URLs.
@@ -182,29 +184,29 @@ class NavigationController {
scoped_refptr<network::ResourceRequestBody> post_data;

// True if this URL should be able to access local resources.
bool can_load_local_resources;
bool can_load_local_resources = false;

// Indicates whether this navigation should replace the current
// navigation entry.
bool should_replace_current_entry;
bool should_replace_current_entry = false;

// Used to specify which frame to navigate. If empty, the main frame is
// navigated. This is currently only used in tests.
std::string frame_name;

// Indicates that the navigation was triggered by a user gesture.
bool has_user_gesture;
bool has_user_gesture = false;

// Indicates that during this navigation, the session history should be
// cleared such that the resulting page is the first and only entry of the
// session history.
//
// The clearing is done asynchronously, and completes when this navigation
// commits.
bool should_clear_history_list;
bool should_clear_history_list = false;

// Indicates whether or not this navigation was initiated via context menu.
bool started_from_context_menu;
bool started_from_context_menu = false;

// Optional URLLoaderFactory to facilitate navigation to a blob URL.
scoped_refptr<network::SharedURLLoaderFactory> blob_url_loader_factory;
@@ -216,7 +218,7 @@ class NavigationController {

// Whether this navigation was triggered by a x-origin redirect following a
// prior (most likely <a download>) download attempt.
bool from_download_cross_origin_redirect;
bool from_download_cross_origin_redirect = false;

// Time at which the input leading to this navigation occurred. This field
// is set for links clicked by the user; the embedder is recommended to set
@@ -225,7 +227,8 @@ class NavigationController {

// Set to |kYes| if the navigation should propagate user activation. This
// is used by embedders where the activation has occurred outside the page.
mojom::WasActivatedOption was_activated;
mojom::WasActivatedOption was_activated =
mojom::WasActivatedOption::kUnknown;

// If this navigation was initiated from a link that specified the
// hrefTranslate attribute, this contains the attribute's value (a BCP47
@@ -236,6 +239,12 @@ class NavigationController {
ReloadType reload_type = ReloadType::NONE;

explicit LoadURLParams(const GURL& url);

// Copies |open_url_params| into LoadURLParams, attempting to copy all
// fields that are present in both structs (some properties are ignored
// because they are unique to LoadURLParams or OpenURLParams).
explicit LoadURLParams(const OpenURLParams& open_url_params);

~LoadURLParams();

DISALLOW_COPY_AND_ASSIGN(LoadURLParams);
@@ -442,26 +442,8 @@ WebContents* Shell::OpenURLFromTab(WebContents* source,
return nullptr;
}

NavigationController::LoadURLParams load_url_params(params.url);
load_url_params.initiator_origin = params.initiator_origin;
load_url_params.source_site_instance = params.source_site_instance;
load_url_params.transition_type = params.transition;
load_url_params.frame_tree_node_id = params.frame_tree_node_id;
load_url_params.referrer = params.referrer;
load_url_params.redirect_chain = params.redirect_chain;
load_url_params.extra_headers = params.extra_headers;
load_url_params.is_renderer_initiated = params.is_renderer_initiated;
load_url_params.should_replace_current_entry =
params.should_replace_current_entry;
load_url_params.blob_url_loader_factory = params.blob_url_loader_factory;
load_url_params.reload_type = params.reload_type;

if (params.uses_post) {
load_url_params.load_type = NavigationController::LOAD_TYPE_HTTP_POST;
load_url_params.post_data = params.post_data;
}

target->GetController().LoadURLWithParams(load_url_params);
target->GetController().LoadURLWithParams(
NavigationController::LoadURLParams(params));
return target;
}

@@ -605,11 +587,10 @@ std::unique_ptr<WebContents> Shell::SwapWebContents(
return new_contents;
}

bool Shell::ShouldAllowRunningInsecureContent(
content::WebContents* web_contents,
bool allowed_per_prefs,
const url::Origin& origin,
const GURL& resource_url) {
bool Shell::ShouldAllowRunningInsecureContent(WebContents* web_contents,
bool allowed_per_prefs,
const url::Origin& origin,
const GURL& resource_url) {
bool allowed_by_test = false;
BlinkTestController* blink_test_controller = BlinkTestController::Get();
if (blink_test_controller && switches::IsRunWebTestsSwitchPresent()) {
@@ -622,7 +603,7 @@ bool Shell::ShouldAllowRunningInsecureContent(
}

PictureInPictureResult Shell::EnterPictureInPicture(
content::WebContents* web_contents,
WebContents* web_contents,
const viz::SurfaceId& surface_id,
const gfx::Size& natural_size) {
// During tests, returning success to pretend the window was created and allow
@@ -20,6 +20,7 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_termination_info.h"
#include "content/public/browser/devtools_agent_host.h"
#include "content/public/browser/navigation_controller.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
@@ -154,26 +155,8 @@ class HeadlessWebContentsImpl::Delegate : public content::WebContentsDelegate {
return nullptr;
}

content::NavigationController::LoadURLParams load_url_params(params.url);
load_url_params.initiator_origin = params.initiator_origin;
load_url_params.source_site_instance = params.source_site_instance;
load_url_params.transition_type = params.transition;
load_url_params.frame_tree_node_id = params.frame_tree_node_id;
load_url_params.referrer = params.referrer;
load_url_params.redirect_chain = params.redirect_chain;
load_url_params.extra_headers = params.extra_headers;
load_url_params.is_renderer_initiated = params.is_renderer_initiated;
load_url_params.should_replace_current_entry =
params.should_replace_current_entry;
load_url_params.reload_type = params.reload_type;

if (params.uses_post) {
load_url_params.load_type =
content::NavigationController::LOAD_TYPE_HTTP_POST;
load_url_params.post_data = params.post_data;
}

target->GetController().LoadURLWithParams(load_url_params);
target->GetController().LoadURLWithParams(
content::NavigationController::LoadURLParams(params));
return target;
}

0 comments on commit fdc83ed

Please sign in to comment.
You can’t perform that action at this time.