Fixes bug where keyword editor would end up prefixing all keyword urls...

Fixes bug where keyword editor would end up prefixing all keyword urls
with "http://" if they didn't have one. This proves problematic as for
the google search url we don't have http:// and don't want it. The fix
is to only add http:// if the url field is editable. If the url field
isn't editable, we know the user hasn't editted it and don't need to
try and fix it up.

BUG=13282
TEST=see bug, but also make sure you don't run into any other problems
with the keyword editor.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=18410

[Peter Kasting, 2009-06-12 19:47:12]

http://codereview.chromium.org/126052/diff/1/2
File chrome/browser/views/edit_keyword_controller.cc (right):

http://codereview.chromium.org/126052/diff/1/2#newcode313
Line 313: // cause problems if we added a scheme (such as {google:baseURL}).
This feels like the wrong fix.  If for example the user puts in their own copy
of the Google entry and also uses {google:baseURL}, we can still run into
problems.

It seems like the right fix is instead to have some call (maybe the one above,
or maybe a new one?) substitute in values for macros in the "url" -- these could
either be the real values or some bogus placeholder ones (although just using
the real ones is probably easiest).

Since we already have code that can do these kinds of substitutions, it doesn't
seem like it should be too hard to call it here.

[sky, 2009-06-12 20:21:55]

New patch uploaded.

[Peter Kasting, 2009-06-12 20:37:25]

http://codereview.chromium.org/126052/diff/1004/1005
File chrome/browser/views/edit_keyword_controller.cc (right):

http://codereview.chromium.org/126052/diff/1004/1005#newcode331
Line 331: // scheme.
This works, but the helper function seems complex.  Why not just add the
following lines to GetURL() here:

TemplateURL t_url;
t_url.SetURL(url, 0, 0);
url = t_url.url()->ReplaceSearchTerms(t_url, L"x", 0,
std::wstring()).possibly_invalid_spec();

I suggest this because even though ReplaceSearchTerms() is documents as
returning a blank string when the URL doesn't support replacement, it looks to
me like it actually returns the unmodified string -- and URLs can have
{google:baseURL} without "supporting replacement" (since that only checks for
search term replacement).

[sky, 2009-06-12 22:53:57]

On 2009/06/12 20:37:25, pkasting wrote:
> http://codereview.chromium.org/126052/diff/1004/1005
> File chrome/browser/views/edit_keyword_controller.cc (right):
> 
> http://codereview.chromium.org/126052/diff/1004/1005#newcode331
> Line 331: // scheme.
> This works, but the helper function seems complex.  Why not just add the
> following lines to GetURL() here:
> 
> TemplateURL t_url;
> t_url.SetURL(url, 0, 0);
> url = t_url.url()->ReplaceSearchTerms(t_url, L"x", 0,
> std::wstring()).possibly_invalid_spec();
> 
> I suggest this because even though ReplaceSearchTerms() is documents as
> returning a blank string when the URL doesn't support replacement, it looks to
> me like it actually returns the unmodified string -- and URLs can have
> {google:baseURL} without "supporting replacement" (since that only checks for
> search term replacement).

Here's the code for the approach you like:

  // Parse the string as a URL to determine the scheme. If we need to, add the
  // scheme. As the scheme may be expanded (as happens with {google:baseURL})
  // we need to run replacement then test the resulting string.
  TemplateURL t_url;
  t_url.SetURL(url, 0, 0);
  GURL expanded_url =
      t_url.url()->ReplaceSearchTerms(t_url, L"x", 0, std::wstring());
  std::string expanded_string = expanded_url.possibly_invalid_spec();
  url_parse::Parsed parts;
  std::string scheme(URLFixerUpper::SegmentURL(expanded_string, &parts));
  if(!parts.scheme.is_valid()) {
    if (scheme.empty()) {
      // Scheme is empty if expanded_string is empty. expanded_string is empty
      // if GURL() didn't like the string we passed to it, which can happen with
      // strings such as GURL("a").
      scheme = chrome::kHttpScheme;
    }
    std::string fixed_scheme(scheme);
    fixed_scheme.append("://");
    url.insert(0, UTF8ToWide(fixed_scheme));
  }

