Enabled pressing TAB to traverse through the Omnibox results, removed moving the caret to the end...

Enabled pressing TAB to traverse through the Omnibox results, removed moving the caret to the end of the line with TAB, and filtered out redundant URLs.

This adds the ability to move through Omnibox result matches using TAB in addition to the arrow keys. To enable this, pressing TAB to move the caret to the end of the line was removed and the keyword hint/shortcut logic has been modified.

The Omnibox popup now shows keyword markers on the right side of matches that have associated keywords (represented by a right arrow). When this kind of match is selected, and the keyword is accepted, the match changes appearance by animating in the associated keyword match from the right to display the "Search X for <>" message. 

If multiple matches have the same keyword then only the most relevant match will display the keyword marker and hint. Pressing TAB while a keyword hint is shown will enter keyword mode in place; the results will no longer change when keyword mode is entered. Additionally, substituting keyword provider matches will only be shown if a keyword substitution
is available.  

Finally, results with redundant destination URLs (e.g., "foo.com", "www.foo.com") will have the less relevant URLs filtered out.

This also addresses some GTK omnibox browsertest flakiness; see bug 112041.

See original change review at http://codereview.chromium.org/6731036

Contributed by aaron.randolph@gmail.com

BUG=57748, 76278, 77662, 80934, 84420
TEST=Press TAB to move the selection down the list of results, SHIFT+TAB to move up.

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

[aaron.randolph, 2012-02-05 00:05:57]

Uploaded a new CL that removes the GTK workaround.  This is to test if the
plumbing added for the workaround is causing Mac browser_tests to crash.  Plus,
the workaround is now unnecessary due to
https://chromiumcodereview.appspot.com/9307018/

[Peter Kasting, 2012-02-05 01:23:16]

LGTM, obviously.  sail, can you test-run this patch through the browser_tests on
your Mac?

[Elliot Glaysher, 2012-02-05 02:34:14]

gtk lgtm

[sail, 2012-02-06 16:21:37]

On 2012/02/05 01:23:16, Peter Kasting wrote:
> LGTM, obviously.  sail, can you test-run this patch through the browser_tests
on
> your Mac?

Sure thing!

[sail, 2012-02-06 17:34:35]

On 2012/02/06 16:21:37, sail wrote:
> On 2012/02/05 01:23:16, Peter Kasting wrote:
> > LGTM, obviously.  sail, can you test-run this patch through the
browser_tests
> on
> > your Mac?
> 
> Sure thing!

All browser tests pass on my Mac.

[sky, 2012-02-06 18:30:44]

Rubber stamp LGTM

[sail, 2012-02-06 18:31:57]

LGTM
(I'll also try the original patch to see if it passes on my Mac).

[Peter Kasting, 2012-02-06 22:26:44]

So, sail tested both the original and this patch, and neither seemed to fail on
his machine.  Our next step is going to be to build one of the patches with
Address Sanitizer on, in hopes that the problem is some kind of memory
corruption that the bots are tripping.  If that also fails to repro, we can try
to land again and then debug on the bots directly.

[aaron.randolph, 2012-02-06 22:43:21]

On 2012/02/06 22:26:44, Peter Kasting wrote:
> So, sail tested both the original and this patch, and neither seemed to fail
on
> his machine.  Our next step is going to be to build one of the patches with
> Address Sanitizer on, in hopes that the problem is some kind of memory
> corruption that the bots are tripping.  If that also fails to repro, we can
try
> to land again and then debug on the bots directly.

Sounds good.  I'm going to try it with Address Sanitizer as well; that looks
like fun.  Or interesting, at least.  :)

[aaron.randolph, 2012-02-11 00:12:05]

On 2012/02/06 22:26:44, Peter Kasting wrote:
> So, sail tested both the original and this patch, and neither seemed to fail
on
> his machine.  Our next step is going to be to build one of the patches with
> Address Sanitizer on, in hopes that the problem is some kind of memory
> corruption that the bots are tripping.  If that also fails to repro, we can
try
> to land again and then debug on the bots directly.

Have you guys had any luck with Address Sanitizer?   I've spent what time I had
this week messing with it, but I keep getting a crash in ffmpeg when I try to
run browser_tests with asan enabled.   I can't blame it on my patch cause I get
the same crash without my patch.  :/

[Peter Kasting, 2012-02-11 19:29:35]

