Skip to content

Commit

Permalink
Reduce string copies in GURL creation
Browse files Browse the repository at this point in the history
Converts GURL's constructor to take StringPieces instead of std::string to
avoid extra copies when converting from constants (which is fairly common).

Adds a WebStringToGURL helper function. The above change broke the ability
to give a WebString to GURL's constructor because that relied on the implicit
string16 conversion on WebString which will no longer match GURL.

This helper function will also eliminate a string copy for almost every
WebString to GURL conversion. Normally if a WebString contains a URL, it will
be ASCII. The old code would convert this ASCII to a temporary base::string16
just to pass into GURL, which can take either type (and will convert to
8-bit in the end either way).

This exposes the internal 8- and 16-bit buffers via new getters. This allows
the underlying buffers to be passed directly to GURL in the native format
without copying.

An alternative to exposing these getters would be to make the conversion
function a friend, or adding a GURL getter directly on WebString. But this
seems like the wrong type of function to have on a string class. There are a
number of other modules in Chrome that can take either 8 or 16 bit strings that
may be able to re-use this for performance-critical places.

The mojo implicit string conversion was similarly broken. In these cases, this
patch just adds a manual get() function call to retrieve the underlying
std::string rather than relying on the implicit conversion.

Media doesn't depend on content (where I put the helper function) and there
were only a few WebString -> GURL conversions there, so I did an explicit
string16 constructor for those (matching the current implicit behavior).

Review URL: https://codereview.chromium.org/1568073002

Cr-Commit-Position: refs/heads/master@{#370274}
  • Loading branch information
brettw authored and Commit bot committed Jan 20, 2016
1 parent 68f2329 commit dfbcc3b
Show file tree
Hide file tree
Showing 63 changed files with 243 additions and 121 deletions.
2 changes: 1 addition & 1 deletion chrome/browser/ui/webui/engagement/site_engagement_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class SiteEngagementUIHandlerImpl : public SiteEngagementUIHandler {

void SetSiteEngagementScoreForOrigin(const mojo::String& origin,
double score) override {
GURL origin_gurl(origin);
GURL origin_gurl(origin.get());
if (!origin_gurl.is_valid() || score < 0 ||
score > SiteEngagementScore::kMaxPoints) {
return;
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ui/webui/omnibox/omnibox_ui_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -151,14 +151,14 @@ void OmniboxUIHandler::OnResultChanged(bool default_match_changed) {
if (bookmark_model) {
for (size_t i = 0; i < result->combined_results.size(); ++i) {
result->combined_results[i]->starred = bookmark_model->IsBookmarked(
GURL(result->combined_results[i]->destination_url));
GURL(result->combined_results[i]->destination_url.get()));
}
for (size_t i = 0; i < result->results_by_provider.size(); ++i) {
const AutocompleteResultsForProviderMojo& result_by_provider =
*result->results_by_provider[i];
for (size_t j = 0; j < result_by_provider.results.size(); ++j) {
result_by_provider.results[j]->starred = bookmark_model->IsBookmarked(
GURL(result_by_provider.results[j]->destination_url));
GURL(result_by_provider.results[j]->destination_url.get()));
}
}
}
Expand Down
5 changes: 3 additions & 2 deletions chrome/renderer/chrome_content_renderer_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
#include "net/base/net_errors.h"
#include "ppapi/c/private/ppb_pdf.h"
#include "ppapi/shared_impl/ppapi_switches.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebURL.h"
#include "third_party/WebKit/public/platform/WebURLError.h"
#include "third_party/WebKit/public/platform/WebURLRequest.h"
Expand Down Expand Up @@ -557,8 +558,8 @@ bool ChromeContentRendererClient::OverrideCreatePlugin(
ChromeViewHostMsg_GetPluginInfo_Output output;
WebString top_origin = frame->top()->securityOrigin().toString();
render_frame->Send(new ChromeViewHostMsg_GetPluginInfo(
render_frame->GetRoutingID(), url, GURL(top_origin), orig_mime_type,
&output));
render_frame->GetRoutingID(), url, blink::WebStringToGURL(top_origin),
orig_mime_type, &output));
*plugin = CreatePlugin(render_frame, frame, params, output);
#else // !defined(ENABLE_PLUGINS)

Expand Down
34 changes: 21 additions & 13 deletions chrome/renderer/content_settings_observer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "content/public/renderer/document_state.h"
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_view.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebContentSettingCallbacks.h"
#include "third_party/WebKit/public/platform/WebURL.h"
#include "third_party/WebKit/public/web/WebDataSource.h"
Expand Down Expand Up @@ -99,7 +100,7 @@ GURL GetOriginOrURL(const WebFrame* frame) {
// URL is not replicated.
if (top_origin == "null")
return frame->top()->document().url();
return GURL(top_origin);
return blink::WebStringToGURL(top_origin);
}

ContentSetting GetContentSettingFromRules(
Expand Down Expand Up @@ -260,9 +261,10 @@ bool ContentSettingsObserver::allowDatabase(const WebString& name,

bool result = false;
Send(new ChromeViewHostMsg_AllowDatabase(
routing_id(), GURL(frame->securityOrigin().toString()),
GURL(frame->top()->securityOrigin().toString()), name, display_name,
&result));
routing_id(),
blink::WebStringToGURL(frame->securityOrigin().toString()),
blink::WebStringToGURL(frame->top()->securityOrigin().toString()),
name, display_name, &result));
return result;
}

Expand All @@ -285,8 +287,8 @@ void ContentSettingsObserver::requestFileSystemAccessAsync(

Send(new ChromeViewHostMsg_RequestFileSystemAccessAsync(
routing_id(), current_request_id_,
GURL(frame->securityOrigin().toString()),
GURL(frame->top()->securityOrigin().toString())));
blink::WebStringToGURL(frame->securityOrigin().toString()),
blink::WebStringToGURL(frame->top()->securityOrigin().toString())));
}

