Add "Go to..." to the context menu when the selected text in a page is a url...

Add "Go to..." to the context menu when the selected text in a page is a url.

So when you select:
"http://www.google.com"

The context menu that will appear will be:
"Go to http://www.google.com"

Instead of:
"Search Google for http://www.google.com"

BUG=1978
TEST=open chromium, select a text that is a url, see if appears the menu "Go to..."; click on it, 
see if it goes to the desired page.

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

[tfarina (gmail-do not use), 2009-10-26 12:10:38]

Hi Finnur, could you review this?

[Finnur, 2009-10-26 16:04:26]

http://codereview.chromium.org/326026/diff/1/3
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/326026/diff/1/3#newcode492
Line 492: <message name="IDS_CONTENT_CONTEXT_GOTOURL" desc="The name of the Go
to url for 'string' command in the content area context menu">
This should say: The name of the Go to 'url' command in the content area context
menu (erase: for 'string')

http://codereview.chromium.org/326026/diff/1/3#newcode493
Line 493: &amp;Go to <ph name="URL"> $1<ex>http://www.google.com/</ex> </ph>
nit: extra whitespace after </ex>

http://codereview.chromium.org/326026/diff/1/2
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/1/2#newcode726
Line 726: PageTransition::GENERATED);
PageTransition::Generated doesn't seem right here. It makes sense in the
previous command, but this is just the user clicking the link, so isn't
PageTransition::Link more appropriate?

[Peter Kasting, 2009-10-26 17:38:22]

http://codereview.chromium.org/326026/diff/4001/5001
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/4001/5001#newcode227
Line 227: GURL url(selection_text);
You're not doing any fixup, so you won't navigate for strings like
"www.google.com".

I think you should instead use the decider machinery that we use for
paste-and-search vs. paste-and-go, which is located in the autocomplete code. 
There's a bug about refactoring it a bit to make it more clearly a fit for
external callers.

[tfarina (gmail-do not use), 2009-10-26 18:08:25]

On 2009/10/26 17:38:22, Peter Kasting wrote:
> http://codereview.chromium.org/326026/diff/4001/5001
> File chrome/browser/tab_contents/render_view_context_menu.cc (right):
> 
> http://codereview.chromium.org/326026/diff/4001/5001#newcode227
> Line 227: GURL url(selection_text);
> You're not doing any fixup, so you won't navigate for strings like
> "www.google.com".
> 
> I think you should instead use the decider machinery that we use for
> paste-and-search vs. paste-and-go, which is located in the autocomplete code. 
> There's a bug about refactoring it a bit to make it more clearly a fit for
> external callers.
After reading the bug I found the code that Scott is using to make use of
AutocompleteEditView, but I don't know how to get it from the
RenderViewContextMenu.
Could you give some direction how can I get the LocationBar to then get the
AutocompleteEditView? Or instead of this, I need to refactor the code to a
external function (outside of AutocompleteEditView?)?

[Peter Kasting, 2009-10-26 18:42:41]

On 2009/10/26 18:08:25, tfarina wrote:
> Could you give some direction how can I get the LocationBar to then get the
> AutocompleteEditView? Or instead of this, I need to refactor the code to a
> external function (outside of AutocompleteEditView?)?

Refactoring to somewhere outside the AutocompleteEditView is definitely best.  I
suspect this is isolated enough that it should be pretty easy.

It's possible to get the location bar from the Browser object (see browser.cc),
but I wouldn't go that route.

[tfarina (gmail-do not use), 2009-10-26 19:19:32]

On 2009/10/26 18:42:41, Peter Kasting wrote:
> Refactoring to somewhere outside the AutocompleteEditView is definitely best. 
I
> suspect this is isolated enough that it should be pretty easy.

It's ok to put the new function in TabContents? From there will be easy to call
it from RenderViewContextMenu.
I accept suggestions.

[Peter Kasting, 2009-10-26 19:23:31]

On 2009/10/26 19:19:32, tfarina wrote:
> It's ok to put the new function in TabContents? From there will be easy to
call
> it from RenderViewContextMenu.
> I accept suggestions.