On 2012/02/11 00:12:05, aaron.randolph wrote:
> Have you guys had any luck with Address Sanitizer?

I've been continually bugging sail to actually build it and he's been shirking. 
Supposedly he finally kicked off a build.

[sail, 2012-02-15 21:27:26]

On 2012/02/11 19:29:35, Peter Kasting wrote:
> On 2012/02/11 00:12:05, aaron.randolph wrote:
> > Have you guys had any luck with Address Sanitizer?
> 
> I've been continually bugging sail to actually build it and he's been
shirking. 
> Supposedly he finally kicked off a build.

I logged into mini42-m1 and applied this patch. browser_tests built and ran
correctly.

At this point we should consider splitting this patch in multiple pieces (using
stubs, flags, etc..) and landing it one piece at a time.

[Peter Kasting, 2012-02-15 21:35:43]

On 2012/02/15 21:27:26, sail wrote:
> On 2012/02/11 19:29:35, Peter Kasting wrote:
> > On 2012/02/11 00:12:05, aaron.randolph wrote:
> > > Have you guys had any luck with Address Sanitizer?
> > 
> > I've been continually bugging sail to actually build it and he's been
> shirking. 
> > Supposedly he finally kicked off a build.
> 
> I logged into mini42-m1 and applied this patch. browser_tests built and ran
> correctly.
> 
> At this point we should consider splitting this patch in multiple pieces
(using
> stubs, flags, etc..) and landing it one piece at a time.

Aaron, do you have any good ideas of how to break this into pieces?

If we can't split it I guess we can try to land and live-debug :(

[aaron.randolph, 2012-02-15 22:53:00]

On 2012/02/15 21:35:43, Peter Kasting wrote:
> On 2012/02/15 21:27:26, sail wrote:
> > I logged into mini42-m1 and applied this patch. browser_tests built and ran
> > correctly.
> > 
> > At this point we should consider splitting this patch in multiple pieces
> (using
> > stubs, flags, etc..) and landing it one piece at a time.
> 
> Aaron, do you have any good ideas of how to break this into pieces?
> 
> If we can't split it I guess we can try to land and live-debug :(

I could back out the stuff in omnibox_view_mac.mm and check those changes in
separately.  The other Mac changes are due to refactorings and would be more
difficult to break out.

Almost all the internal, non-UI changes rely on each other, so I think those
would be near impossible to do separately.  Except for maybe minor things, like
stripping the destination URL and removing redundant matches.

Without a hint as to what caused the Mac bots to crash, it's really difficult to
say what to try to break out.  Does the Mac Debug build bot build with symbols? 
I'm curious why their dumps just showed addresses instead of names. (Is it
possible to specify a larger call stack be dumped out?) Can we try a lot of
various Mac try bots and see what happens? (I'll need to update the CL to the
latest revision.)

[Scott Hess - ex-Googler, 2012-02-15 22:55:26]

On 2012/02/15 22:53:00, aaron.randolph wrote:
> Without a hint as to what caused the Mac bots to crash, it's really difficult
to
> say what to try to break out.  Does the Mac Debug build bot build with
symbols? 
> I'm curious why their dumps just showed addresses instead of names. (Is it
> possible to specify a larger call stack be dumped out?) Can we try a lot of
> various Mac try bots and see what happens? (I'll need to update the CL to the
> latest revision.)

Are they 10.5 bots?  10.5's symbolization doesn't work right.  10.6 should be
showing symbols with the stack dump.

Hmm, though I do seem to recall something about OSX bots building without
symbols a month or so back.  Seems like a bad idea.

-scott

[aaron.randolph, 2012-02-15 23:27:42]

On 2012/02/15 22:55:26, shess wrote:
> On 2012/02/15 22:53:00, aaron.randolph wrote:
> > Without a hint as to what caused the Mac bots to crash, it's really
difficult
> to
> > say what to try to break out.  Does the Mac Debug build bot build with
> symbols? 
> > I'm curious why their dumps just showed addresses instead of names. (Is it
> > possible to specify a larger call stack be dumped out?) Can we try a lot of
> > various Mac try bots and see what happens? (I'll need to update the CL to
the
> > latest revision.)
> 
> Are they 10.5 bots?  10.5's symbolization doesn't work right.  10.6 should be
> showing symbols with the stack dump.
> 
> Hmm, though I do seem to recall something about OSX bots building without
> symbols a month or so back.  Seems like a bad idea.
> 
> -scott