There are a couple of things worth explaining. ReplaceSearchTerms returns a
GURL, but for strings like "foo", GURL("foo").possibly_invalid_spec() returns
the empty string. Brett says GURL does its best to return the string you
supplied to it from possibly_invalid_spec, but not always as this case shows.
SegmentURL is then invoked with an empty string, which returns an empty scheme.

When we spoke you said you liked the approach here because we always call
ReplaceSearchTerms, so that if we had some URL that didn't support replacement
but was escaped in some way (similar to the google base url replacement) it
would still work. We don't do this in the rest of the code though. If the
TemplateURL doesn't support replacements, we don't invoke replace on it when
using it as a bookmark keyword. So I think that point is mote. 

The code to get this approach feels fragile to me for the following reasons:

. I don't know if it's possible for GURL to insert a scheme when one wasn't
passed in.
. ReplaceSearchTerms returns GURL() if the TemplateURLRef wasn't valid.

I prefer the approach in the patch set 2, it avoids these problems and feels
safer.

What do you think?

[Peter Kasting, 2009-06-12 23:09:46]

On 2009/06/12 22:53:57, sky wrote:
> There are a couple of things worth explaining. ReplaceSearchTerms returns a
> GURL, but for strings like "foo", GURL("foo").possibly_invalid_spec() returns
> the empty string.

Ooh, that's bad.

> When we spoke you said you liked the approach here because we always call
> ReplaceSearchTerms, so that if we had some URL that didn't support replacement
> but was escaped in some way (similar to the google base url replacement) it
> would still work. We don't do this in the rest of the code though. If the
> TemplateURL doesn't support replacements, we don't invoke replace on it when
> using it as a bookmark keyword. So I think that point is mote. 

"moot", but fair enough, I agree :).  Although I'm not sure it matters in the
sense that we know the SupportsReplacement() call isn't necessary, so why not
just write the code the simpler way?

> I prefer the approach in the patch set 2, it avoids these problems and feels
> safer.

I'm still not real keen on using "!gurl.valid()" as the determinant of adding a
scheme.

This whole situation feels like a mess.  I'm now thinking maybe
ReplaceSearchTerms() should return a string instead of wrapping everything in a
GURL; then you avoid the problem of going to GURL and back and the above code
becomes a lot safer.

[sky, 2009-06-15 17:11:05]

Updated as per our conversation. I'm also upping the prepopulate id to make sure
if anyone has a bad template url for Google it gets corrected.

[Peter Kasting, 2009-06-15 18:02:50]

LGTM.

You were right about there being lots of callers.  If you want, either of the
following would be fine:
* Since every caller seems to do WideToUTF8(), ReplaceSearchTerms() could return
a UTF-8 string.
* You could write a small wrapper function that just did
GURL(WideToUTF8(ReplaceSearchTerms(...))); and call that most places.

Neither of these are things I'm strongly suggesting, I just mention them in case
the current verbosity bugs you.  Checking in as-is is totally fine with me.

[sky, 2009-06-15 18:06:21]

On 2009/06/15 18:02:50, pkasting wrote:
> LGTM.
> 
> You were right about there being lots of callers.  If you want, either of the
> following would be fine:
> * Since every caller seems to do WideToUTF8(), ReplaceSearchTerms() could
return
> a UTF-8 string.
> * You could write a small wrapper function that just did
> GURL(WideToUTF8(ReplaceSearchTerms(...))); and call that most places.
> 
> Neither of these are things I'm strongly suggesting, I just mention them in
case
> the current verbosity bugs you.  Checking in as-is is totally fine with me.

TemplateURL/TemplateURLRef needs to be converted to string16. When that's done,
it'll be a good time to convert ReplaceSearchTerms to UTF8. For now I'm holding
off.