Optimize URLPrefix::BestURLPrefix

Optimize URLPrefix::BestURLPrefix

This is attempt to optimize URLPrefix::BestURLPrefix by reducing
number of redundant lowercase conversions. Also removed unused
URLPrefix::IsUrlPrefix function.

BUG=604848
R=pkasting@chromium.org, mpearson@chromium.org

Committed: https://crrev.com/921bb35bb2563ce87c583e3821db2192d2d7dc68
Cr-Commit-Position: refs/heads/master@{#389401}

[Mark P, 2016-04-22 21:34:50]

I noticed you added me and Peter as reviewers for this change but did not send
out a message.  Please remember to tell us when you'd like us to take a look.

--mark

[Peter Kasting, 2016-04-23 00:31:02]

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_prefix.cc (right):

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:19: for (const URLPrefix& prefix :
list)
Nit: {}

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:26: // Like
URLPrefix::BestURLPrefixInternal() except also handles the prefix of
Nit: URLPrefix:: qualifier is incorrect

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:35: if ((best_prefix == nullptr) ||
(best_prefix->num_components == 0)) {
Nit: I think "(best_prefix->num_components < www_prefix.num_components)" was
clearer about the reason for the check than "== 0".

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:37: base::CompareCase::SENSITIVE))
Nit: Combine nested ifs

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:48: // Input prefix must be in lower
case.
Nit: lowercase

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_prefix_unittest.cc (right):

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:25: TestCase test_cases[] = {
Nit: const

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:43: for (size_t i = 0; i <
arraysize(test_cases); i++) {
Nit: Use a range-based for loop

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:44: TestCase& test_case =
test_cases[i];
Nit: const

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:47: if
(!test_case.expected_prefix.empty()) {
Nit: Remove ! and reverse arms (so "else" case doesn't read like a double
negative)

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:51: ASSERT_FALSE(prefix);
Nit: Can be EXPECT

[Alexander Yashkin, 2016-04-23 08:40:20]

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_prefix.cc (right):

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:19: for (const URLPrefix& prefix :
list)
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: {}

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:26: // Like
URLPrefix::BestURLPrefixInternal() except also handles the prefix of
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: URLPrefix:: qualifier is incorrect

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:35: if ((best_prefix == nullptr) ||
(best_prefix->num_components == 0)) {
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: I think "(best_prefix->num_components < www_prefix.num_components)" was
> clearer about the reason for the check than "== 0".

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:37: base::CompareCase::SENSITIVE))
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: Combine nested ifs

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix.cc:48: // Input prefix must be in lower
case.
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: lowercase

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
File components/omnibox/browser/url_prefix_unittest.cc (right):

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:25: TestCase test_cases[] = {
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: const

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:43: for (size_t i = 0; i <
arraysize(test_cases); i++) {
On 2016/04/23 00:31:02, Peter Kasting wrote:
> Nit: Use a range-based for loop

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:44: TestCase& test_case =
test_cases[i];
On 2016/04/23 00:31:02, Peter Kasting wrote:
> Nit: const

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:47: if
(!test_case.expected_prefix.empty()) {
On 2016/04/23 00:31:02, Peter Kasting wrote:
> Nit: Remove ! and reverse arms (so "else" case doesn't read like a double
> negative)

Done.

https://codereview.chromium.org/1897403002/diff/1/components/omnibox/browser/...
components/omnibox/browser/url_prefix_unittest.cc:51: ASSERT_FALSE(prefix);
On 2016/04/23 00:31:01, Peter Kasting wrote:
> Nit: Can be EXPECT

Done.

[Peter Kasting, 2016-04-23 19:51:30]

LGTM

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_prefix.cc (right):

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix.cc:14: // It accepts input text and prefix
suffix already converted to lower case.
Nit: How about "Implements URLPrefix::BestURLPrefix().  Expects parameters to
already be lowercased."

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix.cc:38: base::CompareCase::SENSITIVE)) {
Nit: No {}

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_prefix_unittest.cc (right):

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix_unittest.cc:33: // Mixed case test cases
with empty prefix suffix.
Nit: Blank line above this, as well as the two comment lines below.

[Alexander Yashkin, 2016-04-24 07:18:50]

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_prefix.cc (right):

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix.cc:14: // It accepts input text and prefix
suffix already converted to lower case.
On 2016/04/23 19:51:30, Peter Kasting wrote:
> Nit: How about "Implements URLPrefix::BestURLPrefix().  Expects parameters to
> already be lowercased."

Done.

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix.cc:38: base::CompareCase::SENSITIVE)) {
On 2016/04/23 19:51:30, Peter Kasting wrote:
> Nit: No {}

Done.

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/url_prefix_unittest.cc (right):

https://codereview.chromium.org/1897403002/diff/20001/components/omnibox/brow...
components/omnibox/browser/url_prefix_unittest.cc:33: // Mixed case test cases
with empty prefix suffix.
On 2016/04/23 19:51:30, Peter Kasting wrote:
> Nit: Blank line above this, as well as the two comment lines below.

Done.

[Alexander Yashkin, 2016-04-24 07:19:14]

The CQ bit was checked by a-v-y@yandex-team.ru

[Alexander Yashkin, 2016-04-24 07:19:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1897403002/#ps40001
(title: "Fixes after review, round 2")

[commit-bot: I haz the power, 2016-04-24 07:19:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1897403002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1897403002/40001

[commit-bot: I haz the power, 2016-04-24 08:17:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-24 08:18:54]

Description was changed from

==========
Optimize URLPrefix::BestURLPrefix

This is attempt to optimize URLPrefix::BestURLPrefix by reducing
number of redundant lowercase conversions. Also removed unused
URLPrefix::IsUrlPrefix function.

BUG=604848

R=pkasting@chromium.org, mpearson@chromium.org
==========

to

==========
Optimize URLPrefix::BestURLPrefix

This is attempt to optimize URLPrefix::BestURLPrefix by reducing
number of redundant lowercase conversions. Also removed unused
URLPrefix::IsUrlPrefix function.

BUG=604848

R=pkasting@chromium.org, mpearson@chromium.org

Committed: https://crrev.com/921bb35bb2563ce87c583e3821db2192d2d7dc68
Cr-Commit-Position: refs/heads/master@{#389401}
==========

[commit-bot: I haz the power, 2016-04-24 08:18:55]

Patchset 3 (id:??) landed as
https://crrev.com/921bb35bb2563ce87c583e3821db2192d2d7dc68
Cr-Commit-Position: refs/heads/master@{#389401}