A simple version of search term replacement for testing .

A simple version of search term replacement for testing. Google search URLs that support the extended instant API will be replaced in the Omnibox by the search terms.


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

[dominich, 2012-08-23 19:35:19]

sky: Please look over everything UI related
dhollowa: For the ui/cocoa changes
isherman: For the google/ changes

This is not intended to be a final solution for the feature, as noted in the
related bug. It enables us to test the feature (which is behind a flag
off-by-default) while we work on a design for the final version. See the second
related bug for that discussion.

[Ilya Sherman, 2012-08-23 19:50:51]

google/ changes LGTM.  Please update the CL description to explicitly name which
feature this is for.

http://codereview.chromium.org/10867038/diff/1/chrome/browser/google/google_u...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10867038/diff/1/chrome/browser/google/google_u...
chrome/browser/google/google_util.cc:146:
base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &key_value_pairs);
Optional nit: Did you consider using ExtractQueryKeyValue() from googleurl's
url_parse.h?

[dominich, 2012-08-23 20:45:36]

http://codereview.chromium.org/10867038/diff/1/chrome/browser/google/google_u...
File chrome/browser/google/google_util.cc (right):

http://codereview.chromium.org/10867038/diff/1/chrome/browser/google/google_u...
chrome/browser/google/google_util.cc:146:
base::SplitStringIntoKeyValuePairs(url.query(), '=', '&', &key_value_pairs);
On 2012/08/23 19:50:51, Ilya Sherman wrote:
> Optional nit: Did you consider using ExtractQueryKeyValue() from googleurl's
> url_parse.h?

Thanks for the pointer - that's much better.

[dhollowa, 2012-08-23 22:32:24]

Cocoa stuff LGTM.  A general question... how much of this is incremental steps
toward final solution versus temporary solution that will have to be backed out
eventually?  For the stuff that is temporary, it would be good to demarcate them
with TODO(dominich): crbug.com/xxxxx so they doesn't get lost.

[sky, 2012-08-23 22:44:26]

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.cc:203:
google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec())) {
Shouldn't this always be with the default search provider?

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:45: // Returns the text for the
current page's URL. This will have been formatted
nit: newline between 44/45

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:59: bool
WouldReplaceSearchURLWithSearchTerms() const;
Isn't this something like IsURLFromDefaultSearchProvider.

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:565:
model()->UpdatePermanentText(toolbar_model()->GetText(true));
Do you ever pass in false?

[dominich, 2012-08-24 15:40:18]

On 2012/08/23 22:32:24, dhollowa wrote:
> Cocoa stuff LGTM.  A general question... how much of this is incremental steps
> toward final solution versus temporary solution that will have to be backed
out
> eventually?  For the stuff that is temporary, it would be good to demarcate
them
> with TODO(dominich): crbug.com/xxxxx so they doesn't get lost.

The only thing that is temporary already is marked with a comment in
toolbar_model.cc. The rest will stay as-is.

[dominich, 2012-08-24 15:51:07]

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.cc:203:
google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec())) {
On 2012/08/23 22:44:26, sky wrote:
> Shouldn't this always be with the default search provider?

Eventually, yes. See the bugs referenced in the CL for more details.

For now, restricting it to a specific URL helps us test the idea and
implementation.

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:45: // Returns the text for the
current page's URL. This will have been formatted
On 2012/08/23 22:44:26, sky wrote:
> nit: newline between 44/45

Done.

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:59: bool
WouldReplaceSearchURLWithSearchTerms() const;
On 2012/08/23 22:44:26, sky wrote:
> Isn't this something like IsURLFromDefaultSearchProvider.

No. In this temporary version we're restricting to Google instant extended API
urls, which is not necessarily the default provider. In fact, the instant
extended API URL isn't even in the prepopulated search provider list (yet?).

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_win.cc (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_win.cc:565:
model()->UpdatePermanentText(toolbar_model()->GetText(true));
On 2012/08/23 22:44:26, sky wrote:
> Do you ever pass in false?

In this CL, no. In future CLs, probably as we'll want to surface the underlying
URL somehow. This is in this CL in an attempt to set up the toolbar model API to
be as close to the future final version as possible.

[sky, 2012-08-24 18:11:02]

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.cc (right):

http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.cc:203:
google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec())) {
On 2012/08/24 15:51:07, dominich wrote:
> On 2012/08/23 22:44:26, sky wrote:
> > Shouldn't this always be with the default search provider?
> 
> Eventually, yes. See the bugs referenced in the CL for more details.
> 
> For now, restricting it to a specific URL helps us test the idea and
> implementation.

Fair enough. Shouldn't you only do this if default search provider is google
though?

[dominich, 2012-08-24 18:14:38]

On 2012/08/24 18:11:02, sky wrote:
>
http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
> File chrome/browser/ui/toolbar/toolbar_model.cc (right):
> 
>
http://codereview.chromium.org/10867038/diff/7001/chrome/browser/ui/toolbar/t...
> chrome/browser/ui/toolbar/toolbar_model.cc:203:
> google_util::IsInstantExtendedAPIGoogleSearchUrl(url.spec())) {
> On 2012/08/24 15:51:07, dominich wrote:
> > On 2012/08/23 22:44:26, sky wrote:
> > > Shouldn't this always be with the default search provider?
> > 
> > Eventually, yes. See the bugs referenced in the CL for more details.
> > 
> > For now, restricting it to a specific URL helps us test the idea and
> > implementation.
> 
> Fair enough. Shouldn't you only do this if default search provider is google
> though?

I certainly can, but I'm not sure why that should be necessary. When we have the
full version, I can see that it would only apply to default search providers
(that support instant extended API, etc) but for testing it will be useful to
retain the default default search provider and trigger this for any other search
provider that matches the google instant extended api search url pattern.

That way, a tester/developer can set up a Google instant extended API-enabled
search provider and easily switch between that and the normal Google provider.

[sky, 2012-08-24 19:05:59]

Fair enough, LGTM

[commit-bot: I haz the power, 2012-08-24 19:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10867038/1013

[commit-bot: I haz the power, 2012-08-24 20:46:31]

Try job failure for 10867038-1013 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-24 20:49:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dominich@chromium.org/10867038/1013

[commit-bot: I haz the power, 2012-08-24 23:22:12]

Change committed as 153341

[sreeram, 2012-08-27 16:33:14]

On 2012/08/24 23:22:12, I haz the power (commit-bot) wrote:
> Change committed as 153341

It seems to me that this change doesn't extract search terms from the fragment
(ref) portion of the URL, which is where the search terms will be found in the
extended API case (in fact, for any Instant mode).

[dominich.google, 2012-08-27 16:48:03]

On Mon, Aug 27, 2012 at 9:33 AM, <sreeram@chromium.org> wrote:

> On 2012/08/24 23:22:12, I haz the power (commit-bot) wrote:
>
>> Change committed as 153341
>>
>
> It seems to me that this change doesn't extract search terms from the
> fragment
> (ref) portion of the URL, which is where the search terms will be found in
> the
> extended API case (in fact, for any Instant mode).
>

Coming soon. This CL allows for easy testing using a custom search
provider. The next CL will add fragment support for instant URLs.



>
>
http://codereview.chromium.**org/10867038/<http://codereview.chromium.org/108...
>