Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key

Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key

Current TemplateUrl constructor performs replacement of
search_terms_replacement_key field after initialization from
TemplateURLData. This replacement was not handled correctly in
TemplateUrl::MatchesData function. As result, TemplateUrl created from
TemplateUrlData for google engine could not be matched to its own
source data.

R=pkasting@chromium.org

Committed: https://crrev.com/58606e51c44b74f89cf5fbbb370a78731acd942a
Cr-Commit-Position: refs/heads/master@{#433504}

[Peter Kasting, 2016-11-21 02:24:50]

LGTM

Did this trigger any bugs you noticed?  Or just saw it in passing?

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

https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:70: const char
kGoogleInstantExtendedEnabledKey[] =
Nit: constexpr (the rest of these here can be constexpr too, but not required to
fix those up)

https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1198: if
(data_.search_terms_replacement_key == kGoogleInstantExtendedEnabledKey) {
Nit: {} no longer needed

https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1261: // its special initialization in
TemplateUrl constructor.
Nit: Might be more readable to add a local helper to this file called
SearchTermsReplacementKeysMatch() which takes
t_url->search_terms_replacement_key() and data->search_terms_replacement_key as
args.  Then the code here can largely stay as before.

https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
components/search_engines/template_url.cc:1263:
data->search_terms_replacement_key)
Nit: I don't think this indentation is what git cl format would do

[Alexander Yashkin, 2016-11-21 07:45:30]

On 2016/11/21 at 02:24:50, pkasting wrote:
> LGTM
> 
> Did this trigger any bugs you noticed?  Or just saw it in passing?

I encountered this while working on tests for
https://codereview.chromium.org/2479113002/.
I am not aware of any active bugs. 
Looking at code I think the possible thing this could lead to is false search
change notifications for google engine.
When already installed default google search engine could be replaced by google
search engine from some source.

> 
>
https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
> File components/search_engines/template_url.cc (right):
> 
>
https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
> components/search_engines/template_url.cc:70: const char
kGoogleInstantExtendedEnabledKey[] =
> Nit: constexpr (the rest of these here can be constexpr too, but not required
to fix those up)
Fixed in one place. Fixing all should be done in separate CL, IMHO. Will keep
this in mind.
> 
>
https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
> components/search_engines/template_url.cc:1198: if
(data_.search_terms_replacement_key == kGoogleInstantExtendedEnabledKey) {
> Nit: {} no longer needed
Fixed.

> 
>
https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
> components/search_engines/template_url.cc:1261: // its special initialization
in TemplateUrl constructor.
> Nit: Might be more readable to add a local helper to this file called
SearchTermsReplacementKeysMatch() which takes
t_url->search_terms_replacement_key() and data->search_terms_replacement_key as
args.  Then the code here can largely stay as before.
Fixed.

> 
>
https://codereview.chromium.org/2516963002/diff/1/components/search_engines/t...
> components/search_engines/template_url.cc:1263:
data->search_terms_replacement_key)
> Nit: I don't think this indentation is what git cl format would do
Passed through git cl format.

[Alexander Yashkin, 2016-11-21 08:01:22]

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

[Alexander Yashkin, 2016-11-21 08:01:22]

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/2516963002/#ps20001
(title: "More fixes after review")

[commit-bot: I haz the power, 2016-11-21 08:01:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 10:49:32]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479715282303910,
"parent_rev": ["1db48b04adc798ce3b5eb5823fad054a1d2bd0eb", null], "commit_rev":
["ef09fab6e4edb48b453820a84a2b4d949e4cf3ad", null]}

[commit-bot: I haz the power, 2016-11-21 10:50:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-21 10:51:50]

Description was changed from

==========
Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key

Current TemplateUrl constructor performs replacement of
search_terms_replacement_key field after initialization from
TemplateURLData. This replacement was not handled correctly in
TemplateUrl::MatchesData function. As result, TemplateUrl created from
TemplateUrlData for google engine could not be matched to its own
source data.

R=pkasting@chromium.org
==========

to

==========
Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key

Current TemplateUrl constructor performs replacement of
search_terms_replacement_key field after initialization from
TemplateURLData. This replacement was not handled correctly in
TemplateUrl::MatchesData function. As result, TemplateUrl created from
TemplateUrlData for google engine could not be matched to its own
source data.

R=pkasting@chromium.org

Committed: https://crrev.com/58606e51c44b74f89cf5fbbb370a78731acd942a
Cr-Commit-Position: refs/heads/master@{#433504}
==========

[commit-bot: I haz the power, 2016-11-21 10:51:52]

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

[Marc Treib, 2016-11-23 10:27:42]

treib@chromium.org changed reviewers:
+ treib@chromium.org

[Marc Treib, 2016-11-23 10:27:43]

https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
components/search_engines/template_url.cc:651: } else if (parameter ==
kGoogleInstantExtendedEnabledKey) {
Post-commit drive-by: This used to check for "google:instantExtendedEnabledKey",
but now it checks for "{google:instantExtendedEnabledKey}" (with braces). Was
that intended?

[Alexander Yashkin, 2016-11-23 10:41:51]

https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
File components/search_engines/template_url.cc (right):

https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
components/search_engines/template_url.cc:651: } else if (parameter ==
kGoogleInstantExtendedEnabledKey) {
On 2016/11/23 at 10:27:43, Marc Treib wrote:
> Post-commit drive-by: This used to check for
"google:instantExtendedEnabledKey", but now it checks for
"{google:instantExtendedEnabledKey}" (with braces). Was that intended?

Not intended, i'll fix it today.

[Alexander Yashkin, 2016-11-23 11:00:12]

On 2016/11/23 at 10:41:51, Alexander Yashkin wrote:
>
https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
> File components/search_engines/template_url.cc (right):
> 
>
https://codereview.chromium.org/2516963002/diff/20001/components/search_engin...
> components/search_engines/template_url.cc:651: } else if (parameter ==
kGoogleInstantExtendedEnabledKey) {
> On 2016/11/23 at 10:27:43, Marc Treib wrote:
> > Post-commit drive-by: This used to check for
"google:instantExtendedEnabledKey", but now it checks for
"{google:instantExtendedEnabledKey}" (with braces). Was that intended?
> 
> Not intended, i'll fix it today.

https://codereview.chromium.org/2524733008

[Peter Kasting, 2017-01-25 23:54:09]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2654093004/ by pkasting@chromium.org.

The reason for reverting is: Part 2 of 2: Reverting change that caused bug
680197.