No, it doesn't make sense to add this to TabContents, since it has nothing to do
with the tab contents.  It should be a standalone, toplevel API/object.

[tfarina (gmail-do not use), 2009-10-26 19:49:09]

On 2009/10/26 19:23:31, Peter Kasting wrote:
> No, it doesn't make sense to add this to TabContents, since it has nothing to
do
> with the tab contents.  It should be a standalone, toplevel API/object.

Could be an anonymous namespace, with this function, in autocomplete.cc?
Or are you thinking in a class in a new file?

[Peter Kasting, 2009-10-26 20:33:38]

On 2009/10/26 19:49:09, tfarina wrote:
> Could be an anonymous namespace, with this function, in autocomplete.cc?
> Or are you thinking in a class in a new file?

I'd just put it in a new file.  It can live in browser/.

[tfarina (gmail-do not use), 2009-10-29 23:47:04]

Now using the SearchVersusNavigateClassifier. 
Please take a look.

[Peter Kasting, 2009-10-30 00:12:26]

http://codereview.chromium.org/326026/diff/12002/13002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/326026/diff/12002/13002#newcode668
Line 668: <message name="IDS_CONTENT_CONTEXT_GOTOURL" desc="The name of the Go
to url for 'string' command in the content area context menu">
Nit: Looks from the IDS_CONTENT_CONTEXT_SEARCHWEBFOR description like you should
put "In Title Case: " on the fron of the desc here.

http://codereview.chromium.org/326026/diff/12002/13001
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/12002/13001#newcode231
Line 231: std::wstring(), &is_search, &url, NULL, NULL, NULL);
I think here you should bail immediately if the destination URL isn't valid;
otherwise, you should store it on a new member variable
("selection_navigation_url_"?).

