Fixed TemplateURL::ReplaceSearchTermsInURL for non-alphanumeric terms.

Fixed TemplateURL::ReplaceSearchTermsInURL for non-alphanumeric terms.

Currently the TemplateURL::ReplaceSearchTermsInURL function escapes characters differently than the TemplateURL::HandleReplacements function does, and ignores search engine’s input encoding (which causes incorrect results when the input encoding != "UTF-8"). This patch fixes it up.

R=pkasting@chromium.org

Committed: https://crrev.com/f298455dbafbc7b613047d7909ef60649811ef10
Cr-Commit-Position: refs/heads/master@{#326017}

[Peter Kasting, 2015-04-17 16:09:29]

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

[Peter Kasting, 2015-04-17 16:09:30]

* Is there an associate BUG= for this?
* Please add at least one test covering this behavior.

Otherwise, this just looks like a blatant bug in the existing code... why were
we calling EncodeSearchTerms() but not using the result?

It looks to me like Philippe added this code; adding him as a reviewer in case
I'm missing something.

[beaudoin, 2015-04-19 14:16:31]

On 2015/04/17 16:09:30, Peter Kasting wrote:
> * Is there an associate BUG= for this?
> * Please add at least one test covering this behavior.
> 
> Otherwise, this just looks like a blatant bug in the existing code... why were
> we calling EncodeSearchTerms() but not using the result?
> 
> It looks to me like Philippe added this code; adding him as a reviewer in case
> I'm missing something.

I've looked at the patch I did in 2013:
https://codereview.chromium.org/12623029/diff/15001/chrome/browser/search_eng...

From the looks of it I agree with Peter it looks like a blatant bug. I can't
imagine I wrote that inadvertently though (although there's a lot of code I
can't imagine I wrote :)). This was upstreaming of an Android patch I did
before, so it may be some cruft that came along with it. Adding Rohit Rao in
case he remembers something specific about Android that may be affected by this
change.

[beaudoin, 2015-04-19 14:17:12]

LGTM

[Vitaly Baranov, 2015-04-20 15:14:43]

On 2015/04/17 16:09:30, Peter Kasting wrote:
> Is there an associate BUG= for this?
I could not find any bug for this. And TemplateURL::ReplaceSearchTermsInURL()
seems to be not a popular function; in fact,
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/search...
is only place I’ve found where it’s called.

> Please add at least one test covering this behavior.
I’ve changed the TemplateURLTest.ReplaceSearchTermsInURL test. The
GURL::ReplaceComponents function does its own escaping, which is not the same as
the escaping performed by TemplateURL::EncodeSearchTerms(). For example,
GURL::ReplaceComponents() replaces a space with "%20" (instead of "+") in query,
keeps a space unchanged in ref (instead of turning it into "+"), and replaces a
space with "%20" in path (the same thing). And GURL::ReplaceComponents() ignores
the input encoding obviously, therefore I've added two more tests.

[Peter Kasting, 2015-04-20 21:22:39]

LGTM

On 2015/04/20 15:14:43, Vitaly Baranov wrote:
> On 2015/04/17 16:09:30, Peter Kasting wrote:
> > Is there an associate BUG= for this?
> I could not find any bug for this.

OK; I was mostly wondering how you ran into this, then.

> The
> GURL::ReplaceComponents function does its own escaping, which is not the same
as
> the escaping performed by TemplateURL::EncodeSearchTerms(). For example,
> GURL::ReplaceComponents() replaces a space with "%20" (instead of "+") in
query,
> keeps a space unchanged in ref (instead of turning it into "+"), and replaces
a
> space with "%20" in path (the same thing).

Those sound like possible bugs in GURL::ReplaceComponents().  I would file a bug
against brettw and ask.

[Vitaly Baranov, 2015-04-21 11:51:38]

The CQ bit was checked by vitbar@yandex-team.ru

[commit-bot: I haz the power, 2015-04-21 11:52:03]

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

[commit-bot: I haz the power, 2015-04-21 12:58:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-21 12:59:15]

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