Reflecting back on it again, I think it was showing names at the bottom half of
the call stack, but they were unrelated to my changes.  (I tried looking it up
but the logs aren't available anymore.)  The top half was all OS calls, I
assume, since they just showed addresses and there were/are no OS debug symbols?

I've updated the CL to the latest revision.

[aaron.randolph, 2012-02-16 22:28:35]

Well, that looks promising.  safe_browsing tests passed on the valgrind bot, and
all the tests passed on the release bot.  The failures all look completely
unrelated.

I would be really surprised if it was all down to that GTK test workaround.  How
would a noop virtual method cause a crash?

[Peter Kasting, 2012-02-17 20:46:18]

Sigh.

So, we test-landed this yesterday, and Mac stuff seemed to work, but the Linux
ChromiumOS bots broke and it got reverted again.  See
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS/b...
for an example failing run.  It looks as if the following tests may have failed:

browser_tests: OmniboxApiTest.Basic
interactive_ui_tests: OmniboxViewTest.AcceptKeywordBySpace,
OmniboxViewTest.NonSubstitutingKeywordTest

Perhaps there is a problem in the omnibox_view_views.cc code?  Haven't tried to
debug this myself.

I'll kick off another try run, this time for all the CrOS bots I can find, in
hopes of providing more data.

[aaron.randolph, 2012-02-17 20:56:45]

On 2012/02/17 20:46:18, Peter Kasting wrote:
> Sigh.
> 
> So, we test-landed this yesterday, and Mac stuff seemed to work, but the Linux
> ChromiumOS bots broke and it got reverted again.  See
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%2520ChromiumOS...
> for an example failing run.  It looks as if the following tests may have
failed:
> 
> browser_tests: OmniboxApiTest.Basic
> interactive_ui_tests: OmniboxViewTest.AcceptKeywordBySpace,
> OmniboxViewTest.NonSubstitutingKeywordTest
> 
> Perhaps there is a problem in the omnibox_view_views.cc code?  Haven't tried
to
> debug this myself.
> 
> I'll kick off another try run, this time for all the CrOS bots I can find, in
> hopes of providing more data.

Yeah, I saw that this morning.  :/  I've looked a bit today into how to build
CrOS so I can test this out.  I'll work it the same as I did with the Linux
build to see what was making the popup close.

[aaron.randolph, 2012-02-20 22:19:30]

Updated the CL to pass tests on ChromeOS. (omnibox_view_views.cc)

Issue 117421  (http://code.google.com/p/chromium/issues/detail?id=114721) can
probably be reverted back. OmniboxcApiTest.Basic now passes with this patch on
ChromeOS.

I now have test environments for Windows, Mac, Linux, and ChromeOS, so we should
hopefully not run into this kind of issue again.

[Peter Kasting, 2012-02-21 21:24:15]

Is the linux_rel "PopupStaysClosed" failure flakiness?  Do you want me to try
running that bot through again?

[aaron.randolph, 2012-02-21 21:26:04]

On 2012/02/21 21:24:15, Peter Kasting wrote:
> Is the linux_rel "PopupStaysClosed" failure flakiness?  Do you want me to try
> running that bot through again?

It looks like it to me.   All the tests but 1 passed when they were tried again.
  One more time wouldn't hurt, though.

[aaron.randolph, 2012-02-21 22:26:10]

On 2012/02/21 21:26:04, aaron.randolph wrote:
> On 2012/02/21 21:24:15, Peter Kasting wrote:
> > Is the linux_rel "PopupStaysClosed" failure flakiness?  Do you want me to
try
> > running that bot through again?
> 

Seems to be a consistent flakiness.  Fails the first time, passes the second.  
I'm looking into it to see if I can replicate it.

[aaron.randolph, 2012-02-22 20:32:20]

On 2012/02/21 22:26:10, aaron.randolph wrote:
> On 2012/02/21 21:26:04, aaron.randolph wrote:
> > On 2012/02/21 21:24:15, Peter Kasting wrote:
> > > Is the linux_rel "PopupStaysClosed" failure flakiness?  Do you want me to
> try
> > > running that bot through again?
> > 
> 
> Seems to be a consistent flakiness.  Fails the first time, passes the second. 

> I'm looking into it to see if I can replicate it.

I've tried these various ways and they always pass for me.  I haven't seen
anything that says this wasn't an anomaly.  Can we try linux_rel once more and
see what happens today?  I'm also curious if it will happen on linux_chromeos
since the revert for OmniboxApiTest.Basic wasn't in yet when the try was run
yesterday,

[aaron.randolph, 2012-02-23 02:25:23]

While digging around, I noticed these two tests also failed on win_rel and also
passed on their second try, though the build status didn't mention this. 
Furthermore, Basic failed for a different reason than on Linux.

Digging some more, I saw these specific tests were changed a couple weeks ago.  
I looked at the code review (9358036) for that change and found out the try bots
failed these two same tests as well on the first try and then the tests passed
on the second try.  For win_rel, the build status only reported PopupStaysClosed
failed, even though Basic failed as well.   The Linux log no longer exists so I
couldn't verify that one.

So it looks like these failures are pre-existing and unrelated to my patch. 
However, perhaps there may be another issue that needs to be investigated.

[aaron.randolph, 2012-02-23 13:11:27]

On 2012/02/23 02:25:23, aaron.randolph wrote:
> While digging around, I noticed these two tests also failed on win_rel and
also
> passed on their second try, though the build status didn't mention this. 
> Furthermore, Basic failed for a different reason than on Linux.
> 
> Digging some more, I saw these specific tests were changed a couple weeks ago.
 
> I looked at the code review (9358036) for that change and found out the try
bots
> failed these two same tests as well on the first try and then the tests passed
> on the second try.  For win_rel, the build status only reported
PopupStaysClosed
> failed, even though Basic failed as well.   The Linux log no longer exists so
I
> couldn't verify that one.
> 
> So it looks like these failures are pre-existing and unrelated to my patch. 
> However, perhaps there may be another issue that needs to be investigated.

I looked around some more and while I found these first-time failures for
OmniboxApiTest.Basic and OmniboxApiTest.PopupStaysClosed seem to be common on
try bots (see the browser_tests stdio log for any linux_rel and win_rel try,
green or otherwise), the build bots don't show the same failures.  I assume try
bots run the tests differently somehow?

Some example issues I looked at: 9442013, 9431042, 9423037.

[Peter Kasting, 2012-02-23 21:07:11]

On 2012/02/23 13:11:27, aaron.randolph wrote:
> On 2012/02/23 02:25:23, aaron.randolph wrote:
> > While digging around, I noticed these two tests also failed on win_rel and
> also
> > passed on their second try, though the build status didn't mention this. 
> > Furthermore, Basic failed for a different reason than on Linux.
> > 
> > Digging some more, I saw these specific tests were changed a couple weeks
ago.
>  
> > I looked at the code review (9358036) for that change and found out the try
> bots
> > failed these two same tests as well on the first try and then the tests
passed
> > on the second try.

This seems to be http://code.google.com/p/chromium/issues/detail?id=113455 .

I think we should gut the conditionals added in
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/ext...
and just mark both these tests as FLAKY_.  I'll get in touch with Mark to make
sure he's OK with this, do it, and then hopefully land this patch again.

[Peter Kasting, 2012-02-24 02:16:34]

A few random things noticed while fixing local merge conflicts...

http://codereview.chromium.org/9309099/diff/35002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/keyword_provider.cc (right):

http://codereview.chromium.org/9309099/diff/35002/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.cc:478: match.keyword = keyword;
I'm not sure setting this unconditionally is right.  Shouldn't it only be set
when we set |template_url|?  After all, for non-substituting keywords, we don't
want to show any custom "keyword mode" UI.

http://codereview.chromium.org/9309099/diff/35002/chrome/browser/autocomplete...
File chrome/browser/autocomplete/keyword_provider.h (right):

http://codereview.chromium.org/9309099/diff/35002/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.h:64: bool trim_leading_whitespace,
Nit: Indent these even, not 2

http://codereview.chromium.org/9309099/diff/35002/chrome/browser/autocomplete...
chrome/browser/autocomplete/keyword_provider.h:90: const string16& text,
Nit: Can fit first arg on above line and then indent even

[aaron.randolph, 2012-02-24 23:47:10]

Addressed the nits and updated to the latest version.

[commit-bot: I haz the power, 2012-02-29 00:38:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aaron.randolph@gmail.com/9309099/50001

[commit-bot: I haz the power, 2012-02-29 05:43:37]

Change committed as 124125