Makes copying text in the omnibox always use permanent text is the

Makes copying text in the omnibox always use permanent text is the
user hasn't modified the url. I'll add a test case if you like this.

BUG=94126
TEST=see bug
R=pkasting@chromium.org

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

[Peter Kasting, 2011-08-30 01:05:57]

No, this isn't what I meant, although this would eliminate some instances of the
bug.

I meant that what we currently call GetURLForText() shouldn't be calling Parse()
at all -- it should be using the classifier -- and we should be doing a check
like this before calling the classifier, regardless of if the user has selected
everything.  We probably also need to add code to handle the case where the
current text, passed through fixup, has the same host as the permanent URL so
that we get that case right even if it's an intranet URL and the user hasn't
typed any URLs with the current hostname.

[sky, 2011-08-30 17:33:31]

On 2011/08/30 01:05:57, Peter Kasting wrote:
> No, this isn't what I meant, although this would eliminate some instances of
the
> bug.
> 
> I meant that what we currently call GetURLForText() shouldn't be calling
Parse()
> at all -- it should be using the classifier -- and we should be doing a check
> like this before calling the classifier, regardless of if the user has
selected
> everything.

Take a look at the current patch and tell me if you like it. I'm not using
classify for the all selected non-edited case. It doesn't seem like we need it,
but maybe I'm missing something? If you like this I can move GetURLForText into
AdjustTextForCopy since it's only called there and once.

> We probably also need to add code to handle the case where the
> current text, passed through fixup, has the same host as the permanent URL so
> that we get that case right even if it's an intranet URL and the user hasn't
> typed any URLs with the current hostname.

Interesting. I think this case only comes up when incognito. I think I could
handle this by adding an extra param to Classify that makes it way to
AutocompleteController and forces the AutocompleteInput to treat the input as
URL instead of UNKNOWN for this case? Sound good?

  -Scott

[Peter Kasting, 2011-08-30 18:26:29]

On 2011/08/30 17:33:31, sky wrote:
> I'm not using
> classify for the all selected non-edited case. It doesn't seem like we need
it,
> but maybe I'm missing something?

No, you're not missing anything -- we shouldn't need to classify in that case.

> > We probably also need to add code to handle the case where the
> > current text, passed through fixup, has the same host as the permanent URL
so
> > that we get that case right even if it's an intranet URL and the user hasn't
> > typed any URLs with the current hostname.
> 
> Interesting. I think this case only comes up when incognito.

This isn't incognito-only.  But I just realized it also hits the normal
navigation case, not just the copy case.

Try the following on a clean profile:
* Create a bookmark to http://b/ (don't type that in the omnibox)
* Click the bookmark, log in, etc. -- you should wind up at b/home
* Click in the address bar and hit the down arrow to see that the omnibox wants
to search for this by default.  This means hitting enter in the address bar to
reload will instead navigate away.

I think perhaps the right fix for this is to prevent the scheme from being
stripped in this case.  I suspect we should change
net_util.cc:FormatUrlWithOffsets() so that instead of explicitly checking for
"ftp." as a signal to not scheme-strip, it tries Parse()ing or Classify()ing the
URL with and without the scheme and checks if the
canonicalized_urls/destination_urls are the same.  (I'd normally go straight to
the classifier, but I worry in this case about potentially calling FormatUrl()
from inside the classifier, which would make this stuff all break; maybe the
canonicalized_url from Parse() is good enough to detect the "ftp." case.)

[Peter Kasting, 2011-08-30 21:30:52]

Can we add some tests for this somehow?  I'd really like to see this have
regression tests because it's subtle.

http://codereview.chromium.org/7806001/diff/2001/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit.cc (right):

http://codereview.chromium.org/7806001/diff/2001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:401: if (!GetURLForText(*text,
url))
Inlining this function into here is fine with me.

http://codereview.chromium.org/7806001/diff/2001/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit.cc:980: return url->is_valid();
This should be:

  return match.transition == PageTransition::TYPED;

This will only be true when the url is valid, and it will reject cases where we
don't navigate by default.  The reason this is important is because otherwise,
if I'm currently on a Google search result page, and I type "foo" in the omnibox
and copy it, this function will return a destination URL that's a Google search
for "foo" and the caller will happily prepend "http://" because the hosts match.

[sky, 2011-08-30 22:40:22]

Updated.

  -Scott

[Peter Kasting, 2011-08-30 23:04:05]

Can we also test the case of a permanent URL of
"http://www.google.com/search?..." and an input of "foo"?

http://codereview.chromium.org/7806001/diff/4003/chrome/browser/autocomplete/...
File chrome/browser/autocomplete/autocomplete_edit_unittest.cc (right):

http://codereview.chromium.org/7806001/diff/4003/chrome/browser/autocomplete/...
chrome/browser/autocomplete/autocomplete_edit_unittest.cc:139: { "b/foo", 0,
true, "b/foo", "http://b/foo", true, "http://b/foo" },
Will this test actually work?  I'd think the classifier would fail on this due
to my changes, unless you also shove b/ into the history system for the test.

[sky, 2011-08-30 23:58:33]

On Tue, Aug 30, 2011 at 4:04 PM,  <pkasting@chromium.org> wrote:
> Can we also test the case of a permanent URL of
> "http://www.google.com/search?..." and an input of "foo"?

Added.

http://codereview.chromium.org/7806001/diff/4003/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/autocomplete_edit_unittest.cc (right):
>
>
http://codereview.chromium.org/7806001/diff/4003/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/autocomplete_edit_unittest.cc:139: {
> "b/foo", 0, true, "b/foo", "http://b/foo", true, "http://b/foo" },
> Will this test actually work?  I'd think the classifier would fail on
> this due to my changes, unless you also shove b/ into the history system
> for the test.

It's caught by this case:

  if (!user_input_in_progress() && is_all_selected) {
    // The user selected all the text and has not edited it. Use the url as the
    // text so that if the scheme was stripped it's added back, and the url
    // is unescaped (we escape parts of the url for display).
    *url = PermanentURL();
    *text = UTF8ToUTF16(url->spec());
    *write_url = true;
    return;
  }

As long as you haven't modified the text and everything is selected,
then you'll get http:// .

  -Scott

[Peter Kasting, 2011-08-31 00:00:19]

On 2011/08/30 23:58:33, sky wrote:
> It's caught by this case:
> 
>   if (!user_input_in_progress() && is_all_selected) {
>     // The user selected all the text and has not edited it. Use the url as
the
>     // text so that if the scheme was stripped it's added back, and the url
>     // is unescaped (we escape parts of the url for display).
>     *url = PermanentURL();
>     *text = UTF8ToUTF16(url->spec());
>     *write_url = true;
>     return;
>   }
> 
> As long as you haven't modified the text and everything is selected,
> then you'll get http:// .

Ah... then maybe the comment on that should say "Unmodified URLs shouldn't get a
scheme added regardless of how they'd be classified".

LGTM