Introduces a search term extraction mechanism working for arbitrary search providers.

Introduces a search term extraction mechanism working for arbitrary search providers.
Repalces the omnibox content with the actual search term when searching on the user's default search engine.
Meant to be used with instant extended.

Note: Will only convert URLs that have the espv=1 query parameter.

BUG=139176, 135106
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=160884

[beaudoin, 2012-09-12 21:02:19]

Here's my first take on this. It doesn't yet support an {anything} wildcard to
ignore the path but it could easily be added if desired. Moreover, it entirely
ignores non-search-terms query parameters, so we can't have matching depend on
the presence/absence of a specific parameter, so the espv=1 thingie is out. If
it's an absolute requirement we may have to rethink some of this.

[dhollowa, 2012-09-12 22:00:39]

With the changes to webdata, you'll need to make similar schema changes for
sync.  I.e.

 - chrome/browser/search_engines/template_url_service.cc
 - sync/proto/search_engine_specifics.proto

[dominich, 2012-09-12 22:23:12]

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:442: url.path() != path_)
nit: add braces here

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
does any provider have more than one of these alternative urls?

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_unittest.cc:734:
TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
you should also test for blank search strings in both the query and ref
parameters.

[beaudoin, 2012-09-13 18:16:45]

Fixed dominich@ comments. Working on implementing changes to sync as mentioned
by dhollowa@.

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.cc:442: url.path() != path_)
On 2012/09/12 22:23:12, dominich wrote:
> nit: add braces here

Done.

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
On 2012/09/12 22:23:12, dominich wrote:
> does any provider have more than one of these alternative urls?

If we want to support the various Google various corpuses we will need more than
one, for example: https://maps.google.com/maps?q={searchTerm}

Maybe supporting the {anything} keyword Peter suggested would solve that for
Google properties, but I think having multiple alternate urls is a small cost
that could help us in some cases.

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_unittest.cc:734:
TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
On 2012/09/12 22:23:12, dominich wrote:
> you should also test for blank search strings in both the query and ref
> parameters.

Added them.

However, more importantly: what is the expected behavior in this situation?
Currently, the full URL will show since the empty string is used to represent
"no match".

[Peter Kasting, 2012-09-13 18:22:08]

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
On 2012/09/13 18:16:45, beaudoin wrote:
> On 2012/09/12 22:23:12, dominich wrote:
> > does any provider have more than one of these alternative urls?
> 
> If we want to support the various Google various corpuses we will need more
than
> one, for example: https://maps.google.com/maps?q=%7BsearchTerm%7D

We should definitely not claim that Google Maps Search is an alternate URL for
Google Search.  Those are two distinct engines IMO.

In any case, I'm uncomfortable with "comma separated list" (especially as commas
can occur in URLs) and we don't need this now anyway, so write the simplest
possible change that does what we want and we'll add more flexibility later when
it becomes important.

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_unittest.cc:734:
TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
On 2012/09/13 18:16:45, beaudoin wrote:
> On 2012/09/12 22:23:12, dominich wrote:
> > you should also test for blank search strings in both the query and ref
> > parameters.
> 
> Added them.
> 
> However, more importantly: what is the expected behavior in this situation?
> Currently, the full URL will show since the empty string is used to represent
> "no match".

I don't have the full context here.  Is the question what should happen with a
URL like "google.com/search?q="?  If so I would say we should fail to extract
search terms from that URL, as opposed to successfully extracting the empty
string.

[dominich, 2012-09-13 18:30:30]

https://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

https://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
On 2012/09/13 18:22:08, Peter Kasting wrote:
> On 2012/09/13 18:16:45, beaudoin wrote:
> > On 2012/09/12 22:23:12, dominich wrote:
> > > does any provider have more than one of these alternative urls?
> > 
> > If we want to support the various Google various corpuses we will need more
> than
> > one, for example: https://maps.google.com/maps?q=%257BsearchTerm%257D
> 
> We should definitely not claim that Google Maps Search is an alternate URL for
> Google Search.  Those are two distinct engines IMO.
> 
> In any case, I'm uncomfortable with "comma separated list" (especially as
commas
> can occur in URLs) and we don't need this now anyway, so write the simplest
> possible change that does what we want and we'll add more flexibility later
when
> it becomes important.

That's exactly what I was getting at. This is added complexity (and not
necessarily the best way of adding the feature) that isn't necessary. It's not
clear to me how we will end up supporting each corpus. For instance, I can
imagine having a specific image_url for image search, should other providers
support that, so drag-and-drop works. Or the same for maps so we can launch the
browser directly onto a map search from, say, an email client.

Either way, I'd call this the extended_instant_url.

https://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:734:
TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
On 2012/09/13 18:22:08, Peter Kasting wrote:
> On 2012/09/13 18:16:45, beaudoin wrote:
> > On 2012/09/12 22:23:12, dominich wrote:
> > > you should also test for blank search strings in both the query and ref
> > > parameters.
> > 
> > Added them.
> > 
> > However, more importantly: what is the expected behavior in this situation?
> > Currently, the full URL will show since the empty string is used to
represent
> > "no match".
> 
> I don't have the full context here.  Is the question what should happen with a
> URL like "google.com/search?q="?  If so I would say we should fail to extract
> search terms from that URL, as opposed to successfully extracting the empty
> string.

Agreed. We don't want to have an empty string in the Omnibox.

https://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

https://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/toolbar_model.cc:216: return
template_url->ExtractSearchTermsFromURL(url);
is this checking for espv=1 in the query component? That's what the
IsInstantExtendedAPIGoogleSearchUrl did.

[beaudoin, 2012-09-13 18:31:49]

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
On 2012/09/13 18:22:08, Peter Kasting wrote:
> On 2012/09/13 18:16:45, beaudoin wrote:
> > On 2012/09/12 22:23:12, dominich wrote:
> > > does any provider have more than one of these alternative urls?
> > 
> > If we want to support the various Google various corpuses we will need more
> than
> > one, for example: https://maps.google.com/maps?q=%257BsearchTerm%257D
> 
> We should definitely not claim that Google Maps Search is an alternate URL for
> Google Search.  Those are two distinct engines IMO.
> 
> In any case, I'm uncomfortable with "comma separated list" (especially as
commas
> can occur in URLs) and we don't need this now anyway, so write the simplest
> possible change that does what we want and we'll add more flexibility later
when
> it becomes important.

Sounds good, reverting to a single alternate_url. I wasn't too happy with the
comma-separated list either. It would probably require fancier serialization,
maybe encoding the URLs somehow.

This may hint at a deeper problem, however: do we want to extract search
keywords on the user's default search engine or do we want to do it on any
search engine we know of? To me, this subtlety hides a deeper architectural
difference. If (a) then the current approach is the right one. If (b) then we
should probably care less about the specific search engine and more about a
catalog of extraction patterns.

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_unittest.cc:734:
TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
On 2012/09/13 18:22:08, Peter Kasting wrote:
> On 2012/09/13 18:16:45, beaudoin wrote:
> > On 2012/09/12 22:23:12, dominich wrote:
> > > you should also test for blank search strings in both the query and ref
> > > parameters.
> > 
> > Added them.
> > 
> > However, more importantly: what is the expected behavior in this situation?
> > Currently, the full URL will show since the empty string is used to
represent
> > "no match".
> 
> I don't have the full context here.  Is the question what should happen with a
> URL like "google.com/search?q="?  If so I would say we should fail to extract
> search terms from that URL, as opposed to successfully extracting the empty
> string.

Ok, right now it fails to match which means it returns "". The slight difference
between that and a successful match is that:

google.com/search?q=123#q=

will match "123".

[dominich, 2012-09-13 18:34:19]

On Thu, Sep 13, 2012 at 11:31 AM, <[email protected]> wrote:

