Skip to content

Commit

Permalink
Add StringPiece getters for GURL, minor cleanup.
Browse files Browse the repository at this point in the history
This adds *_piece() getters for each URL component which avoids intermediate string copies.

Does some extra commenting and cleanup in gurl.h with new comments. The getters and test functions for each component type are now grouped. I removed references to future additions to encoding parameters which we will never do at this point. This also removes an unnecessary lower-casing step in GURL::SchemeIs since things are known lowercase, so scheme comparisons should be faster.

I did a quick grep for obvious cases of some of the getters that can be replaced with the *_piece versions and updated them. In net_util there is an additional cleanup where a more complicated test could be replaced with EndsWith.

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

Cr-Commit-Position: refs/heads/master@{#350733}
  • Loading branch information
brettw authored and Commit bot committed Sep 25, 2015
1 parent 1ed5226 commit adc8468
Show file tree
Hide file tree
Showing 15 changed files with 148 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ AutocompleteControllerAndroid::ClassifyPage(const GURL& gurl,
const std::string& url = gurl.spec();

if (gurl.SchemeIs(content::kChromeUIScheme) &&
gurl.host() == chrome::kChromeUINewTabHost) {
gurl.host_piece() == chrome::kChromeUINewTabHost) {
return OmniboxEventProto::NTP;
}

Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/extensions/bookmark_app_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ void GenerateIcons(
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES));
if (!domain_and_registry.empty()) {
icon_letter = domain_and_registry[0];
} else if (!app_url.host().empty()) {
icon_letter = app_url.host()[0];
} else if (app_url.has_host()) {
icon_letter = app_url.host_piece()[0];
}

// If no color has been specified, use a dark gray so it will stand out on the
Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/extensions/extension_tab_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -576,8 +576,9 @@ bool ExtensionTabUtil::IsKillURL(const GURL& url) {
if (!fixed_url.SchemeIs(content::kChromeUIScheme))
return false;

base::StringPiece fixed_host = fixed_url.host_piece();
for (size_t i = 0; i < arraysize(kill_hosts); ++i) {
if (fixed_url.host() == kill_hosts[i])
if (fixed_host == kill_hosts[i])
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/extension_web_ui.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ void UnregisterAndReplaceOverrideForWebContents(const std::string& page,
return;

GURL url = web_contents->GetURL();
if (!url.SchemeIs(content::kChromeUIScheme) || url.host() != page)
if (!url.SchemeIs(content::kChromeUIScheme) || url.host_piece() != page)
return;

// Don't use Reload() since |url| isn't the same as the internal URL that
Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ui/chrome_pages.cc
Original file line number Diff line number Diff line change
Expand Up @@ -237,9 +237,9 @@ GURL GetSettingsUrl(const std::string& sub_page) {

bool IsSettingsSubPage(const GURL& url, const std::string& sub_page) {
return (url.SchemeIs(content::kChromeUIScheme) &&
(url.host() == chrome::kChromeUISettingsHost ||
url.host() == chrome::kChromeUISettingsFrameHost) &&
url.path() == "/" + sub_page);
(url.host_piece() == chrome::kChromeUISettingsHost ||
url.host_piece() == chrome::kChromeUISettingsFrameHost) &&
url.path_piece() == "/" + sub_page);
}

bool IsTrustedPopupWindowWithScheme(const Browser* browser,
Expand Down
2 changes: 1 addition & 1 deletion content/browser/appcache/appcache_histograms.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
namespace content {

static std::string OriginToCustomHistogramSuffix(const GURL& origin_url) {
if (origin_url.host() == "docs.google.com")
if (origin_url.host_piece() == "docs.google.com")
return ".Docs";
return std::string();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ scoped_refptr<ServiceWorkerDevToolsAgentHost> GetMatchingServiceWorker(
scoped_refptr<ServiceWorkerDevToolsAgentHost> best_host;
std::string best_scope;
for (auto host : agent_hosts) {
if (host->GetURL().host() != url.host())
if (host->GetURL().host_piece() != url.host_piece())
continue;
std::string path = host->GetURL().path();
std::string file = host->GetURL().ExtractFileName();
Expand Down
14 changes: 4 additions & 10 deletions net/base/mime_sniffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -458,13 +458,12 @@ static bool SniffForOfficeDocs(const char* content,
return false;

OfficeDocType type = DOC_TYPE_NONE;
base::StringPiece url_path = url.path_piece();
for (size_t i = 0; i < arraysize(kOfficeExtensionTypes); ++i) {
std::string url_path = url.path();

if (url_path.length() < kOfficeExtensionTypes[i].extension_len)
continue;

base::StringPiece extension = base::StringPiece(url_path).substr(
base::StringPiece extension = url_path.substr(
url_path.length() - kOfficeExtensionTypes[i].extension_len);
if (base::EqualsCaseInsensitiveASCII(
extension,
Expand Down Expand Up @@ -760,15 +759,10 @@ static bool SniffCRX(const char* content,
};

// Only consider files that have the extension ".crx".
static const char kCRXExtension[] = ".crx";
// Ignore null by subtracting 1.
static const int kExtensionLength = arraysize(kCRXExtension) - 1;
if (url.path().rfind(kCRXExtension, std::string::npos, kExtensionLength) ==
url.path().size() - kExtensionLength) {
if (base::EndsWith(url.path_piece(), ".crx", base::CompareCase::SENSITIVE))
counter->Add(1);
} else {
else
return false;
}

*have_enough_content &= TruncateSize(kBytesRequiredForMagic, &size);
if (CheckForMagicNumbers(content, size,
Expand Down
19 changes: 11 additions & 8 deletions net/base/net_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ namespace net {

namespace {

std::string NormalizeHostname(const std::string& host) {
std::string NormalizeHostname(base::StringPiece host) {
std::string result = base::ToLowerASCII(host);
if (!result.empty() && *result.rbegin() == '.')
result.resize(result.size() - 1);
Expand Down Expand Up @@ -168,7 +168,7 @@ base::string16 StripWWW(const base::string16& text) {

base::string16 StripWWWFromHost(const GURL& url) {
DCHECK(url.is_valid());
return StripWWW(base::ASCIIToUTF16(url.host()));
return StripWWW(base::ASCIIToUTF16(url.host_piece()));
}

int SetNonBlocking(int fd) {
Expand Down Expand Up @@ -429,7 +429,7 @@ void GetIdentityFromURL(const GURL& url,
}

std::string GetHostOrSpecFromURL(const GURL& url) {
return url.has_host() ? TrimEndingDot(url.host()) : url.spec();
return url.has_host() ? TrimEndingDot(url.host_piece()) : url.spec();
}

GURL SimplifyUrlForRequest(const GURL& url) {
Expand Down Expand Up @@ -541,7 +541,7 @@ int GetPortFromSockaddr(const struct sockaddr* address, socklen_t address_len) {
return base::NetToHost16(*port_field);
}

bool ResolveLocalHostname(const std::string& host,
bool ResolveLocalHostname(base::StringPiece host,
uint16_t port,
AddressList* address_list) {
static const unsigned char kLocalhostIPv4[] = {127, 0, 0, 1};
Expand Down Expand Up @@ -570,7 +570,7 @@ bool ResolveLocalHostname(const std::string& host,
return true;
}

bool IsLocalhost(const std::string& host) {
bool IsLocalhost(base::StringPiece host) {
std::string normalized_host = NormalizeHostname(host);
if (IsLocalHostname(normalized_host) || IsLocal6Hostname(normalized_host))
return true;
Expand Down Expand Up @@ -602,7 +602,7 @@ bool IsLocalhost(const std::string& host) {
return false;
}

bool IsLocalhostTLD(const std::string& host) {
bool IsLocalhostTLD(base::StringPiece host) {
return IsNormalizedLocalhostTLD(NormalizeHostname(host));
}

Expand All @@ -621,9 +621,12 @@ bool HasGoogleHost(const GURL& url) {
".googleapis.com",
".ytimg.com",
};
const std::string& host = url.host();
base::StringPiece host = url.host_piece();
for (const char* suffix : kGoogleHostSuffixes) {
if (base::EndsWith(host, suffix, base::CompareCase::INSENSITIVE_ASCII))
// Here it's possible to get away with faster case-sensitive comparisons
// because the list above is all lowercase, and a GURL's host name will
// always be canonicalized to lowercase as well.
if (base::EndsWith(host, suffix, base::CompareCase::SENSITIVE))
return true;
}
return false;
Expand Down
6 changes: 3 additions & 3 deletions net/base/net_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ NET_EXPORT_PRIVATE int GetPortFromSockaddr(const struct sockaddr* address,
// hostname and false otherwise. Special IPv6 names (e.g. "localhost6")
// will resolve to an IPv6 address only, whereas other names will
// resolve to both IPv4 and IPv6.
NET_EXPORT_PRIVATE bool ResolveLocalHostname(const std::string& host,
NET_EXPORT_PRIVATE bool ResolveLocalHostname(base::StringPiece host,
uint16_t port,
AddressList* address_list);

Expand All @@ -212,9 +212,9 @@ NET_EXPORT_PRIVATE bool ResolveLocalHostname(const std::string& host,
// Note that this function does not check for IP addresses other than
// the above, although other IP addresses may point to the local
// machine.
NET_EXPORT_PRIVATE bool IsLocalhost(const std::string& host);
NET_EXPORT_PRIVATE bool IsLocalhost(base::StringPiece host);

NET_EXPORT_PRIVATE bool IsLocalhostTLD(const std::string& host);
NET_EXPORT_PRIVATE bool IsLocalhostTLD(base::StringPiece host);

// Returns true if the url's host is a Google server. This should only be used
// for histograms and shouldn't be used to affect behavior.
Expand Down
4 changes: 2 additions & 2 deletions net/base/sdch_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -214,8 +214,8 @@ SdchProblemCode SdchManager::CanFetchDictionary(
*/
// Item (1) above implies item (2). Spec should be updated.
// I take "host name match" to be "is identical to"
if (referring_url.host() != dictionary_url.host() ||
referring_url.scheme() != dictionary_url.scheme())
if (referring_url.host_piece() != dictionary_url.host_piece() ||
referring_url.scheme_piece() != dictionary_url.scheme_piece())
return SDCH_DICTIONARY_LOAD_ATTEMPT_FROM_DIFFERENT_HOST;

// TODO(jar): Remove this failsafe conservative hack which is more restrictive
Expand Down
3 changes: 2 additions & 1 deletion net/http/transport_security_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,8 @@ bool GetHPKPReport(const HostPortPair& host_port_pair,
// results in a pinning violation which results in a report being sent
// to A.com, etc.)
bool IsReportUriValidForHost(const GURL& report_uri, const std::string& host) {
return (report_uri.host() != host || !report_uri.SchemeIsCryptographic());
return (report_uri.host_piece() != host ||
!report_uri.SchemeIsCryptographic());
}

bool CheckPinsAndMaybeSendReport(
Expand Down
12 changes: 6 additions & 6 deletions url/gurl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -370,13 +370,13 @@ bool GURL::IsStandard() const {
return url::IsStandard(spec_.data(), parsed_.scheme);
}

bool GURL::SchemeIs(const char* lower_ascii_scheme) const {
bool GURL::SchemeIs(base::StringPiece lower_ascii_scheme) const {
DCHECK(base::IsStringASCII(lower_ascii_scheme));
DCHECK(base::ToLowerASCII(lower_ascii_scheme) == lower_ascii_scheme);

if (parsed_.scheme.len <= 0)
return lower_ascii_scheme == NULL;
return base::LowerCaseEqualsASCII(
base::StringPiece(spec_.data() + parsed_.scheme.begin,
parsed_.scheme.len),
lower_ascii_scheme);
return lower_ascii_scheme.empty();
return scheme_piece() == lower_ascii_scheme;
}

bool GURL::SchemeIsHTTPOrHTTPS() const {
Expand Down
Loading

0 comments on commit adc8468

Please sign in to comment.