TemplateURL Performance - don't call replace search terms

TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb1781663a4f06%5E%21/#F8
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=

Committed: https://crrev.com/93e29f7d3258329fcfd54808b8785168e9271ad6
Cr-Commit-Position: refs/heads/master@{#371851}

[Mark P, 2016-01-19 20:06:29]

Description was changed from

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=
==========

to

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=
==========

[Mark P, 2016-01-19 20:06:29]

mpearson@chromium.org changed reviewers:
+ pkasting@chromium.org

[Mark P, 2016-01-19 20:06:43]

Peter,

Please take a look at your leisure.

thanks,
mark

[Peter Kasting, 2016-01-20 01:42:56]

LGTM

https://codereview.chromium.org/1595103007/diff/1/components/search_engines/t...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/1595103007/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:778: search_term_key_location_ =
url::Parsed::QUERY;
While you're touching this stuff, I don't think any of this block is needed.

ParseHostAndSearchTermKey() is only called one place: the end of
ParseIfNecessary().  That function, in turn, is guaranteed to have called
InvalidateCachedValues() shortly before.  So I think all these have already been
reset.

The only possible wrench here is that in between the two calls,
ParseIfNecessary() calls ParseURL(), which is non-static, and I didn't trace
deeply through that to figure out why it's non-static and what it touches.

So ideally you could verify this block can be nuked, although if you want that
could be done separately.

[Mark P, 2016-01-20 05:18:06]

https://codereview.chromium.org/1595103007/diff/1/components/search_engines/t...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/1595103007/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:778: search_term_key_location_ =
url::Parsed::QUERY;
On 2016/01/20 01:42:56, Peter Kasting wrote:
> While you're touching this stuff, I don't think any of this block is needed.
> 
> ParseHostAndSearchTermKey() is only called one place: the end of
> ParseIfNecessary().  That function, in turn, is guaranteed to have called
> InvalidateCachedValues() shortly before.  So I think all these have already
been
> reset.
> 
> The only possible wrench here is that in between the two calls,
> ParseIfNecessary() calls ParseURL(), which is non-static, and I didn't trace
> deeply through that to figure out why it's non-static and what it touches.
> 
> So ideally you could verify this block can be nuked, although if you want that
> could be done separately.

I looked closer.  ParseURL() cannot be static because it calls ParseParameter(),
which uses prepopulated_ and calls GetPostParamsString(), which uses owner_.

That said, it looks safe to remove the block you suggested.  Done.

[Mark P, 2016-01-20 05:18:13]

The CQ bit was checked by mpearson@chromium.org

[Mark P, 2016-01-20 05:18:13]

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/1595103007/#ps20001
(title: "remove unnecessary block of code")

[commit-bot: I haz the power, 2016-01-20 05:19:20]

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

[commit-bot: I haz the power, 2016-01-20 05:50:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 05:50:07]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Mark P, 2016-01-20 18:18:06]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2016-01-20 18:19:33]

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

[Mark P, 2016-01-20 18:20:37]

The CQ bit was unchecked by mpearson@chromium.org

[Mark P, 2016-01-27 19:11:30]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2016-01-27 19:12:07]

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

[commit-bot: I haz the power, 2016-01-27 20:09:35]

Description was changed from

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=
==========

to

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=
==========

[commit-bot: I haz the power, 2016-01-27 20:09:35]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-27 20:10:46]

Description was changed from

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=
==========

to

==========
TemplateURL Performance - don't call replace search terms

That's a heavy function to call.  I removed the call.

The only thing it was being used for apparently was to get the port.
Frankly, I don't see much importance in checking the port of
search engines, but I added port_ and a cached internal variable
to maintain this support and entirely removed the heavy call.

For reference, the port line was part of a large change
https://chromium.googlesource.com/chromium/src/+/5b30787532fe44ff67e29fbbedfb...
for which there was no motivating discussion about the need to check
ports.  I guess there's no harm in it though.

BUG=

Committed: https://crrev.com/93e29f7d3258329fcfd54808b8785168e9271ad6
Cr-Commit-Position: refs/heads/master@{#371851}
==========

[commit-bot: I haz the power, 2016-01-27 20:10:47]

Patchset 2 (id:??) landed as
https://crrev.com/93e29f7d3258329fcfd54808b8785168e9271ad6
Cr-Commit-Position: refs/heads/master@{#371851}