>
> http://codereview.chromium.**org/10908226/diff/7001/chrome/**
>
browser/search_engines/**template_url_prepopulate_data.**cc<http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engines/template_url_prepopulate_data.cc>
> File chrome/browser/search_engines/**template_url_prepopulate_data.**cc
> (right):
>
> http://codereview.chromium.**org/10908226/diff/7001/chrome/**
>
browser/search_engines/**template_url_prepopulate_data.**cc#newcode57<http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engines/template_url_prepopulate_data.cc#newcode57>
>
> chrome/browser/search_engines/**template_url_prepopulate_data.**cc:57: //
> Comma separated list of URL patterns that can be used, in addition to
> On 2012/09/13 18:22:08, Peter Kasting wrote:
>
>> On 2012/09/13 18:16:45, beaudoin wrote:
>> > On 2012/09/12 22:23:12, dominich wrote:
>> > > does any provider have more than one of these alternative urls?
>> >
>> > If we want to support the various Google various corpuses we will
>>
> need more
>
>> than
>> > one, for example:
>>
>
https://maps.google.com/maps?**q=%257BsearchTerm%257D<https://maps.google.com...
>
>  We should definitely not claim that Google Maps Search is an alternate
>>
> URL for
>
>> Google Search.  Those are two distinct engines IMO.
>>
>
>  In any case, I'm uncomfortable with "comma separated list" (especially
>>
> as commas
>
>> can occur in URLs) and we don't need this now anyway, so write the
>>
> simplest
>
>> possible change that does what we want and we'll add more flexibility
>>
> later when
>
>> it becomes important.
>>
>
> Sounds good, reverting to a single alternate_url. I wasn't too happy
> with the comma-separated list either. It would probably require fancier
> serialization, maybe encoding the URLs somehow.
>
> This may hint at a deeper problem, however: do we want to extract search
> keywords on the user's default search engine or do we want to do it on
> any search engine we know of? To me, this subtlety hides a deeper
> architectural difference. If (a) then the current approach is the right
> one. If (b) then we should probably care less about the specific search
> engine and more about a catalog of extraction patterns.


Only on the user's default search engine.


>
>
> http://codereview.chromium.**org/10908226/diff/7001/chrome/**
>
browser/search_engines/**template_url_unittest.cc<http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engines/template_url_unittest.cc>
> File chrome/browser/search_engines/**template_url_unittest.cc (right):
>
> http://codereview.chromium.**org/10908226/diff/7001/chrome/**
>
browser/search_engines/**template_url_unittest.cc#**newcode734<http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engines/template_url_unittest.cc#newcode734>
>
> chrome/browser/search_engines/**template_url_unittest.cc:734:
> TEST_F(TemplateURLTest, ExtractSearchTermsFromURL) {
> On 2012/09/13 18:22:08, Peter Kasting wrote:
>
>> On 2012/09/13 18:16:45, beaudoin wrote:
>> > On 2012/09/12 22:23:12, dominich wrote:
>> > > you should also test for blank search strings in both the query
>>
> and ref
>
>> > > parameters.
>> >
>> > Added them.
>> >
>> > However, more importantly: what is the expected behavior in this
>>
> situation?
>
>> > Currently, the full URL will show since the empty string is used to
>>
> represent
>
>> > "no match".
>>
>
>  I don't have the full context here.  Is the question what should
>>
> happen with a
>
>> URL like "google.com/search?q="?  If so I would say we should fail to
>>
> extract
>
>> search terms from that URL, as opposed to successfully extracting the
>>
> empty
>
>> string.
>>
>
> Ok, right now it fails to match which means it returns "". The slight
> difference between that and a successful match is that:
>
> google.com/search?q=123#q=
>
> will match "123".
>

This is incorrect. Ref should always override query. The example should
fail to match.


>
>
http://codereview.chromium.**org/10908226/<http://codereview.chromium.org/109...
>

[Peter Kasting, 2012-09-13 18:38:22]

On 2012/09/13 18:34:19, dominich wrote:
> > Ok, right now it fails to match which means it returns "". The slight
> > difference between that and a successful match is that:
> >
> > google.com/search?q=123#q=
> >
> > will match "123".
> >
> 
> This is incorrect. Ref should always override query. The example should
> fail to match.

Can we encode this preference in some way other than hardcoding it?  I can
imagine that for another search engine, query should always override ref, or
more generally, "URL format A should be matched in preference to URL format B",
where A and B may each be the main or the alternate URL.

I'm not sure of the best encoding format for this sort of data though.

[dominich, 2012-09-13 18:41:07]

On Thu, Sep 13, 2012 at 11:38 AM, <[email protected]> wrote:

> On 2012/09/13 18:34:19, dominich wrote:
>
>> > Ok, right now it fails to match which means it returns "". The slight
>> > difference between that and a successful match is that:
>> >
>> > google.com/search?q=123#q=
>> >
>> > will match "123".
>> >
>>
>
>  This is incorrect. Ref should always override query. The example should
>> fail to match.
>>
>
> Can we encode this preference in some way other than hardcoding it?  I can
> imagine that for another search engine, query should always override ref,
> or
> more generally, "URL format A should be matched in preference to URL
> format B",
> where A and B may each be the main or the alternate URL.
>
> I'm not sure of the best encoding format for this sort of data though.
>

I had a version of this CL that did that - it changed the URL to be a
struct of URL/component pairs where component was either PATH, QUERY or REF
and specified which URL component to prefer when matching.


>
>
http://codereview.chromium.**org/10908226/<http://codereview.chromium.org/109...
>

[dominich, 2012-09-14 21:34:04]

http://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:203: if (!profile ||
!chrome::search::IsInstantExtendedAPIEnabled(profile))
One more thought (from a discussion with dcblack): We should only do the
replacement if the navigation entry page transition is typed (or maybe reload).
User interaction is a necessary requirement.

[beaudoin, 2012-09-15 02:29:26]

@David As suggested, I added alternate_urls to sync and pref. I've also added
them to policy but I'm not sure it's required or whether I did that correctly. A
quick look would be really appreciated.

Still need to do/think about:
- How to handle the espv=1 requirement (Only for Google? For every provider?)
- Whether we revert to a single alternate_url for now or if we suffer the
version change once and go to multiple alternate_urls right away (and if so, are
comma separated lists good enough?)
- Do we limit omnibox replacement to some specific transitions
- Do we need to support an {anything} flag
- Are we happy with alternate_urls_ order as determining QUERY vs REF priority
or do we want a flag or a priority index per search URL.
- If alternate_urls_ are present, should we disregard url and instant_url
entirely for search term extraction? It would make the code cleaner and allow
for mandatory query/ref parameters.
- Fail early in case of empty query parameter. For example: 
http://google.com/?q=123#q=  should fail, not match 123.

For future CLs:
- Express the prepopulated search engines as a JSON format.
- Perform omnibox search term replacement for any known search engine, including
other Google corpuses (not just default).

[beaudoin, 2012-09-15 02:37:00]

Addedum...

For future CLs:
- Ensure TemplateURLService::UpdateKeywordSearchTermsForURL uses the same search
term extraction mechanism as omnibox replacement.

[dhollowa, 2012-09-19 19:51:03]

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:366: // |instant_url|, to extract
search terms from a URL. Enforce use of a setter
Do we actually extract from |instant_url|?  My understanding of the instant code
is that it puts the search url into history for search terms extraction to work
correctly.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:470: // Returns the total number of
URL comprised in this template, including
nit: s/URL/URLs/

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:480: // Use the various URL
comprised in this template to match the provided |url|
nit: s/URL/URLs/

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:88: ASCIIToUTF16("tractor
supply"),
Hmm.  I'm confused.  Why did this change?

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:186: // Test that we don't
replace any URLs when the InstantExtended API is enabled.
This comment seems wrong.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:146: bool
MigrateToVersion47AddAlternateUrlsColumn();
nit: s/Url/URL/ for local consistency.  I realize it is mixed, but it'd be nice
to be consistent.  Maybe change |MigrateToVersion29InstantUrlToSupportsInstant|
too.

[beaudoin, 2012-09-22 06:55:45]

+joaodasilva  for policy
+lipalani  for sync
+sky  for browser

Pushed a new set of changes answering most code review comments.

Highlights:
- Now requires espv=1 in the query parameters. This is checked no matter which
default search engine you use.
- Still using multiple alternate_urls as a comma separated list. I actually
needed two alternate_urls to ensure we match both google.com/?espv=1&q=bar#q=foo
and google.com/search?espv=1&q=bar#q=foo (GWS treats them both as a search for
foo but for the second one we extracted "bar")
- google.com/?espv=1&q=foo#q=  No longer matches.

Todo:
- Not yet gated on a GENERATED page transition. Not sure how to do this. Could
be left for a future CL?

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/7001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_prepopulate_data.cc:57: // Comma
separated list of URL patterns that can be used, in addition to
On 2012/09/13 18:31:49, beaudoin wrote:
> On 2012/09/13 18:22:08, Peter Kasting wrote:
> > On 2012/09/13 18:16:45, beaudoin wrote:
> > > On 2012/09/12 22:23:12, dominich wrote:
> > > > does any provider have more than one of these alternative urls?
> > > 
> > > If we want to support the various Google various corpuses we will need
more
> > than
> > > one, for example: https://maps.google.com/maps?q=%25257BsearchTerm%25257D
> > 
> > We should definitely not claim that Google Maps Search is an alternate URL
for
> > Google Search.  Those are two distinct engines IMO.
> > 
> > In any case, I'm uncomfortable with "comma separated list" (especially as
> commas
> > can occur in URLs) and we don't need this now anyway, so write the simplest
> > possible change that does what we want and we'll add more flexibility later
> when
> > it becomes important.
> 
> Sounds good, reverting to a single alternate_url. I wasn't too happy with the
> comma-separated list either. It would probably require fancier serialization,
> maybe encoding the URLs somehow.
> 
> This may hint at a deeper problem, however: do we want to extract search
> keywords on the user's default search engine or do we want to do it on any
> search engine we know of? To me, this subtlety hides a deeper architectural
> difference. If (a) then the current approach is the right one. If (b) then we
> should probably care less about the specific search engine and more about a
> catalog of extraction patterns.

After discussions, leaving to multiple alternate_urls_

http://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:203: if (!profile ||
!chrome::search::IsInstantExtendedAPIEnabled(profile))
On 2012/09/14 21:34:04, dominich wrote:
> One more thought (from a discussion with dcblack): We should only do the
> replacement if the navigation entry page transition is typed (or maybe
reload).
> User interaction is a necessary requirement.

From the discussion it looks like we should only replace on GENERATED
transitions, but I don't know how/where to check for that. Pointers appreciated.

http://codereview.chromium.org/10908226/diff/11002/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:216: return
template_url->ExtractSearchTermsFromURL(url);
On 2012/09/13 18:30:30, dominich wrote:
> is this checking for espv=1 in the query component? That's what the
> IsInstantExtendedAPIGoogleSearchUrl did.

Done.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:366: // |instant_url|, to extract
search terms from a URL. Enforce use of a setter
On 2012/09/19 19:51:03, dhollowa wrote:
> Do we actually extract from |instant_url|?  My understanding of the instant
code
> is that it puts the search url into history for search terms extraction to
work
> correctly.

Not sure I fully understand your comment, but still updated the comment since we
are no longer using |instant_url|

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:470: // Returns the total number of
URL comprised in this template, including
On 2012/09/19 19:51:03, dhollowa wrote:
> nit: s/URL/URLs/

Done.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/search_engin...
chrome/browser/search_engines/template_url.h:480: // Use the various URL
comprised in this template to match the provided |url|
On 2012/09/19 19:51:03, dhollowa wrote:
> nit: s/URL/URLs/

Done.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:88: ASCIIToUTF16("tractor
supply"),
On 2012/09/19 19:51:03, dhollowa wrote:
> Hmm.  I'm confused.  Why did this change?

Reverted now that espv=1 is required.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:186: // Test that we don't
replace any URLs when the InstantExtended API is enabled.
On 2012/09/19 19:51:03, dhollowa wrote:
> This comment seems wrong.

Done.

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/webdata/keyw...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/10908226/diff/5004/chrome/browser/webdata/keyw...
chrome/browser/webdata/keyword_table.h:146: bool
MigrateToVersion47AddAlternateUrlsColumn();
On 2012/09/19 19:51:03, dhollowa wrote:
> nit: s/Url/URL/ for local consistency.  I realize it is mixed, but it'd be
nice
> to be consistent.  Maybe change
|MigrateToVersion29InstantUrlToSupportsInstant|
> too.

Done.

[Peter Kasting, 2012-09-23 02:03:33]

I'm still opposed to any solution that involves parsing a string for
comma-separated URLs.

[beaudoin1, 2012-09-23 02:37:50]

Proposal: Serialize the list in a fancier way (any helper string list
serialization code in Chrome?) Or maybe I can directly use a string list in
Webdatabase, profile and sync? (From phone.)
On Sep 22, 2012 10:03 PM, <[email protected]> wrote:

> I'm still opposed to any solution that involves parsing a string for
> comma-separated URLs.
>
>
http://codereview.chromium.**org/10908226/<http://codereview.chromium.org/109...
>

[Peter Kasting, 2012-09-24 05:29:29]

On Sat, Sep 22, 2012 at 7:37 PM, Philippe Beaudoin <[email protected]>wrote:

> Proposal: Serialize the list in a fancier way (any helper string list
> serialization code in Chrome?) Or maybe I can directly use a string list in
> Webdatabase, profile and sync? (From phone.)
>
The only place you can't use non-POD classes is as something requiring
static initialization (e.g. compile-time constants).  They're fine
elsewhere.

PK

[Joao da Silva, 2012-09-24 09:12:29]

The pref should be a ListValue instead of a comma-separated list of strings in a
StringValue. Same for the policy value. How that list is serialized is a detail
of the persistence layer, and shouldn't percolate to other layers.

If you keep the pref as a string then there has to be some conversion from the
policy (as a list of strings) to the pref. See how
DefaultSearchProviderEncodings does that (which is an unfortunate type mismatch,
and shouldn't be repeated if possible).

It'd be extra cool if you added a test for this in
chrome/browser/policy/policy_browsertest.cc. For hints and
copy-pasteable-snippets see

IN_PROC_BROWSER_TEST_F(PolicyTest, DefaultSearchProvider)

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1554: 'supported_on': ['chrome.*:10-',
'chrome_os:0.11-'],
Update the versions to 24

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1558: 'caption': '''Comma separated list
of alternate URLs for the default search provider.''',
Why not use a list policy, like DefaultSearchProviderEncodings?

http://codereview.chromium.org/10908226/diff/17001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_factory.cc (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_factory.cc:82: std::string(),
Seems like this should be a list pref? (same for the encoding, but it's too late
for that now)

http://codereview.chromium.org/10908226/diff/17001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:550: "test_policy": {
"DefaultSearchProviderSearchURL": "http://www.google.com/?q={searchTerms}",
"DefaultSearchProviderAlternateURLs": ["UTF-8"] },
The value for DefaultSearchProviderAlternateURLs is wrong, because it's not a
valid search URL and because it's a list though the policy is currently a
string.

[beaudoin, 2012-09-27 17:37:13]

Summary of changes in that latest version:
- Made the alternate_urls pref a ListValue
- Similarly policy is now an Array
- Prepopulated alternate_urls uses a JSON array (instead of comma separated
list)
- Webdatabase uses a JSON array too (instead of CSV)
- The sync proto uses a "repeated string" (instead of a CSV in a string)

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1554: 'supported_on': ['chrome.*:10-',
'chrome_os:0.11-'],
On 2012/09/24 09:12:29, Joao da Silva wrote:
> Update the versions to 24

Done.

http://codereview.chromium.org/10908226/diff/17001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1558: 'caption': '''Comma separated list
of alternate URLs for the default search provider.''',
On 2012/09/24 09:12:29, Joao da Silva wrote:
> Why not use a list policy, like DefaultSearchProviderEncodings?

Done.

http://codereview.chromium.org/10908226/diff/17001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service_factory.cc (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service_factory.cc:82: std::string(),
On 2012/09/24 09:12:29, Joao da Silva wrote:
> Seems like this should be a list pref? (same for the encoding, but it's too
late
> for that now)

Done.

http://codereview.chromium.org/10908226/diff/17001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

http://codereview.chromium.org/10908226/diff/17001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:550: "test_policy": {
"DefaultSearchProviderSearchURL": "http://www.google.com/?q={searchTerms}",
"DefaultSearchProviderAlternateURLs": ["UTF-8"] },
On 2012/09/24 09:12:29, Joao da Silva wrote:
> The value for DefaultSearchProviderAlternateURLs is wrong, because it's not a
> valid search URL and because it's a list though the policy is currently a
> string.

Done.

[Joao da Silva, 2012-09-28 11:46:10]

policy changes LGTM. It'd be extra cool to have a test in
src/chrome/browser/policy/policy_browsertest.cc for this :-)

http://codereview.chromium.org/10908226/diff/38002/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1582: 'caption': '''Comma separated list
of alternate URLs for the default search provider.''',
Update the caption: 'List of alternate ...'

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:12: #include
"base/string_number_conversions.h"
needed?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:22: #include "googleurl/src/url_parse.h"
needed?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (left):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:22: // The query key that identifies a
Google Extended API request for Instant.
Did you mean to delete kInstantExtendedAPIParam too? If not then please leave
the comment.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.h:292: // Make sure that the
|path| if present in |prefs_| and is a ListValue.  If
typo: *is present in |prefs_| ... Same in the comment above.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:35: //
TODO(beaudoin): Cleanup
Remove this?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:6: #include
"base/command_line.h"
used?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:13: #include
"chrome/common/chrome_switches.h"
used?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:686:
data.alternate_urls.push_back("http://google.com/alt/?ext=foo&q={searchTerms}#ref=bar");
nit: line length

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:446: base::JSONReader json_reader;
nit: move this next to the place where it's used

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:475: if
(value->GetAsList(&alternate_urls_value)) {
|value| can be NULL from the return of ReadToValue().

[dhollowa, 2012-09-28 18:06:29]

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (left):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:22: // The query key that identifies a
Google Extended API request for Instant.
On 2012/09/28 11:46:10, Joao da Silva wrote:
> Did you mean to delete kInstantExtendedAPIParam too? If not then please leave
> the comment.

This is used in |UIThreadSearchTermsData::InstantEnabledParam| so it needs to
stay.  +1 that the comment should remain too.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:89: bool
IsInstantExtendedAPIGoogleSearchUrl(const std::string& url);
This should stay and continue to be used by search_tab_helper.cc.  Extracting
search terms is orthogonal to enabling the instant-extended experience on the
search-results page.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:44: // correct string field. Use
|INDEXED| to indicate that the numerical |index_|
nit: |index_in_owner_|

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:229: // If |type_| is |INDEXED|,
this |index_| is used instead to refer to a URL
nit: |index_in_owner_|

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3614: ListValue*
alternate_urls = NULL;
nit: How about initializing to |&empty_list| here and avoiding conditional logic
below (:3626)?

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:169: else if
(CanExtractSearchTerms(url))
Instant extended is not equivalent to search term extractability.  We should
keep |google_util::IsInstantExtendedAPIGoogleSearchUrl| for the moment.  We can
think about a generalized signal for search providers that support
instant-extended later.

In short espv=1 has to stay, and this code should check for its presence, not
key off extractability.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:92: // Serialize |alternate_urls| to
JSON.
I am fine with serializing this list to JSON for now, but as mentioned in your
comment below is not a long term solution.  Can you please log a bug for this
and cite it in the comment please?  I'd like to make sure this doesn't get lost.

[beaudoin, 2012-10-02 17:43:28]

Summary of changes:
- Answered all review comments.
- Added a test for search term replacement to policy_browsertest (@joaodasilva
please review)
- Restored IsInstantExtendedAPIGoogleSearchUrl
- Entered crbug.com/153520 to track moving alternate_urls to their own SQL
table.

Thanks a lot everybody for the reviews! I know this is a painful one and your
time is very appreciated.

http://codereview.chromium.org/10908226/diff/38002/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:1582: 'caption': '''Comma separated list
of alternate URLs for the default search provider.''',
On 2012/09/28 11:46:10, Joao da Silva wrote:
> Update the caption: 'List of alternate ...'

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:12: #include
"base/string_number_conversions.h"
On 2012/09/28 11:46:10, Joao da Silva wrote:
> needed?

Needed.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:22: #include "googleurl/src/url_parse.h"
On 2012/09/28 11:46:10, Joao da Silva wrote:
> needed?

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (left):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:22: // The query key that identifies a
Google Extended API request for Instant.
On 2012/09/28 18:06:29, dhollowa wrote:
> On 2012/09/28 11:46:10, Joao da Silva wrote:
> > Did you mean to delete kInstantExtendedAPIParam too? If not then please
leave
> > the comment.
> 
> This is used in |UIThreadSearchTermsData::InstantEnabledParam| so it needs to
> stay.  +1 that the comment should remain too.
> 

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:89: bool
IsInstantExtendedAPIGoogleSearchUrl(const std::string& url);
On 2012/09/28 18:06:29, dhollowa wrote:
> This should stay and continue to be used by search_tab_helper.cc.  Extracting
> search terms is orthogonal to enabling the instant-extended experience on the
> search-results page.

IMHO it's fishy to have two different mechanisms for detecting Google Search
URLs, one relying on a hardcoded procedure (this one) and another parameterized
by the TemplateURL data.

This is a deeper issue though, so reverting in that CL.

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_handler.h (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_handler.h:292: // Make sure that the
|path| if present in |prefs_| and is a ListValue.  If
On 2012/09/28 11:46:10, Joao da Silva wrote:
> typo: *is present in |prefs_| ... Same in the comment above.

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:44: // correct string field. Use
|INDEXED| to indicate that the numerical |index_|
On 2012/09/28 18:06:29, dhollowa wrote:
> nit: |index_in_owner_|

Good catch! :)

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:229: // If |type_| is |INDEXED|,
this |index_| is used instead to refer to a URL
On 2012/09/28 18:06:29, dhollowa wrote:
> nit: |index_in_owner_|

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:35: //
TODO(beaudoin): Cleanup
On 2012/09/28 11:46:10, Joao da Silva wrote:
> Remove this?

OOops! Sorry...

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3614: ListValue*
alternate_urls = NULL;
On 2012/09/28 18:06:29, dhollowa wrote:
> nit: How about initializing to |&empty_list| here and avoiding conditional
logic
> below (:3626)?

Good catch! I thought "value->GetAsList(&alternate_urls);" would set
alternate_urls to NULL on failure, but upon checking the code it leaves it
unchanged. Also means I have to change my LOG(ERROR) code below.

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:6: #include
"base/command_line.h"
On 2012/09/28 11:46:10, Joao da Silva wrote:
> used?

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:13: #include
"chrome/common/chrome_switches.h"
On 2012/09/28 11:46:10, Joao da Silva wrote:
> used?

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_unittest.cc:686:
data.alternate_urls.push_back("http://google.com/alt/?ext=foo&q={searchTerms}#ref=bar");
On 2012/09/28 11:46:10, Joao da Silva wrote:
> nit: line length

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:169: else if
(CanExtractSearchTerms(url))
On 2012/09/28 18:06:29, dhollowa wrote:
> Instant extended is not equivalent to search term extractability.  We should
> keep |google_util::IsInstantExtendedAPIGoogleSearchUrl| for the moment.  We
can
> think about a generalized signal for search providers that support
> instant-extended later.
> 
> In short espv=1 has to stay, and this code should check for its presence, not
> key off extractability.

QQ: Should I check for espv=1 for search term replacement too? (I do for the
moment.)

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:92: // Serialize |alternate_urls| to
JSON.
On 2012/09/28 18:06:29, dhollowa wrote:
> I am fine with serializing this list to JSON for now, but as mentioned in your
> comment below is not a long term solution.  Can you please log a bug for this
> and cite it in the comment please?  I'd like to make sure this doesn't get
lost.

crbug.com/153520

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:446: base::JSONReader json_reader;
On 2012/09/28 11:46:10, Joao da Silva wrote:
> nit: move this next to the place where it's used

Done.

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:475: if
(value->GetAsList(&alternate_urls_value)) {
On 2012/09/28 11:46:10, Joao da Silva wrote:
> |value| can be NULL from the return of ReadToValue().

Good catch!

Done.

[dhollowa, 2012-10-02 18:14:13]

LGTM.  Thanks for persevering!

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
File chrome/browser/google/google_util.h (left):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
chrome/browser/google/google_util.h:89: bool
IsInstantExtendedAPIGoogleSearchUrl(const std::string& url);
On 2012/10/02 17:43:29, beaudoin wrote:
> On 2012/09/28 18:06:29, dhollowa wrote:
> > This should stay and continue to be used by search_tab_helper.cc. 
Extracting
> > search terms is orthogonal to enabling the instant-extended experience on
the
> > search-results page.
> 
> IMHO it's fishy to have two different mechanisms for detecting Google Search
> URLs, one relying on a hardcoded procedure (this one) and another
parameterized
> by the TemplateURL data.
> 
> This is a deeper issue though, so reverting in that CL.
> 
> Done.
> 

Yes, ideally search term extraction is completely independent of
extended-instant.  For now though we're governing them with a single flag.  The
search_tab_helper code has everything to do with extended-instant and nothing to
do with search term extraction.  So it is correct that search_tab_helper
continue to use this method, but that GetSearchTermsFromGoogleSearchURL doesn't
(and is gone now).

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search_tab_helper.cc (right):

http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
chrome/browser/ui/search/search_tab_helper.cc:169: else if
(CanExtractSearchTerms(url))
On 2012/10/02 17:43:29, beaudoin wrote:
> On 2012/09/28 18:06:29, dhollowa wrote:
> > Instant extended is not equivalent to search term extractability.  We should
> > keep |google_util::IsInstantExtendedAPIGoogleSearchUrl| for the moment.  We
> can
> > think about a generalized signal for search providers that support
> > instant-extended later.
> > 
> > In short espv=1 has to stay, and this code should check for its presence,
not
> > key off extractability.
> 
> QQ: Should I check for espv=1 for search term replacement too? (I do for the
> moment.)
> 
> Done.

That's not quite true.  I believe you're referring to
|ToolbarModel::TryToExtractSearchTermsFromURL|, right?  That checks the
--enable-instant-extended-api flag, but does not explicitly check for espv=1.

That is the correct solution.

[beaudoin, 2012-10-02 18:49:22]

On 2012/10/02 18:14:13, dhollowa wrote:
> LGTM.  Thanks for persevering!
> 
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
> File chrome/browser/google/google_util.h (left):
> 
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
> chrome/browser/google/google_util.h:89: bool
> IsInstantExtendedAPIGoogleSearchUrl(const std::string& url);
> On 2012/10/02 17:43:29, beaudoin wrote:
> > On 2012/09/28 18:06:29, dhollowa wrote:
> > > This should stay and continue to be used by search_tab_helper.cc. 
> Extracting
> > > search terms is orthogonal to enabling the instant-extended experience on
> the
> > > search-results page.
> > 
> > IMHO it's fishy to have two different mechanisms for detecting Google Search
> > URLs, one relying on a hardcoded procedure (this one) and another
> parameterized
> > by the TemplateURL data.
> > 
> > This is a deeper issue though, so reverting in that CL.
> > 
> > Done.
> > 
> 
> Yes, ideally search term extraction is completely independent of
> extended-instant.  For now though we're governing them with a single flag. 
The
> search_tab_helper code has everything to do with extended-instant and nothing
to
> do with search term extraction.  So it is correct that search_tab_helper
> continue to use this method, but that GetSearchTermsFromGoogleSearchURL
doesn't
> (and is gone now).
> 
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
> File chrome/browser/ui/search/search_tab_helper.cc (right):
> 
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
> chrome/browser/ui/search/search_tab_helper.cc:169: else if
> (CanExtractSearchTerms(url))
> On 2012/10/02 17:43:29, beaudoin wrote:
> > On 2012/09/28 18:06:29, dhollowa wrote:
> > > Instant extended is not equivalent to search term extractability.  We
should
> > > keep |google_util::IsInstantExtendedAPIGoogleSearchUrl| for the moment. 
We
> > can
> > > think about a generalized signal for search providers that support
> > > instant-extended later.
> > > 
> > > In short espv=1 has to stay, and this code should check for its presence,
> not
> > > key off extractability.
> > 
> > QQ: Should I check for espv=1 for search term replacement too? (I do for the
> > moment.)
> > 
> > Done.
> 
> That's not quite true.  I believe you're referring to
> |ToolbarModel::TryToExtractSearchTermsFromURL|, right?  That checks the
> --enable-instant-extended-api flag, but does not explicitly check for espv=1.
> 
> That is the correct solution.

I'm talking about 
  TemplateURL::ExtractSearchTermsFromInstantExtendedURL

It first checks for espv=1 and fails early if it's not found. This was done to
answer dominich@ comment #6 (and also some comments in the off-CL thread). I
find it a bit smelly to check for espv=1 there because it assumes every search
engine knows not to be scared by that parameter. If you want me to drop it I'd
be happy to.

[dhollowa, 2012-10-02 19:21:02]

On 2012/10/02 18:49:22, beaudoin wrote:
> On 2012/10/02 18:14:13, dhollowa wrote:
> > LGTM.  Thanks for persevering!
> > 
> >
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
> > File chrome/browser/google/google_util.h (left):
> > 
> >
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/google/goog...
> > chrome/browser/google/google_util.h:89: bool
> > IsInstantExtendedAPIGoogleSearchUrl(const std::string& url);
> > On 2012/10/02 17:43:29, beaudoin wrote:
> > > On 2012/09/28 18:06:29, dhollowa wrote:
> > > > This should stay and continue to be used by search_tab_helper.cc. 
> > Extracting
> > > > search terms is orthogonal to enabling the instant-extended experience
on
> > the
> > > > search-results page.
> > > 
> > > IMHO it's fishy to have two different mechanisms for detecting Google
Search
> > > URLs, one relying on a hardcoded procedure (this one) and another
> > parameterized
> > > by the TemplateURL data.
> > > 
> > > This is a deeper issue though, so reverting in that CL.
> > > 
> > > Done.
> > > 
> > 
> > Yes, ideally search term extraction is completely independent of
> > extended-instant.  For now though we're governing them with a single flag. 
> The
> > search_tab_helper code has everything to do with extended-instant and
nothing
> to
> > do with search term extraction.  So it is correct that search_tab_helper
> > continue to use this method, but that GetSearchTermsFromGoogleSearchURL
> doesn't
> > (and is gone now).
> > 
> >
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
> > File chrome/browser/ui/search/search_tab_helper.cc (right):
> > 
> >
>
http://codereview.chromium.org/10908226/diff/38002/chrome/browser/ui/search/s...
> > chrome/browser/ui/search/search_tab_helper.cc:169: else if
> > (CanExtractSearchTerms(url))
> > On 2012/10/02 17:43:29, beaudoin wrote:
> > > On 2012/09/28 18:06:29, dhollowa wrote:
> > > > Instant extended is not equivalent to search term extractability.  We
> should
> > > > keep |google_util::IsInstantExtendedAPIGoogleSearchUrl| for the moment. 
> We
> > > can
> > > > think about a generalized signal for search providers that support
> > > > instant-extended later.
> > > > 
> > > > In short espv=1 has to stay, and this code should check for its
presence,
> > not
> > > > key off extractability.
> > > 
> > > QQ: Should I check for espv=1 for search term replacement too? (I do for
the
> > > moment.)
> > > 
> > > Done.
> > 
> > That's not quite true.  I believe you're referring to
> > |ToolbarModel::TryToExtractSearchTermsFromURL|, right?  That checks the
> > --enable-instant-extended-api flag, but does not explicitly check for
espv=1.
> > 
> > That is the correct solution.
> 
> I'm talking about 
>   TemplateURL::ExtractSearchTermsFromInstantExtendedURL
> 
> It first checks for espv=1 and fails early if it's not found. This was done to
> answer dominich@ comment #6 (and also some comments in the off-CL thread). I
> find it a bit smelly to check for espv=1 there because it assumes every search
> engine knows not to be scared by that parameter. If you want me to drop it I'd
> be happy to.

Ya, that is wrong.  The only test should be (1) IsInstantExtendedAPIEnabled (aka
the feature flag), (2) that we're extracting from the default search provider.

I.e. if you enabled instant-extended and switch default search provider to
non-google the search term extraction stuff will work but the ntp changes,
instant, etc will not work.

[beaudoin, 2012-10-02 20:25:27]

On 2012/10/02 19:21:02, dhollowa wrote:
> Ya, that is wrong.  The only test should be (1) IsInstantExtendedAPIEnabled
(aka
> the feature flag), (2) that we're extracting from the default search provider.
> 
> I.e. if you enabled instant-extended and switch default search provider to
> non-google the search term extraction stuff will work but the ntp changes,
> instant, etc will not work.

Done. Update template_url.cc and related unit/browser tests.

[dhollowa, 2012-10-02 20:38:10]

On 2012/10/02 20:25:27, beaudoin wrote:
> On 2012/10/02 19:21:02, dhollowa wrote:
> > Ya, that is wrong.  The only test should be (1) IsInstantExtendedAPIEnabled
> (aka
> > the feature flag), (2) that we're extracting from the default search
provider.
> > 
> > I.e. if you enabled instant-extended and switch default search provider to
> > non-google the search term extraction stuff will work but the ntp changes,
> > instant, etc will not work.
> 
> Done. Update template_url.cc and related unit/browser tests.

Thanks for pointing this out.  SLGTM.

[Peter Kasting, 2012-10-02 21:47:59]

I finally stopped putting it off and reviewed this thing :).  Luckily the
previous review passes took care of most of the critical stuff, leaving me with
mostly nits.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:611: const
char* const search_url = "http://test.com/search?t={searchTerms}";
Nit: Any way we can factor out some of this to a helper function?  It looks like
we repeat most of this big policy-creation block several times.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (left):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:618: 
Nit: Don't remove this blank line

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:133: DCHECK(type_ != INDEXED);
Nit: DCHECK_NE(INDEXED, type);

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:146: DCHECK(index_in_owner_ >= 0L
&& index_in_owner_ < owner_->URLCount());
Nit: The >= 0 check is nonsensical since size_t is unsigned; just do:

  DCHECK_LT(index_in_owner_, owner_->URLCount());

(2 places)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:424: 
Nit: Unnecessary blank line

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:426: const GURL& url, string16*
search_terms) const {
Nit: One arg per line, first arg on first line:

bool TemplateURLRef::ExtractSearchTermsFromURL(const GURL& url,
                                               string16* search_terms) const {

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:437: // See crbug/139176
Let's not plan to do this.  Just remove this TODO.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:442: // Scheme, host, path and
port must match.
Is it feasible (maybe as a followup change) to support parsing URLs like
"http://foo/{searchTerms}/"?

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:451: std::string params;
Nit: Simpler:

  const std::string& params(
      (search_term_key_location_ == url_parse::Parsed::QUERY) ?
          url.query() : url.ref());

We use this pattern a lot with 2-value enums in the infobar code; there's no
real need to NOTREACHED the existence of a third value.  (Even if there was, we
could do it with a conditional instead of a switch and still save code.)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:646: // query.
Ugh, I don't like hardcoding this.  We don't need to, either, if we ensure that
URLs with more than one "{searchTerms}" are rejected.  We may already be doing
that somewhere else and preventing these being created -- if not it seems
reasonable to do so, or at the very least to just fail here.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:787: DCHECK(!url().empty());
Nit: This is guaranteed at all times, you need not DCHECK it (2 places)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:796: if (index <
data_.alternate_urls.size())
Nit: Simpler:

  return (index < data_.alternate_urls.size()) ?
      data_.alternate_urls[index] : url();

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:45: // |index_in_owner_| should be
used instead.
Do you think it'd be clearer to do something like this:

  enum Type {
    SEARCH = -3,
    SUGGEST = -2,
    INSTANT = -1,
    // All nonnegative values represent an alternate URL, obtainable by calling
    // GetURL(value) on the owner.
  };

This way we wouldn't have two fields, one of which is only valid sometimes, to
represent the type of URL we have.  OTOH, this might force us to add casts in
some places.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:139: string16* search_terms) const;
Nit: Is it ever valid to return an empty string for |search_terms|?  If not, we
can simply return the search terms by value and callers can check for them being
non-empty to see if the extraction succeeded.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:352:
FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, SerializeAlternateURLs);
This test doesn't seem to exist?

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:459: // a search term is present
both in the query and the ref, we can prioritize
Nit: Don't hardcode the idea that ref > query into the comments here.  How about
something like this instead:

  // Gets the search URL at the given index.  The alternate URLs, if any, are
  // numbered starting at 0, and the primary search URL follows.  This is used
  // to decode the search term given a search URL (see
  // ExtractSearchTermsFromURL()) and allows us to specify a priority order for
  // matching by listing alternate URLs in an appropriate order (the URL at
  // index 0 is treated as the highest priority).  For example, if a TemplateURL
  // has alternate URL "http://foo/#q={searchTerms}" and search URL
  // "http://foo/?q={searchTerms}", and the URL to be decoded is
  // "http://foo/?q=a#q=b", the alternate URL will match first and the decoded
  // search term will be "b".

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:465: // |search_terms| if no search
terms can be matched.
Nit: Add note about the matching priority, perhaps by simply referencing the
comments above.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3520: Profile*
profile,
Nit: Why did you change the indenting here?  The old way was fine.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3543: LOG(ERROR)
<< "Invalid alternate url, expected a string.";
Nit: Rather than log, let's just DCHECK (or not check at all) that GetString()
succeeds.

We should also DCHECK similarly that the string is non-empty.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3617: LOG(ERROR)
<< "Cannot parse alternate_urls as JSON list.";
Nit: Again, let's either DCHECK or not check.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1306:
se_specifics->add_alternate_urls(turl.alternate_urls()[i]);
Would be nice if we could just call a
"set_alternate_urls(turl.alternate_urls())" function.  Is that possible?

Similar question applies to reading the value back out.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1662: ListValue
alternate_urls_value;
Nit: Move this up to with the other temps above, name it |alternate_urls|, and
populate it inside the "if (t_url)" conditional with everything else

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1728: std::string
alternate_url;
Nit: Declare inside loop (the efficiency loss doesn't matter)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1731:
data.alternate_urls.push_back(alternate_url);
Isn't there some helper already that can return a ListValue as a vector?

If not we should consider adding helpers to the Value class to convert
ListValues to/from vectors.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:204: string16 result;
Nit: Declare this at the very bottom and return std::string() for the failure
cases before then.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:213: if (!template_url_service)
Can this ever be NULL?

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (left):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:67: const char
kKeywordColumnsConcatenated[] = "id || short_name || keyword || "
Nit: Why rewrap this value (and all the subsequent lines)?

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:416: // code relying on
|kKeywordColumnsConcatenated|.
Why?  We have other VARCHAR columns that don't default to the empty string.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:425: !SignBackup(47))
We should only migrate and sign the backup if it was valid to begin with;
otherwise we should drop it.  See the version 45 migration code above.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:504: const char*
keywords_columns_concatenated = kKeywordColumnsConcatenated;
Nit: Probably makes sense to have helper functions to get the column strings for
the current version.  (2 places)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2214: // if the former
is smaller.
This comment is wrong

[beaudoin, 2012-10-03 22:46:52]

Summary of changes:
- Fixed or discussed all of PK's nits
- Fixed broken tests in ToolbarModelTest
- Refactored ConfigurationPolicyPrefStoreDefaultSearchTest to extract some
cut-and-pasted code.
- Refactored KeywordTable to remove error-prone redundant column lists in static
strings
- KeywordTable migration now drops the backup table if the signature is wrong.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:611: const
char* const search_url = "http://test.com/search?t={searchTerms}";
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Any way we can factor out some of this to a helper function?  It looks
like
> we repeat most of this big policy-creation block several times.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (left):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:618: 
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Don't remove this blank line

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:133: DCHECK(type_ != INDEXED);
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: DCHECK_NE(INDEXED, type);

Kept type_ for uniformity. Please explain if there are any good reason to change
to the local var, I'll change them everywhere.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:146: DCHECK(index_in_owner_ >= 0L
&& index_in_owner_ < owner_->URLCount());
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: The >= 0 check is nonsensical since size_t is unsigned; just do:
> 
>   DCHECK_LT(index_in_owner_, owner_->URLCount());
> 
> (2 places)

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:424: 
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Unnecessary blank line

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:426: const GURL& url, string16*
search_terms) const {
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: One arg per line, first arg on first line:
> 
> bool TemplateURLRef::ExtractSearchTermsFromURL(const GURL& url,
>                                                string16* search_terms) const {

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:437: // See crbug/139176
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Let's not plan to do this.  Just remove this TODO.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:442: // Scheme, host, path and
port must match.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Is it feasible (maybe as a followup change) to support parsing URLs like
> "http://foo/{searchTerms}/"?

Entered a TODO and a bug for this: crbug.com/153798

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:451: std::string params;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Simpler:
> 
>   const std::string& params(
>       (search_term_key_location_ == url_parse::Parsed::QUERY) ?
>           url.query() : url.ref());
> 
> We use this pattern a lot with 2-value enums in the infobar code; there's no
> real need to NOTREACHED the existence of a third value.  (Even if there was,
we
> could do it with a conditional instead of a switch and still save code.)

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:646: // query.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Ugh, I don't like hardcoding this.  We don't need to, either, if we ensure
that
> URLs with more than one "{searchTerms}" are rejected.  We may already be doing
> that somewhere else and preventing these being created -- if not it seems
> reasonable to do so, or at the very least to just fail here.

Removed the comment. The goal here is just to find {searchTerms} either in the
ref or the query. As you say, the order in which we check should not matter as
long as there is a single {searchTerms}.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:787: DCHECK(!url().empty());
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: This is guaranteed at all times, you need not DCHECK it (2 places)

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:796: if (index <
data_.alternate_urls.size())
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Simpler:
> 
>   return (index < data_.alternate_urls.size()) ?
>       data_.alternate_urls[index] : url();

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:45: // |index_in_owner_| should be
used instead.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Do you think it'd be clearer to do something like this:
> 
>   enum Type {
>     SEARCH = -3,
>     SUGGEST = -2,
>     INSTANT = -1,
>     // All nonnegative values represent an alternate URL, obtainable by
calling
>     // GetURL(value) on the owner.
>   };
> 
> This way we wouldn't have two fields, one of which is only valid sometimes, to
> represent the type of URL we have.  OTOH, this might force us to add casts in
> some places.

I'm not a fan of using enums as ints but it's true that the extra field is
annoying. I won't do it this time around but if you and/or other reviewers feel
strongly about this I'll change it.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:139: string16* search_terms) const;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Is it ever valid to return an empty string for |search_terms|?  If not,
we
> can simply return the search terms by value and callers can check for them
being
> non-empty to see if the extraction succeeded.

This is done to identify the case:
 "q=" was found but nothing after it.
Allowing us to fail on:
  http://google.com/?q=foo#q=
as was requested by a previous reviewer.

For a bit more details on why this works and why we need both the bool and the
return string, see around line 806 in template_url.cc.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:352:
FRIEND_TEST_ALL_PREFIXES(TemplateURLTest, SerializeAlternateURLs);
On 2012/10/02 21:47:59, Peter Kasting wrote:
> This test doesn't seem to exist?

Good catch! (Would make for a nice presubmit check. :))

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:459: // a search term is present
both in the query and the ref, we can prioritize
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Don't hardcode the idea that ref > query into the comments here.  How
about
> something like this instead:
> 
>   // Gets the search URL at the given index.  The alternate URLs, if any, are
>   // numbered starting at 0, and the primary search URL follows.  This is used
>   // to decode the search term given a search URL (see
>   // ExtractSearchTermsFromURL()) and allows us to specify a priority order
for
>   // matching by listing alternate URLs in an appropriate order (the URL at
>   // index 0 is treated as the highest priority).  For example, if a
TemplateURL
>   // has alternate URL "http://foo/#q={searchTerms}" and search URL
>   // "http://foo/?q={searchTerms}", and the URL to be decoded is
>   // "http://foo/?q=a#q=b", the alternate URL will match first and the decoded
>   // search term will be "b".

Shuffled your proposal a bit between this and ExtractSearchTermsFromURL.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:465: // |search_terms| if no search
terms can be matched.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Add note about the matching priority, perhaps by simply referencing the
> comments above.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3520: Profile*
profile,
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Why did you change the indenting here?  The old way was fine.

I was using base::ListValue at some point which went over 80c. Forgot to change
it back. :)

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3543: LOG(ERROR)
<< "Invalid alternate url, expected a string.";
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Rather than log, let's just DCHECK (or not check at all) that GetString()
> succeeds.
> 
> We should also DCHECK similarly that the string is non-empty.

No longer checking for GetString. The check would require an extra temp variable
or an if() and a NOTREACHED(). Added a DCHECK for a non-empty string.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3617: LOG(ERROR)
<< "Cannot parse alternate_urls as JSON list.";
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Again, let's either DCHECK or not check.

No longer checking for GetAsList. The check would require an extra temp variable
or an if() and a NOTREACHED(). 

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1306:
se_specifics->add_alternate_urls(turl.alternate_urls()[i]);
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Would be nice if we could just call a
> "set_alternate_urls(turl.alternate_urls())" function.  Is that possible?
> 
> Similar question applies to reading the value back out.

Unfortunately, here is the complete interface generated for alternate_urls in
se_specifics:

  // repeated string alternate_urls = 19;
  inline int alternate_urls_size() const;
  inline void clear_alternate_urls();
  static const int kAlternateUrlsFieldNumber = 19;
  inline const ::std::string& alternate_urls(int index) const;
  inline ::std::string* mutable_alternate_urls(int index);
  inline void set_alternate_urls(int index, const ::std::string& value);
  inline void set_alternate_urls(int index, const char* value);
  inline void set_alternate_urls(int index, const char* value, size_t size);
  inline ::std::string* add_alternate_urls();
  inline void add_alternate_urls(const ::std::string& value);
  inline void add_alternate_urls(const char* value);
  inline void add_alternate_urls(const char* value, size_t size);
  inline const ::google::protobuf::RepeatedPtrField< ::std::string>&
alternate_urls() const;
  inline ::google::protobuf::RepeatedPtrField< ::std::string>*
mutable_alternate_urls();

And RepeatedPtrField does not seem to allow direct assignment from STL vectors
either...

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1662: ListValue
alternate_urls_value;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Move this up to with the other temps above, name it |alternate_urls|, and
> populate it inside the "if (t_url)" conditional with everything else

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1728: std::string
alternate_url;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Declare inside loop (the efficiency loss doesn't matter)

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1731:
data.alternate_urls.push_back(alternate_url);
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Isn't there some helper already that can return a ListValue as a vector?
> 
> If not we should consider adding helpers to the Value class to convert
> ListValues to/from vectors.

I think the problem is that ListValue does not guarantee it contains only
strings. (Because there is an AppendStrings() method.)

I could create base/value_utils.* for such specialized ListValue (or maybe that
is what base/value_conversion.* is for?)

In all cases, unless you feel strongly about this, I suggest we wrap that CL.
I'm happy to follow-up quickly with this afterward.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:204: string16 result;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Declare this at the very bottom and return std::string() for the failure
> cases before then.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model.cc:213: if (!template_url_service)
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Can this ever be NULL?

Upon checking, it is created if it does not already exist, removed the check.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (left):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:67: const char
kKeywordColumnsConcatenated[] = "id || short_name || keyword || "
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Why rewrap this value (and all the subsequent lines)?

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:416: // code relying on
|kKeywordColumnsConcatenated|.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Why?  We have other VARCHAR columns that don't default to the empty string.

It's needed for the migration. When adding the column to every existing entry in
the table, if there is no default then NULL will be in there and it breaks the
table signing code. (Took me forever to debug this one!)

We do not need the default after the migration because we will always put a JSON
list in there (potentially empty).

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:425: !SignBackup(47))
On 2012/10/02 21:47:59, Peter Kasting wrote:
> We should only migrate and sign the backup if it was valid to begin with;
> otherwise we should drop it.  See the version 45 migration code above.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:504: const char*
keywords_columns_concatenated = kKeywordColumnsConcatenated;
On 2012/10/02 21:47:59, Peter Kasting wrote:
> Nit: Probably makes sense to have helper functions to get the column strings
for
> the current version.  (2 places)

Refactored this in a major way, got rid of all the error prone cut-and-paste of
column names.

Done.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2214: // if the former
is smaller.
On 2012/10/02 21:47:59, Peter Kasting wrote:
> This comment is wrong

Done.

[Peter Kasting, 2012-10-03 22:59:32]

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:133: DCHECK(type_ != INDEXED);
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > Nit: DCHECK_NE(INDEXED, type);
> 
> Kept type_ for uniformity. Please explain if there are any good reason to
change
> to the local var, I'll change them everywhere.

Nope, it was a typo, I intended |type_| :)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:45: // |index_in_owner_| should be
used instead.
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > Do you think it'd be clearer to do something like this:
> > 
> >   enum Type {
> >     SEARCH = -3,
> >     SUGGEST = -2,
> >     INSTANT = -1,
> >     // All nonnegative values represent an alternate URL, obtainable by
> calling
> >     // GetURL(value) on the owner.
> >   };
> > 
> > This way we wouldn't have two fields, one of which is only valid sometimes,
to
> > represent the type of URL we have.  OTOH, this might force us to add casts
in
> > some places.
> 
> I'm not a fan of using enums as ints but it's true that the extra field is
> annoying. I won't do it this time around but if you and/or other reviewers
feel
> strongly about this I'll change it.

I don't feel strongly.  Both ways have issues.  My hope was that changing to
this would shorten the code and not introduce many casts, in which case I think
it would be an improvement.  But I didn't check to see if that was actually the
case :)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:139: string16* search_terms) const;
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > Nit: Is it ever valid to return an empty string for |search_terms|?  If not,
> we
> > can simply return the search terms by value and callers can check for them
> being
> > non-empty to see if the extraction succeeded.
> 
> This is done to identify the case:
>  "q=" was found but nothing after it.
> Allowing us to fail on:
>   http://google.com/?q=foo#q=
> as was requested by a previous reviewer.
> 
> For a bit more details on why this works and why we need both the bool and the
> return string, see around line 806 in template_url.cc.

Ah, I get it.  Makes  sense, but I suggest adding more comments here about
whether an empty string would be valid to extract (it is) as well as at that
callsite to explain why we want to detect empty strings and handle them
differently than we would a failed extraction.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_service.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_service.cc:1731:
data.alternate_urls.push_back(alternate_url);
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > Isn't there some helper already that can return a ListValue as a vector?
> > 
> > If not we should consider adding helpers to the Value class to convert
> > ListValues to/from vectors.
> 
> I think the problem is that ListValue does not guarantee it contains only
> strings. (Because there is an AppendStrings() method.)
> 
> I could create base/value_utils.* for such specialized ListValue (or maybe
that
> is what base/value_conversion.* is for?)
> 
> In all cases, unless you feel strongly about this, I suggest we wrap that CL.
> I'm happy to follow-up quickly with this afterward.

It's fine to not address this in this CL.  We may end up doing nothing here.

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:416: // code relying on
|kKeywordColumnsConcatenated|.
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > Why?  We have other VARCHAR columns that don't default to the empty string.
> 
> It's needed for the migration. When adding the column to every existing entry
in
> the table, if there is no default then NULL will be in there and it breaks the
> table signing code. (Took me forever to debug this one!)
> 
> We do not need the default after the migration because we will always put a
JSON
> list in there (potentially empty).

So doesn't that mean that all our other VARCHARs should really be DEFAULT ''
too?  Since if we were to ever insert a row without specifying one, the embedded
NUL would break things?  This will also establish a pattern so that if others
add more string columns, they'll be less likely to trip on this.

Alternately, can we write extraction code such that embedded NULs are handled
correctly?  Maybe that's not possible, or maybe we could simply strip all \0s
from the string?

[beaudoin, 2012-10-04 00:03:58]

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.h (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:45: // |index_in_owner_| should be
used instead.
On 2012/10/03 22:59:33, Peter Kasting wrote:
> On 2012/10/03 22:46:52, beaudoin wrote:
> > On 2012/10/02 21:47:59, Peter Kasting wrote:
> > > Do you think it'd be clearer to do something like this:
> > > 
> > >   enum Type {
> > >     SEARCH = -3,
> > >     SUGGEST = -2,
> > >     INSTANT = -1,
> > >     // All nonnegative values represent an alternate URL, obtainable by
> > calling
> > >     // GetURL(value) on the owner.
> > >   };
> > > 
> > > This way we wouldn't have two fields, one of which is only valid
sometimes,
> to
> > > represent the type of URL we have.  OTOH, this might force us to add casts
> in
> > > some places.
> > 
> > I'm not a fan of using enums as ints but it's true that the extra field is
> > annoying. I won't do it this time around but if you and/or other reviewers
> feel
> > strongly about this I'll change it.
> 
> I don't feel strongly.  Both ways have issues.  My hope was that changing to
> this would shorten the code and not introduce many casts, in which case I
think
> it would be an improvement.  But I didn't check to see if that was actually
the
> case :)

I confess I did not actually implement it to see how much it would simplify the
code. :)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.h:139: string16* search_terms) const;
On 2012/10/03 22:59:33, Peter Kasting wrote:
> On 2012/10/03 22:46:52, beaudoin wrote:
> > On 2012/10/02 21:47:59, Peter Kasting wrote:
> > > Nit: Is it ever valid to return an empty string for |search_terms|?  If
not,
> > we
> > > can simply return the search terms by value and callers can check for them
> > being
> > > non-empty to see if the extraction succeeded.
> > 
> > This is done to identify the case:
> >  "q=" was found but nothing after it.
> > Allowing us to fail on:
> >   http://google.com/?q=foo#q=
> > as was requested by a previous reviewer.
> > 
> > For a bit more details on why this works and why we need both the bool and
the
> > return string, see around line 806 in template_url.cc.
> 
> Ah, I get it.  Makes  sense, but I suggest adding more comments here about
> whether an empty string would be valid to extract (it is) as well as at that
> callsite to explain why we want to detect empty strings and handle them
> differently than we would a failed extraction.

Done. (Patch Set 12)

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:416: // code relying on
|kKeywordColumnsConcatenated|.
On 2012/10/03 22:59:33, Peter Kasting wrote:
> On 2012/10/03 22:46:52, beaudoin wrote:
> > On 2012/10/02 21:47:59, Peter Kasting wrote:
> > > Why?  We have other VARCHAR columns that don't default to the empty
string.
> > 
> > It's needed for the migration. When adding the column to every existing
entry
> in
> > the table, if there is no default then NULL will be in there and it breaks
the
> > table signing code. (Took me forever to debug this one!)
> > 
> > We do not need the default after the migration because we will always put a
> JSON
> > list in there (potentially empty).
> 
> So doesn't that mean that all our other VARCHARs should really be DEFAULT ''
> too?  Since if we were to ever insert a row without specifying one, the
embedded
> NUL would break things?  This will also establish a pattern so that if others
> add more string columns, they'll be less likely to trip on this.
> 
> Alternately, can we write extraction code such that embedded NULs are handled
> correctly?  Maybe that's not possible, or maybe we could simply strip all \0s
> from the string?

I think the fact that any update to the table (save for ALTER TABLE like this)
goes through BindURLToStatement is a good enough guarantee that we wont add a
NUL in there.

I tried to play with the alternate solution however the way we generate the
string for the entire table, in GetTableContents(), is by using a SELECT query
with all the columns concatenated by " || ". When you iterate through the
results of that query, however, it appears that s.ColumnString(0) will return an
empty string if one of the concatenated column had NUL. I did not dig deeper in
sql/statement.cc to see if we can change that behavior.

[Joao da Silva, 2012-10-04 14:45:37]

Still LGTM. Thanks for the new test!

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:732:
base::Value::CreateStringValue(bad_search_url));
nit: indent

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (right):

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:636:
IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) {
Very cool test, thanks for writing it!

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:656: EXPECT_NE(kAlternateURL1,
default_search->alternate_urls()[1]);
Is the default_search always guaranteed to have at least 2 alternate URLs?

[beaudoin, 2012-10-04 15:39:29]

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:732:
base::Value::CreateStringValue(bad_search_url));
On 2012/10/04 14:45:37, Joao da Silva wrote:
> nit: indent

Done.

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/poli...
File chrome/browser/policy/policy_browsertest.cc (right):

http://codereview.chromium.org/10908226/diff/73002/chrome/browser/policy/poli...
chrome/browser/policy/policy_browsertest.cc:656: EXPECT_NE(kAlternateURL1,
default_search->alternate_urls()[1]);
On 2012/10/04 14:45:37, Joao da Silva wrote:
> Is the default_search always guaranteed to have at least 2 alternate URLs?

No. I just wanted to have a more thorough test. Want me to test other
configurations?

[Peter Kasting, 2012-10-04 19:32:43]

LGTM

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2214: // if the former
is smaller.
On 2012/10/03 22:46:52, beaudoin wrote:
> On 2012/10/02 21:47:59, Peter Kasting wrote:
> > This comment is wrong
> 
> Done.

You fixed the wrong comment.  The one that used to be on the v45 migrations
function was right.  The one you have there now should be on this function
instead and refer to v47.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:560: 
Nit: Extra blank line

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:635: if (!url.ref().empty())
Nit: This might be slightly clearer if we move the empty() check to
FindSearchTermsKey(), and have that function return the string it finds instead
of setting a member directly.  We can also then avoid any appearance of
favoritism for the ref over the query.  Something like this:

  ...
  std::string query_key(FindSearchTermsKey(url.query()));
  std::string ref_key(FindSearchTermsKey(url.ref()));
  if (query_key.empty() == ref_key.empty())
    return;  // No key or multiple keys found.  We only handle having one key.
  search_term_key_ = query_key.empty() ? ref_key : query_key;
  search_term_key_location_ = query_key.empty() ?
      url_parse::Parsed::REF : url_parse::Parsed::QUERY;
  host_ = url.host();
  path_ = url.path();
}

std::string TemplateURLRef::FindSearchTermsKey(
    const std::string& params) const {
  if (params.empty())
    return std::string();
  while (...) {
    ...
    return params.substr(key.begin, key.len);
    ...
  }
  return std::string();
}

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:799: // patterns. This means that
given patterns
Nit: This comment helps.  I might add even more justification:

      ...
      // calling ExtractSearchTermsFromURL() on "http://foo/?q=bar#q=' will
      // return false.  This is important for at least Google, where such URLs
      // are invalid.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3531: std::string
alternate_url;
Nit: Move this temp inside the loop

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:77: return JoinString(columns,
std::string(concatenated ? " || " : ", "));
Good idea!  Wish I'd thought of it back when I was writing all these dumb string
constants.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.h:156: // version of the table.
Nit: I'd be OK with making this public if it allows us to remove the FRIEND_TEST
declarations for all the migration tests above.

[beaudoin, 2012-10-04 23:10:44]

Answered this last round of comments. It's coming to an end! :)

OWNERS review needed (please suggest someone if you can't make it):
@lipalani : 
  sync/*
@sky : 
  chrome/common/*
  chrome/browser/webdata/*

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_database_migration_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/60001/chrome/browser/webdata/web...
chrome/browser/webdata/web_database_migration_unittest.cc:2214: // if the former
is smaller.
On 2012/10/04 19:32:43, Peter Kasting wrote:
> On 2012/10/03 22:46:52, beaudoin wrote:
> > On 2012/10/02 21:47:59, Peter Kasting wrote:
> > > This comment is wrong
> > 
> > Done.
> 
> You fixed the wrong comment.  The one that used to be on the v45 migrations
> function was right.  The one you have there now should be on this function
> instead and refer to v47.

Note to self: Next time go to bed instead of trying to rush fixes. ;)

Done.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/policy/conf...
File chrome/browser/policy/configuration_policy_pref_store_unittest.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/policy/conf...
chrome/browser/policy/configuration_policy_pref_store_unittest.cc:560: 
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Nit: Extra blank line

Done.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:635: if (!url.ref().empty())
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Nit: This might be slightly clearer if we move the empty() check to
> FindSearchTermsKey(), and have that function return the string it finds
instead
> of setting a member directly.  We can also then avoid any appearance of
> favoritism for the ref over the query.  Something like this:
> 
>   ...
>   std::string query_key(FindSearchTermsKey(url.query()));
>   std::string ref_key(FindSearchTermsKey(url.ref()));
>   if (query_key.empty() == ref_key.empty())
>     return;  // No key or multiple keys found.  We only handle having one key.
>   search_term_key_ = query_key.empty() ? ref_key : query_key;
>   search_term_key_location_ = query_key.empty() ?
>       url_parse::Parsed::REF : url_parse::Parsed::QUERY;
>   host_ = url.host();
>   path_ = url.path();
> }
> 
> std::string TemplateURLRef::FindSearchTermsKey(
>     const std::string& params) const {
>   if (params.empty())
>     return std::string();
>   while (...) {
>     ...
>     return params.substr(key.begin, key.len);
>     ...
>   }
>   return std::string();
> }

Much cleaner!

Moved FindSearchTermsKey to the anonymous namespace as it was no longer
dependent on anything in the object.

Done.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url.cc:799: // patterns. This means that
given patterns
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Nit: This comment helps.  I might add even more justification:
> 
>       ...
>       // calling ExtractSearchTermsFromURL() on "http://foo/?q=bar#q=' will
>       // return false.  This is important for at least Google, where such URLs
>       // are invalid.

Done.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
File chrome/browser/search_engines/template_url_prepopulate_data.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/search_engi...
chrome/browser/search_engines/template_url_prepopulate_data.cc:3531: std::string
alternate_url;
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Nit: Move this temp inside the loop

Done.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:77: return JoinString(columns,
std::string(concatenated ? " || " : ", "));
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Good idea!  Wish I'd thought of it back when I was writing all these dumb
string
> constants.

:)

Ack.

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.h (right):

http://codereview.chromium.org/10908226/diff/78003/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.h:156: // version of the table.
On 2012/10/04 19:32:43, Peter Kasting wrote:
> Nit: I'd be OK with making this public if it allows us to remove the
FRIEND_TEST
> declarations for all the migration tests above.

Done.

[sky, 2012-10-05 15:31:32]

http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:89: // Serialize |alternate_urls| to
JSON.
Serializing to JSON is pretty heavy handed. Can't you use a separator between
the urls and write as a string?

[beaudoin1, 2012-10-05 17:04:11]

http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
File chrome/browser/webdata/keyword_table.cc (right):

http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
chrome/browser/webdata/keyword_table.cc:89: // Serialize |alternate_urls| to
JSON.
On 2012/10/05 15:31:32, sky wrote:
> Serializing to JSON is pretty heavy handed. Can't you use a separator between
> the urls and write as a string?

It was the initial implementation. :) See the rest of the thread for arguments
by reviewers on why a CSV-style serialization is not good enough. (ie. The urls
can contain commas and all sort of other characters.) Short of a homebrewed
escaping mechanism, JSON seems like the best option. We also plan to move this
list to an alternate table at some point which will make this irrelevant.

[sky, 2012-10-05 17:14:14]

Can it contain \0 ?

  -Scott

On Fri, Oct 5, 2012 at 10:04 AM,  <[email protected]> wrote:
>
>
http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
> File chrome/browser/webdata/keyword_table.cc (right):
>
>
http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
> chrome/browser/webdata/keyword_table.cc:89: // Serialize
> |alternate_urls| to JSON.
> On 2012/10/05 15:31:32, sky wrote:
>>
>> Serializing to JSON is pretty heavy handed. Can't you use a separator
>
> between
>>
>> the urls and write as a string?
>
>
> It was the initial implementation. :) See the rest of the thread for
> arguments by reviewers on why a CSV-style serialization is not good
> enough. (ie. The urls can contain commas and all sort of other
> characters.) Short of a homebrewed escaping mechanism, JSON seems like
> the best option. We also plan to move this list to an alternate table at
> some point which will make this irrelevant.
>
> http://codereview.chromium.org/10908226/

[beaudoin1, 2012-10-05 17:30:14]

@pkasting: Would you be fine using \0 as a separator?

On 2012/10/05 17:14:14, sky wrote:
> Can it contain \0 ?
> 
>   -Scott
> 
> On Fri, Oct 5, 2012 at 10:04 AM,  <mailto:[email protected]> wrote:
> >
> >
>
http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
> > File chrome/browser/webdata/keyword_table.cc (right):
> >
> >
>
http://codereview.chromium.org/10908226/diff/83001/chrome/browser/webdata/key...
> > chrome/browser/webdata/keyword_table.cc:89: // Serialize
> > |alternate_urls| to JSON.
> > On 2012/10/05 15:31:32, sky wrote:
> >>
> >> Serializing to JSON is pretty heavy handed. Can't you use a separator
> >
> > between
> >>
> >> the urls and write as a string?
> >
> >
> > It was the initial implementation. :) See the rest of the thread for
> > arguments by reviewers on why a CSV-style serialization is not good
> > enough. (ie. The urls can contain commas and all sort of other
> > characters.) Short of a homebrewed escaping mechanism, JSON seems like
> > the best option. We also plan to move this list to an alternate table at
> > some point which will make this irrelevant.
> >
> > http://codereview.chromium.org/10908226/

[Peter Kasting, 2012-10-05 17:32:23]

On Fri, Oct 5, 2012 at 10:30 AM, <[email protected]> wrote:

> @pkasting: Would you be fine using \0 as a separator?


I would really like to leave as JSON.

I don't see how this will have a meaningful performance impact and I'm a
huge fan of not writing your own escaping.  Plus, I've seen URLs
(admittedly, only ones used to probe for security bugs) that contained \0.

PK

http://codereview.chromium.**org/10908226/<http://codereview.chromium.org/109...
>

[sky, 2012-10-05 20:37:56]

Fair enough, LGTM

[beaudoin, 2012-10-05 22:15:56]

+akalin, atwilson

I really need that LGTM for sync/  pretty please? :)

[akalin, 2012-10-05 23:01:41]

sync lgtm

[commit-bot: I haz the power, 2012-10-09 17:53:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10908226/75006

[commit-bot: I haz the power, 2012-10-09 18:54:25]

Change committed as 160884