bool ContentSettingsObserver::allowImage(bool enabled_per_settings,
Expand Down Expand Up @@ -321,8 +323,10 @@ bool ContentSettingsObserver::allowIndexedDB(const WebString& name,

bool result = false;
Send(new ChromeViewHostMsg_AllowIndexedDB(
routing_id(), GURL(frame->securityOrigin().toString()),
GURL(frame->top()->securityOrigin().toString()), name, &result));
routing_id(),
blink::WebStringToGURL(frame->securityOrigin().toString()),
blink::WebStringToGURL(frame->top()->securityOrigin().toString()),
name, &result));
return result;
}

Expand Down Expand Up @@ -350,7 +354,8 @@ bool ContentSettingsObserver::allowScript(bool enabled_per_settings) {
ContentSetting setting = GetContentSettingFromRules(
content_setting_rules_->script_rules,
frame,
GURL(frame->document().securityOrigin().toString()));
blink::WebStringToGURL(
frame->document().securityOrigin().toString()));
allow = setting != CONTENT_SETTING_BLOCK;
}
allow = allow || IsWhitelistedForContentSettings();
Expand Down Expand Up @@ -386,15 +391,18 @@ bool ContentSettingsObserver::allowStorage(bool local) {
bool result = false;

StoragePermissionsKey key(
GURL(frame->document().securityOrigin().toString()), local);
blink::WebStringToGURL(frame->document().securityOrigin().toString()),
local);
std::map<StoragePermissionsKey, bool>::const_iterator permissions =
cached_storage_permissions_.find(key);
if (permissions != cached_storage_permissions_.end())
return permissions->second;

Send(new ChromeViewHostMsg_AllowDOMStorage(
routing_id(), GURL(frame->securityOrigin().toString()),
GURL(frame->top()->securityOrigin().toString()), local, &result));
routing_id(),
blink::WebStringToGURL(frame->securityOrigin().toString()),
blink::WebStringToGURL(frame->top()->securityOrigin().toString()),
local, &result));
cached_storage_permissions_[key] = result;
return result;
}
Expand Down Expand Up @@ -486,7 +494,7 @@ void ContentSettingsObserver::didUseKeygen() {
WebFrame* frame = render_frame()->GetWebFrame();
Send(new ChromeViewHostMsg_DidUseKeygen(
routing_id(),
GURL(frame->securityOrigin().toString())));
blink::WebStringToGURL(frame->securityOrigin().toString())));
}

