Avoid comparisons between url::Component::len and 0

Use the more descriptive methods which do the same thing, thereby
reducing dependence upon the representation of `len`.

-- update one comment.
-- flag some unreachable code for future cleanup.

Change-Id: Id595b05e5aee23bb344fd5ecd922888edc84906d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4024209
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Owners-Override: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1071388}

components/omnibox/browser/history_quick_provider.cc

@@ -310,7 +310,7 @@ AutocompleteMatch HistoryQuickProvider::QuickMatchToACMatch(
     match.contents = url_formatter::FormatUrl(
         info.url(),
         AutocompleteMatch::GetFormatTypes(
-            autocomplete_input_.parts().scheme.len > 0 ||
+            autocomplete_input_.parts().scheme.is_nonempty() ||
                 history_match.match_in_scheme,
             history_match.match_in_subdomain),
         base::UnescapeRule::SPACES, nullptr, nullptr, nullptr);

components/omnibox/browser/history_url_provider.cc

@@ -1127,7 +1127,7 @@ AutocompleteMatch HistoryURLProvider::HistoryMatchToACMatch(
           client()->GetSchemeClassifier(), &inline_autocomplete_offset);
 
   const auto format_types = AutocompleteMatch::GetFormatTypes(
-      params.input.parts().scheme.len > 0 || !params.trim_http ||
+      params.input.parts().scheme.is_nonempty() || !params.trim_http ||
           history_match.match_in_scheme,
       history_match.match_in_subdomain);
   match.contents = url_formatter::FormatUrl(info.url(), format_types,

components/omnibox/browser/titled_url_match_utils.cc

@@ -66,7 +66,8 @@ AutocompleteMatch TitledUrlMatchToAutocompleteMatch(
                                         titled_url_match.url_match_positions,
                                         &match_in_scheme, &match_in_subdomain);
   auto format_types = AutocompleteMatch::GetFormatTypes(
-      input.parts().scheme.len > 0 || match_in_scheme, match_in_subdomain);
+      input.parts().scheme.is_nonempty() || match_in_scheme,
+      match_in_subdomain);
   const std::u16string formatted_url = url_formatter::FormatUrl(
       url, format_types, base::UnescapeRule::SPACES, nullptr, nullptr, nullptr);
 

components/safe_browsing/core/browser/db/v4_protocol_manager_util.cc

@@ -469,7 +469,7 @@ void V4ProtocolManagerUtil::CanonicalizeUrl(const GURL& url,
 
   // 3. In hostname, remove all leading and trailing dots.
   base::StringPiece host;
-  if (parsed.host.len > 0)
+  if (parsed.host.is_nonempty())
     host = base::StringPiece(url_unescaped_str.data() + parsed.host.begin,
                              parsed.host.len);
 
@@ -482,7 +482,7 @@ void V4ProtocolManagerUtil::CanonicalizeUrl(const GURL& url,
 
   // 5. In path, replace runs of consecutive slashes with a single slash.
   base::StringPiece path;
-  if (parsed.path.len > 0)
+  if (parsed.path.is_nonempty())
     path = base::StringPiece(url_unescaped_str.data() + parsed.path.begin,
                              parsed.path.len);
   std::string path_without_consecutive_slash(RemoveConsecutiveChars(path, '/'));
@@ -513,15 +513,15 @@ void V4ProtocolManagerUtil::CanonicalizeUrl(const GURL& url,
   url::ParseStandardURL(escaped_canon_url_str.data(),
                         escaped_canon_url_str.length(), &final_parsed);
 
-  if (canonicalized_hostname && final_parsed.host.len > 0) {
+  if (canonicalized_hostname && final_parsed.host.is_nonempty()) {
     *canonicalized_hostname = escaped_canon_url_str.substr(
         final_parsed.host.begin, final_parsed.host.len);
   }
-  if (canonicalized_path && final_parsed.path.len > 0) {
+  if (canonicalized_path && final_parsed.path.is_nonempty()) {
     *canonicalized_path = escaped_canon_url_str.substr(final_parsed.path.begin,
                                                        final_parsed.path.len);
   }
-  if (canonicalized_query && final_parsed.query.len > 0) {
+  if (canonicalized_query && final_parsed.query.is_nonempty()) {
     *canonicalized_query = escaped_canon_url_str.substr(
         final_parsed.query.begin, final_parsed.query.len);
   }

components/url_formatter/url_formatter.cc

@@ -695,7 +695,7 @@ std::u16string FormatUrlWithAdjustments(
   } else if ((format_types & kFormatUrlOmitTrailingSlashOnBareHostname) &&
              CanStripTrailingSlash(url)) {
     // Omit the path, which is a single trailing slash. There's no query or ref.
-    if (parsed.path.len > 0) {
+    if (parsed.path.is_nonempty()) {
       adjustments->push_back(base::OffsetAdjuster::Adjustment(
           parsed.path.begin, parsed.path.len, 0));
     }

components/url_pattern_index/url_pattern.cc

@@ -312,7 +312,7 @@ base::StringPiece UrlPattern::UrlInfo::GetLowerCaseSpec() const {
 }
 
 base::StringPiece UrlPattern::UrlInfo::GetStringHost() const {
-  if (host().len <= 0)
+  if (host().is_empty())
     return base::StringPiece();
   return base::StringPiece(&spec_[host().begin], host().len);
 }

net/tools/tld_cleanup/tld_cleanup_util.cc

@@ -115,7 +115,7 @@ NormalizeResult NormalizeRule(std::string* domain, Rule* rule) {
   GURL gurl(url);
   const std::string& spec = gurl.possibly_invalid_spec();
   url::Component host = gurl.parsed_for_possibly_invalid_spec().host;
-  if (host.len < 0) {
+  if (!host.is_valid()) {
     LOG(ERROR) << "Ignoring rule that couldn't be normalized: " << *domain;
     return kError;
   }

third_party/blink/renderer/platform/weborigin/kurl.cc

@@ -325,7 +325,7 @@ bool KURL::ProtocolIsInHTTPFamily() const {
 bool KURL::HasPath() const {
   // Note that http://www.google.com/" has a path, the path is "/". This can
   // return false only for invalid or nonstandard URLs.
-  return parsed_.path.len >= 0;
+  return parsed_.path.is_valid();
 }
 
 String KURL::LastPathComponent() const {
@@ -337,7 +337,7 @@ String KURL::LastPathComponent() const {
   // the GoogleURL library. For "/foo/bar/" the library will return the empty
   // string, but WebCore wants "bar".
   url::Component path = parsed_.path;
-  if (path.len > 0 && string_[path.end() - 1] == '/')
+  if (path.is_nonempty() && string_[path.end() - 1] == '/')
     path.len--;
 
   url::Component file;
@@ -364,7 +364,7 @@ String KURL::Host() const {
 }
 
 uint16_t KURL::Port() const {
-  if (!is_valid_ || parsed_.port.len <= 0)
+  if (!is_valid_ || parsed_.port.is_empty())
     return 0;
   DCHECK(!string_.IsNull());
   int port = string_.Is8Bit()
@@ -401,7 +401,7 @@ String KURL::FragmentIdentifier() const {
 }
 
 bool KURL::HasFragmentIdentifier() const {
-  return parsed_.ref.len >= 0;
+  return parsed_.ref.is_valid();
 }
 
 String KURL::BaseAsString() const {
@@ -410,9 +410,10 @@ String KURL::BaseAsString() const {
 }
 
 String KURL::Query() const {
-  if (parsed_.query.len >= 0)
+  if (parsed_.query.is_valid())
     return ComponentString(parsed_.query);
 
+  // TODO(tsepez): not reachable?
   // Bug: https://bugs.webkit.org/show_bug.cgi?id=21015 this function returns
   // an empty string when the query is empty rather than a null (not sure
   // which is right).
@@ -487,7 +488,8 @@ bool KURL::SetProtocol(const String& protocol) {
     // 3. If url includes credentials or has a non-null port, and buffer is
     //    "file", then return.
     if (new_protocol_is_file && !old_protocol_is_file &&
-        (HasPort() || parsed_.username.len > 0 || parsed_.password.len > 0)) {
+        (HasPort() || parsed_.username.is_nonempty() ||
+         parsed_.password.is_nonempty())) {
       // This fails silently, which is weird, but necessary to give the expected
       // behaviour when setting location.protocol. See
       // https://html.spec.whatwg.org/multipage/history.html#dom-location-protocol.
@@ -496,7 +498,7 @@ bool KURL::SetProtocol(const String& protocol) {
 
     // 4. If url’s scheme is "file" and its host is an empty host, then return.
     if (!new_protocol_is_file && old_protocol_is_file &&
-        parsed_.host.len <= 0) {
+        parsed_.host.is_empty()) {
       // This fails silently as above.
       return true;
     }
@@ -783,11 +785,11 @@ bool EqualIgnoringFragmentIdentifier(const KURL& a, const KURL& b) {
   // begin (if it exists) points to the character *after* the '#', so we need
   // to subtract one.
   int a_length = a.string_.length();
-  if (a.parsed_.ref.len >= 0)
+  if (a.parsed_.ref.is_valid())
     a_length = a.parsed_.ref.begin - 1;
 
   int b_length = b.string_.length();
-  if (b.parsed_.ref.len >= 0)
+  if (b.parsed_.ref.is_valid())
     b_length = b.parsed_.ref.begin - 1;
 
   if (a_length != b_length)
@@ -965,8 +967,9 @@ StringView KURL::StringViewForInvalidComponent() const {
 }
 
 StringView KURL::ComponentStringView(const url::Component& component) const {
-  if (!is_valid_ || component.len <= 0)
+  if (!is_valid_ || component.is_empty())
     return StringViewForInvalidComponent();
+
   // begin and len are in terms of bytes which do not match
   // if string() is UTF-16 and input contains non-ASCII characters.
   // However, the only part in urlString that can contain non-ASCII
@@ -975,7 +978,6 @@ StringView KURL::ComponentStringView(const url::Component& component) const {
   // byte) will be longer than what's needed by 'mid'. However, mid
   // truncates len to avoid go past the end of a string so that we can
   // get away without doing anything here.
-
   int max_length = GetString().length() - component.begin;
   return StringView(GetString(), component.begin,
                     component.len > max_length ? max_length : component.len);

url/gurl.cc

@@ -411,9 +411,9 @@ std::string GURL::ExtractFileName() const {
 }
 
 base::StringPiece GURL::PathForRequestPiece() const {
-  DCHECK(parsed_.path.len > 0)
+  DCHECK(parsed_.path.is_nonempty())
       << "Canonical path for requests should be non-empty";
-  if (parsed_.ref.len >= 0) {
+  if (parsed_.ref.is_valid()) {
     // Clip off the reference when it exists. The reference starts after the
     // #-sign, so we have to subtract one to also remove it.
     return base::StringPiece(&spec_[parsed_.path.begin],
@@ -455,7 +455,7 @@ base::StringPiece GURL::GetContentPiece() const {
   if (!is_valid_)
     return base::StringPiece();
   url::Component content_component = parsed_.GetContent();
-  if (!SchemeIs(url::kJavaScriptScheme) && parsed_.ref.len >= 0)
+  if (!SchemeIs(url::kJavaScriptScheme) && parsed_.ref.is_valid())
     content_component.len -= parsed_.ref.len + 1;
   return ComponentStringPiece(content_component);
 }

url/third_party/mozilla/url_parse.cc

@@ -217,7 +217,7 @@ void ParsePath(const CHAR* spec,
     ref->reset();
     return;
   }
-  DCHECK(path.len > 0) << "We should never have 0 length paths";
+  DCHECK(path.is_nonempty()) << "We should never have 0 length paths";
 
   // Search for first occurrence of either ? or #.
   int query_separator = -1;  // Index of the '?'

url/url_canon_etc.cc

@@ -169,7 +169,7 @@ bool DoUserInfo(const CHAR* username_spec,
                 CanonOutput* output,
                 Component* out_username,
                 Component* out_password) {
-  if (username.len <= 0 && password.len <= 0) {
+  if (username.is_empty() && password.is_empty()) {
     // Common case: no user info. We strip empty username/passwords.
     *out_username = Component();
     *out_password = Component();
@@ -178,7 +178,7 @@ bool DoUserInfo(const CHAR* username_spec,
 
   // Write the username.
   out_username->begin = output->length();
-  if (username.len > 0) {
+  if (username.is_nonempty()) {
     // This will escape characters not valid for the username.
     AppendStringOfType(&username_spec[username.begin],
                        static_cast<size_t>(username.len), CHAR_USERINFO,
@@ -188,7 +188,7 @@ bool DoUserInfo(const CHAR* username_spec,
 
   // When there is a password, we need the separator. Note that we strip
   // empty but specified passwords.
-  if (password.len > 0) {
+  if (password.is_nonempty()) {
     output->push_back(':');
     out_password->begin = output->length();
     AppendStringOfType(&password_spec[password.begin],

url/url_canon_path.cc

@@ -407,7 +407,7 @@ bool DoPath(const CHAR* spec,
             Component* out_path) {
   bool success = true;
   out_path->begin = output->length();
-  if (path.len > 0) {
+  if (path.is_nonempty()) {
     // Write out an initial slash if the input has none. If we just parse a URL
     // and then canonicalize it, it will of course have a slash already. This
     // check is for the replacement and relative URL resolving cases of file

url/url_canon_query.cc

@@ -106,7 +106,7 @@ void DoCanonicalizeQuery(const CHAR* spec,
                          CharsetConverter* converter,
                          CanonOutput* output,
                          Component* out_query) {
-  if (query.len < 0) {
+  if (!query.is_valid()) {
     *out_query = Component();
     return;
   }

url/url_canon_relative.cc

@@ -239,7 +239,7 @@ void CopyOneComponent(const char* source,
                       const Component& source_component,
                       CanonOutput* output,
                       Component* output_component) {
-  if (source_component.len < 0) {
+  if (!source_component.is_valid()) {
     // This component is not present.
     *output_component = Component();
     return;
@@ -323,7 +323,7 @@ bool DoResolveRelativePath(const char* base_url,
                               std::max({path.end(), query.end(), ref.end()}));
   output->Append(base_url, base_parsed.path.begin);
 
-  if (path.len > 0) {
+  if (path.is_nonempty()) {
     // The path is replaced or modified.
     int true_path_begin = output->length();
 
@@ -492,7 +492,7 @@ bool DoResolveRelativeURL(const char* base_url,
   // paths (even the default path of "/" is OK).
   //
   // We allow hosts with no length so we can handle file URLs, for example.
-  if (base_parsed.path.len <= 0) {
+  if (base_parsed.path.is_empty()) {
     // On error, return the input (resolving a relative URL on a non-relative
     // base = the base).
     int base_len = base_parsed.Length();
@@ -501,7 +501,7 @@ bool DoResolveRelativeURL(const char* base_url,
     return false;
   }
 
-  if (relative_component.len <= 0) {
+  if (relative_component.is_empty()) {
     // Empty relative URL, leave unchanged, only removing the ref component.
     int base_len = base_parsed.Length();
     base_len -= base_parsed.ref.len + 1;

url/url_parse_unittest.cc

@@ -89,8 +89,8 @@ struct FileSystemURLParseCase {
 bool ComponentMatches(const char* input,
                       const char* reference,
                       const Component& component) {
-  // If the component is nonexistent (length == -1), it should begin at 0.
-  EXPECT_TRUE(component.len >= 0 || component.len == -1);
+  // Check that the -1 sentinel is the only allowed negative value.
+  EXPECT_TRUE(component.is_valid() || component.len == -1);
 
   // Begin should be valid.
   EXPECT_LE(0, component.begin);
@@ -98,7 +98,7 @@ bool ComponentMatches(const char* input,
   // A NULL reference means the component should be nonexistent.
   if (!reference)
     return component.len == -1;
-  if (component.len < 0)
+  if (!component.is_valid())
     return false;  // Reference is not NULL but we don't have anything
 
   if (strlen(reference) != static_cast<size_t>(component.len))