http://codereview.chromium.org/326026/diff/12002/13001#newcode233
Line 233: std::wstring label(l10n_util::GetStringF(
Use GetStringFUTF16() and omit WideToUTF16().  Personally I wouldn't bother with
the temp |label| either.

http://codereview.chromium.org/326026/diff/12002/13001#newcode718
Line 718: case IDS_CONTENT_CONTEXT_SEARCHWEBFOR: {
Both of these handlers can be collapsed into a single entry that navigates to
the member variable you set above.

[tfarina (gmail-do not use), 2009-10-30 23:50:39]

Changes made Peter, please take a look.

http://codereview.chromium.org/326026/diff/12002/13002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/326026/diff/12002/13002#newcode668
Line 668: <message name="IDS_CONTENT_CONTEXT_GOTOURL" desc="The name of the Go
to url for 'string' command in the content area context menu">
On 2009/10/30 00:12:26, Peter Kasting wrote:
> Nit: Looks from the IDS_CONTENT_CONTEXT_SEARCHWEBFOR description like you
should
> put "In Title Case: " on the fron of the desc here.

Done.

http://codereview.chromium.org/326026/diff/12002/13001
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/12002/13001#newcode233
Line 233: std::wstring label(l10n_util::GetStringF(
On 2009/10/30 00:12:26, Peter Kasting wrote:
> Use GetStringFUTF16() and omit WideToUTF16().  Personally I wouldn't bother
with
> the temp |label| either.

Done.

[Peter Kasting, 2009-10-31 00:18:19]

LGTM

http://codereview.chromium.org/326026/diff/15002/19001
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/15002/19001#newcode232
Line 232: GURL destination_url;
Nit: You don't need this temp, you can write straight into
selection_navigation_url_.

http://codereview.chromium.org/326026/diff/15002/19001#newcode237
Line 237: else
Nit: No else after return

http://codereview.chromium.org/326026/diff/15002/19002
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/326026/diff/15002/19002#newcode112
Line 112: // navigate to it.
Nit: This might be clearer:
"The destination URL to use if the user tries to search for or navigate to a
text selection."

[tfarina (gmail-do not use), 2009-10-31 00:37:31]

http://codereview.chromium.org/326026/diff/15002/19001
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/326026/diff/15002/19001#newcode232
Line 232: GURL destination_url;
On 2009/10/31 00:18:19, Peter Kasting wrote:
> Nit: You don't need this temp, you can write straight into
> selection_navigation_url_.

Removed.

http://codereview.chromium.org/326026/diff/15002/19001#newcode237
Line 237: else
On 2009/10/31 00:18:19, Peter Kasting wrote:
> Nit: No else after return

Removed.

http://codereview.chromium.org/326026/diff/15002/19002
File chrome/browser/tab_contents/render_view_context_menu.h (right):

http://codereview.chromium.org/326026/diff/15002/19002#newcode112
Line 112: // navigate to it.
On 2009/10/31 00:18:19, Peter Kasting wrote:
> Nit: This might be clearer:
> "The destination URL to use if the user tries to search for or navigate to a
> text selection."

Done.

[Peter Kasting, 2009-10-31 01:13:56]

Landed in r30656.

[Nico, 2009-10-31 05:21:44]

Reverted in r30657.

[tfarina (gmail-do not use), 2009-10-31 15:07:01]

On 2009/10/31 05:21:44, Nico wrote:
> Reverted in r30657.

I looked into the buildbots errors from the commit of Peter.
I think this can make the bots happy again.

[tfarina (gmail-do not use), 2009-10-31 17:34:06]

I uploaded another fix for linux/mac errors. One conversion from string16 to
wstring was missing.

[tfarina (gmail-do not use), 2009-11-01 16:53:16]

Peter, Nico, these are the trybots results.
http://build.chromium.org/buildbot/try-server/buildstatus?builder=linux&numbe...
http://build.chromium.org/buildbot/try-server/buildstatus?builder=mac&number=...
http://build.chromium.org/buildbot/try-server/buildstatus?builder=win&number=...

The results are:
linux - OK.
mac - timeout.
win - passed before, but failed this time. 

Also I ran the unit_tests this morning to see if the same errors occurred, but
the tests passed OK.

[Nico, 2009-11-01 18:02:10]

Ok. I sent it to the try servers again, let's see if windows passes this time.

On Sun, Nov 1, 2009 at 9:53 AM,  <thiago.farina@gmail.com> wrote:
> Peter, Nico, these are the trybots results.
>
http://build.chromium.org/buildbot/try-server/buildstatus?builder=linux&numbe...
>
http://build.chromium.org/buildbot/try-server/buildstatus?builder=mac&number=...
>
http://build.chromium.org/buildbot/try-server/buildstatus?builder=win&number=...
>
> The results are:
> linux - OK.
> mac - timeout.
> win - passed before, but failed this time.
>
> Also I ran the unit_tests this morning to see if the same errors occurred,
> but
> the tests passed OK.
>
> http://codereview.chromium.org/326026
>

[Nico, 2009-11-01 21:50:22]

Win and Linux trybots came back fine, win timed out. I figured to give this a
try; seems still broken (landed again in r30679, reverted again in r30681).

[Nico, 2009-11-02 06:23:05]

…and landed for good in r30685. grd changes still guarantee interesting times,
it seems.

[Peter Kasting, 2009-11-02 10:27:32]

Since all the drama happened while I was at home, was the problem the whole time
just that the builders needed a clobber due to the .grd change?  Or was there
also a compile error due to string16 issues?  Looking at the different patches
it looks like the only change was what ought to be a useless use of string16
(that just causes an extra string conversion), but maybe my in-my-head compiler
doesn't match reality.

[Nico, 2009-11-02 14:30:33]

There was one real issue, the first commit had compile errors, and
something Thiago did fixed that.

The other problems were due to me not realizing that the windows test
bots grab the builds from two other bots that compile and package
chromium. Because I didn't know that, I clobbered the testers and not
the builders.

On Mon, Nov 2, 2009 at 3:27 AM,  <pkasting@chromium.org> wrote:
> Since all the drama happened while I was at home, was the problem the who=
le
> time
> just that the builders needed a clobber due to the .grd change? =A0Or was
> there
> also a compile error due to string16 issues? =A0Looking at the different
> patches
> it looks like the only change was what ought to be a useless use of strin=
g16
> (that just causes an extra string conversion), but maybe my in-my-head
> compiler
> doesn't match reality.
>
> http://codereview.chromium.org/326026
>