void ContentSettingsObserver::didNotAllowPlugins() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "extensions/renderer/script_context.h"
#include "storage/common/fileapi/file_system_util.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/web/WebDOMFileSystem.h"
#include "third_party/WebKit/public/web/WebDocument.h"
#include "third_party/WebKit/public/web/WebLocalFrame.h"
Expand Down Expand Up @@ -36,7 +37,8 @@ void MediaGalleriesCustomBindings::GetMediaFileSystemObject(

blink::WebLocalFrame* webframe =
blink::WebLocalFrame::frameForCurrentContext();
const GURL origin = GURL(webframe->document().securityOrigin().toString());
const GURL origin =
blink::WebStringToGURL(webframe->document().securityOrigin().toString());
std::string fs_name =
storage::GetFileSystemName(origin, storage::kFileSystemTypeExternal);
fs_name.append("_");
Expand Down
4 changes: 3 additions & 1 deletion chrome/renderer/extensions/resource_request_policy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "extensions/common/manifest_handlers/webview_info.h"
#include "extensions/renderer/dispatcher.h"
#include "extensions/renderer/renderer_extension_registry.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebString.h"
#include "third_party/WebKit/public/web/WebConsoleMessage.h"
#include "third_party/WebKit/public/web/WebDocument.h"
Expand Down Expand Up @@ -74,7 +75,8 @@ bool ResourceRequestPolicy::CanRequestResource(
// The page_origin may be GURL("null") for unique origins like data URLs,
// but this is ok for the checks below. We only care if it matches the
// current extension or has a devtools scheme.
GURL page_origin = GURL(frame->top()->securityOrigin().toString());
GURL page_origin =
blink::WebStringToGURL(frame->top()->securityOrigin().toString());

// Exceptions are:
// - empty origin (needed for some edge cases when we have empty origins)
Expand Down
11 changes: 6 additions & 5 deletions chrome/renderer/page_load_histograms.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "extensions/common/url_pattern.h"
#include "net/base/url_util.h"
#include "net/http/http_response_headers.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebURLRequest.h"
#include "third_party/WebKit/public/platform/WebURLResponse.h"
#include "third_party/WebKit/public/web/WebDocument.h"
Expand Down Expand Up @@ -910,14 +911,14 @@ void PageLoadHistograms::Dump(WebFrame* frame) {
DCHECK(handled || !data_reduction_proxy_was_used);
}

bool came_from_websearch =
IsFromGoogleSearchResult(frame->document().url(),
GURL(frame->document().referrer()));
bool came_from_websearch = IsFromGoogleSearchResult(
frame->document().url(),
blink::WebStringToGURL(frame->document().referrer()));
int websearch_chrome_joint_experiment_id = kNoExperiment;
bool is_preview = false;
if (came_from_websearch) {
websearch_chrome_joint_experiment_id =
GetQueryStringBasedExperiment(GURL(frame->document().referrer()));
websearch_chrome_joint_experiment_id = GetQueryStringBasedExperiment(
blink::WebStringToGURL(frame->document().referrer()));
is_preview = ViaHeaderContains(frame, "1.1 Google Instant Proxy Preview");
}

Expand Down
3 changes: 2 additions & 1 deletion chrome/renderer/plugins/chrome_plugin_placeholder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_thread.h"
#include "gin/object_template_builder.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/web/WebDocument.h"
#include "third_party/WebKit/public/web/WebInputEvent.h"
#include "third_party/WebKit/public/web/WebLocalFrame.h"
Expand Down Expand Up @@ -262,7 +263,7 @@ void ChromePluginPlaceholder::PluginListChanged() {
render_frame()->Send(
new ChromeViewHostMsg_GetPluginInfo(routing_id(),
GURL(GetPluginParams().url),
GURL(top_origin),
blink::WebStringToGURL(top_origin),
mime_type,
&output));
if (output.status == status_)
Expand Down
6 changes: 4 additions & 2 deletions chrome/renderer/prerender/prerender_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "content/public/common/referrer.h"
#include "content/public/renderer/render_thread.h"
#include "content/public/renderer/render_view.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebPrerenderingSupport.h"
#include "third_party/WebKit/public/platform/WebString.h"
#include "third_party/WebKit/public/platform/WebURL.h"
Expand Down Expand Up @@ -143,8 +144,9 @@ void PrerenderDispatcher::add(const WebPrerender& prerender) {
content::RenderThread::Get()->Send(new PrerenderHostMsg_AddLinkRelPrerender(
extra_data.prerender_id(), attributes,
content::Referrer::SanitizeForRequest(
GURL(prerender.url()), content::Referrer(GURL(prerender.referrer()),
prerender.referrerPolicy())),
GURL(prerender.url()),
content::Referrer(blink::WebStringToGURL(prerender.referrer()),
prerender.referrerPolicy())),
extra_data.size(), extra_data.render_view_route_id()));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "net/base/escape.h"
#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebString.h"
#include "third_party/WebKit/public/web/WebFrame.h"
#include "third_party/WebKit/public/web/WebLocalFrame.h"
Expand Down Expand Up @@ -94,7 +95,7 @@ class TestPhishingDOMFeatureExtractor : public PhishingDOMFeatureExtractor {
GURL full_url;
if (parsed_url.has_scheme()) {
// This is already a complete URL.
full_url = GURL(partial_url);
full_url = GURL(blink::WebStringToGURL(partial_url));
} else if (!base_domain_.empty()) {
// This is a partial URL and only one frame in testing html.
full_url = GURL("http://" + base_domain_).Resolve(partial_url);
Expand Down
7 changes: 5 additions & 2 deletions chrome/renderer/worker_content_settings_client_proxy.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "content/public/renderer/render_frame.h"
#include "content/public/renderer/render_thread.h"
#include "ipc/ipc_sync_message_filter.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/web/WebDocument.h"
#include "third_party/WebKit/public/web/WebFrame.h"
#include "third_party/WebKit/public/web/WebSecurityOrigin.h"
Expand All @@ -22,8 +23,10 @@ WorkerContentSettingsClientProxy::WorkerContentSettingsClientProxy(
frame->top()->securityOrigin().isUnique())
is_unique_origin_ = true;
sync_message_filter_ = content::RenderThread::Get()->GetSyncMessageFilter();
document_origin_url_ = GURL(frame->document().securityOrigin().toString());
top_frame_origin_url_ = GURL(frame->top()->securityOrigin().toString());
document_origin_url_ =
blink::WebStringToGURL(frame->document().securityOrigin().toString());
top_frame_origin_url_ =
blink::WebStringToGURL(frame->top()->securityOrigin().toString());
}

WorkerContentSettingsClientProxy::~WorkerContentSettingsClientProxy() {}
Expand Down
3 changes: 2 additions & 1 deletion components/autofill/content/renderer/form_autofill_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "components/autofill/core/common/autofill_util.h"
#include "components/autofill/core/common/form_data.h"
#include "components/autofill/core/common/form_field_data.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebString.h"
#include "third_party/WebKit/public/platform/WebVector.h"
#include "third_party/WebKit/public/web/WebDocument.h"
Expand Down Expand Up @@ -1419,7 +1420,7 @@ bool WebFormElementToFormData(
// If the completed URL is not valid, just use the action we get from
// WebKit.
if (!form->action.is_valid())
form->action = GURL(form_element.action());
form->action = GURL(blink::WebStringToGURL(form_element.action()));

WebVector<WebFormControlElement> control_elements;
form_element.getFormControlElements(control_elements);
Expand Down
5 changes: 3 additions & 2 deletions components/html_viewer/blink_platform_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "net/base/ip_address_number.h"
#include "net/base/net_errors.h"
#include "net/base/net_util.h"
#include "third_party/WebKit/public/platform/URLConversion.h"
#include "third_party/WebKit/public/platform/WebWaitableEvent.h"
#include "ui/base/resource/resource_bundle.h"
#include "ui/events/gestures/blink/web_gesture_curve_impl.h"
Expand Down Expand Up @@ -168,8 +169,8 @@ BlinkPlatformImpl::createOffscreenGraphicsContext3D(
blink::WebGraphicsContext3D::WebGraphicsInfo* gl_info) {
// TODO(penghuang): Use the app from the right HTMLDocument.
return WebGraphicsContext3DCommandBufferImpl::CreateOffscreenContext(
global_state_, app_, GURL(attributes.topDocumentURL), attributes,
share_context, gl_info);
global_state_, app_, blink::WebStringToGURL(attributes.topDocumentURL),
attributes, share_context, gl_info);
}

blink::WebGraphicsContext3D*
Expand Down
2 changes: 1 addition & 1 deletion components/html_viewer/html_document.cc
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ void HTMLDocument::Load() {
}
}

const GURL url(extra_data->synthetic_response->url);
const GURL url(extra_data->synthetic_response->url.get());

blink::WebURLRequest web_request;
web_request.initialize();
Expand Down
10 changes: 5 additions & 5 deletions components/html_viewer/web_url_loader_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ blink::WebURLResponse::HTTPVersion StatusLineToHTTPVersion(
blink::WebURLResponse ToWebURLResponse(const URLResponsePtr& url_response) {
blink::WebURLResponse result;
result.initialize();
result.setURL(GURL(url_response->url));
result.setURL(GURL(url_response->url.get()));
result.setMIMEType(blink::WebString::fromUTF8(url_response->mime_type));
result.setTextEncodingName(blink::WebString::fromUTF8(url_response->charset));
result.setHTTPVersion(StatusLineToHTTPVersion(url_response->status_line));
Expand Down Expand Up @@ -106,7 +106,7 @@ void WebURLLoaderImpl::loadSynchronously(
if (url_response->error) {
error.domain = WebString::fromUTF8(net::kErrorDomain);
error.reason = url_response->error->code;
error.unreachableURL = GURL(url_response->url);
error.unreachableURL = GURL(url_response->url.get());
return;
}

Expand Down Expand Up @@ -177,7 +177,7 @@ void WebURLLoaderImpl::setDefersLoading(bool defers_loading) {

void WebURLLoaderImpl::OnReceivedResponse(const blink::WebURLRequest& request,
URLResponsePtr url_response) {
url_ = GURL(url_response->url);
url_ = GURL(url_response->url.get());

if (url_response->error) {
OnReceivedError(std::move(url_response));
Expand All @@ -201,7 +201,7 @@ void WebURLLoaderImpl::OnReceivedError(URLResponsePtr url_response) {
blink::WebURLError web_error;
web_error.domain = blink::WebString::fromUTF8(net::kErrorDomain);
web_error.reason = url_response->error->code;
web_error.unreachableURL = GURL(url_response->url);
web_error.unreachableURL = GURL(url_response->url.get());
web_error.staleCopyInCache = false;
web_error.isCancellation =
url_response->error->code == net::ERR_ABORTED ? true : false;
Expand All @@ -214,7 +214,7 @@ void WebURLLoaderImpl::OnReceivedRedirect(const blink::WebURLRequest& request,
// TODO(erg): setFirstPartyForCookies() and setHTTPReferrer() are unset here.
blink::WebURLRequest new_request;
new_request.initialize();
new_request.setURL(GURL(url_response->redirect_url));
new_request.setURL(GURL(url_response->redirect_url.get()));
new_request.setDownloadToFile(request.downloadToFile());
new_request.setRequestContext(request.requestContext());
new_request.setFrameType(request.frameType());
Expand Down
2 changes: 1 addition & 1 deletion components/web_view/frame.cc
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ void Frame::StartNavigate(mojo::URLRequestPtr request) {

DVLOG(2) << "Frame::StartNavigate id=" << id_ << " url=" << request->url;

const GURL requested_url(request->url);
const GURL requested_url(request->url.get());
base::TimeTicks navigation_start_time =
base::TimeTicks::FromInternalValue(request->originating_time_ticks);
tree_->delegate_->CanNavigateFrame(
Expand Down
2 changes: 1 addition & 1 deletion components/web_view/pending_web_view_load.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ PendingWebViewLoad::~PendingWebViewLoad() {}

void PendingWebViewLoad::Init(mojo::URLRequestPtr request) {
DCHECK(!frame_connection_);
pending_url_ = GURL(request->url);
pending_url_ = GURL(request->url.get());
navigation_start_time_ =
base::TimeTicks::FromInternalValue(request->originating_time_ticks);
frame_connection_.reset(new FrameConnection);
Expand Down
Loading

0 comments on commit dfbcc3b

Please sign in to comment.