Make the OmniboxNavigationObserver update the ShortcutsProvider.

Make the OmniboxNavigationObserver update the ShortcutsProvider.

This means the ShortcutsProvider will only store shortcuts to successful
navigations, eliminating a major source of bad omnibox suggestions.

BUG=151044
TEST=Type "sughoidusgbpoiun.com" in omnibox, hit enter; retype "sugho" and ensure old bogus URL is not suggested
R=brettw@chromium.org, sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228846

[Peter Kasting, 2013-10-14 20:53:15]

There may still be an issue with the ShortcutsBackend being NULL in some
scenarios, but I have an interview to do, so sending this almost-final patch for
review.

bartn: shortcuts
sky: omnibox
brettw: history OWNERS

[Bart N., 2013-10-14 21:31:54]

Are you planning to add some unit tests?

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:194: string16
text_lowercase(base::i18n::ToLower(text));
const

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:196: for
(ShortcutMap::const_iterator it(
Hmmm... why don't you use GUID -> shortcut iterator map here? It seems that you
need to look up match.destination_url and check for the prefix.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:202: base::Time::Now(),
For consistency and performance, you should take this outside the loop, i.e.

const base::Time now = base::Time::Now();

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:208: base::Time::Now(), 1));
Replace it with "now" introduced in previous step.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:235: if (content::Details<const
history::URLsDeletedDetails>(details)->
Style: -> should be on the second line.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.h (right):

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_navigation_observer.h:98: string16 text_;
It looks like that both text_ and match_ can be const. Since you are here, can
you check if other fields can be  made const as well?

[sky, 2013-10-14 22:57:27]

LGTM

[brettw, 2013-10-15 17:51:10]

history lgtm rubberstamp

[Peter Kasting, 2013-10-16 01:28:55]

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:194: string16
text_lowercase(base::i18n::ToLower(text));
On 2013/10/14 21:31:54, Bart N. wrote:
> const

I agree, and I made the change here, but note that most Chrome code does not use
(and many reviewers frown on) const on locals.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:196: for
(ShortcutMap::const_iterator it(
On 2013/10/14 21:31:54, Bart N. wrote:
> Hmmm... why don't you use GUID -> shortcut iterator map here? It seems that
you
> need to look up match.destination_url and check for the prefix.

I'm confused.  How is using the GUID map useful?  We don't have a GUID sitting
around, so we'd have to iterate through the whole map; at least with the
ShortcutMap we can reduce to only iterating through shortcuts whose text begins
with the user text, which should be a much smaller set.

Plus, with the GUID map, we'd still want to check the text to ensure it begins
with the user's input... so it just seems worse to use this?

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:202: base::Time::Now(),
On 2013/10/14 21:31:54, Bart N. wrote:
> For consistency and performance, you should take this outside the loop, i.e.
> 
> const base::Time now = base::Time::Now();

Done.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:208: base::Time::Now(), 1));
On 2013/10/14 21:31:54, Bart N. wrote:
> Replace it with "now" introduced in previous step.

Done.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:235: if (content::Details<const
history::URLsDeletedDetails>(details)->
On 2013/10/14 21:31:54, Bart N. wrote:
> Style: -> should be on the second line.

Operators go on ends of lines, not starts.  But I solved this a different way by
factoring out a temp.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.h (right):

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_navigation_observer.h:98: string16 text_;
On 2013/10/14 21:31:54, Bart N. wrote:
> It looks like that both text_ and match_ can be const. Since you are here, can
> you check if other fields can be  made const as well?

Done.

[Peter Kasting, 2013-10-16 03:05:32]

Committed patchset #4 manually as r228846 (presubmit successful).

[Bart N., 2013-10-16 17:42:46]

What is your decision regarding the functional test we discussed yesterday?

Secondly, I've noticed you already submitted this CL although I haven't LGTMed.
I was under impression you asked me for shortcuts review? I still believe this
CL needs more work.

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
File chrome/browser/history/shortcuts_backend.cc (right):

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:196: for
(ShortcutMap::const_iterator it(
On 2013/10/16 01:28:55, Peter Kasting wrote:
> On 2013/10/14 21:31:54, Bart N. wrote:
> > Hmmm... why don't you use GUID -> shortcut iterator map here? It seems that
> you
> > need to look up match.destination_url and check for the prefix.
> 
> I'm confused.  How is using the GUID map useful?  We don't have a GUID sitting
> around, so we'd have to iterate through the whole map; at least with the
> ShortcutMap we can reduce to only iterating through shortcuts whose text
begins
> with the user text, which should be a much smaller set.
> 
> Plus, with the GUID map, we'd still want to check the text to ensure it begins
> with the user's input... so it just seems worse to use this?
Sorry, my mistake. I thought that this was a URL hash as opposite to a global
ID. If so, my proposal was to reduce time to O(1) from O(logN).

https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
chrome/browser/history/shortcuts_backend.cc:235: if (content::Details<const
history::URLsDeletedDetails>(details)->
On 2013/10/16 01:28:55, Peter Kasting wrote:
> On 2013/10/14 21:31:54, Bart N. wrote:
> > Style: -> should be on the second line.
> 
> Operators go on ends of lines, not starts.  But I solved this a different way
by
> factoring out a temp.
Interesting, unlike the internal Google style.

https://codereview.chromium.org/27057004/diff/41001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):

https://codereview.chromium.org/27057004/diff/41001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:80: if
(shortcuts_backend_)
This looks odd. The only place where you assign shortcuts_backend_ is in the
constructor and my understanding is that
ShortcutsBackendFactory::GetForProfile(profile) may return NULL.

If so, would it be possible to avoid listening on successful notification if the
backend is NULL? What's the point otherwise?

[Peter Kasting, 2013-10-16 21:48:58]

On 2013/10/16 17:42:46, Bart N. wrote:
> What is your decision regarding the functional test we discussed yesterday?

As I noted to you in my office, my intent was to not implement such a test.

> Secondly, I've noticed you already submitted this CL although I haven't
LGTMed.
> I was under impression you asked me for shortcuts review? I still believe this
> CL needs more work.

Sorry, my mistake.  Somehow I had it in my head that you'd already signed off. 
I didn't intend to bypass you, I just screwed up :(.  If there are more changes
I can land those as a followup.

>
https://codereview.chromium.org/27057004/diff/10001/chrome/browser/history/sh...
> chrome/browser/history/shortcuts_backend.cc:235: if (content::Details<const
> history::URLsDeletedDetails>(details)->
> On 2013/10/16 01:28:55, Peter Kasting wrote:
> > On 2013/10/14 21:31:54, Bart N. wrote:
> > > Style: -> should be on the second line.
> > 
> > Operators go on ends of lines, not starts.  But I solved this a different
way
> by
> > factoring out a temp.
> Interesting, unlike the internal Google style.

The style guide only addresses boolean operators directly, but its comment is
that EOL operators are more common:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Boolea...

> 
>
https://codereview.chromium.org/27057004/diff/41001/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_navigation_observer.cc (right):
> 
>
https://codereview.chromium.org/27057004/diff/41001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_navigation_observer.cc:80: if
> (shortcuts_backend_)
> This looks odd. The only place where you assign shortcuts_backend_ is in the
> constructor and my understanding is that
> ShortcutsBackendFactory::GetForProfile(profile) may return NULL.
> 
> If so, would it be possible to avoid listening on successful notification if
the
> backend is NULL? What's the point otherwise?

It's possible to avoid being called back if both (a) the shortcuts backend is
NULL and (b) we don't have an infobar to possibly show for the alternate nav URL
case.

However, there are a few reasons I didn't bother to try and implement that:
(1) We still have to conditional-check the shortcuts backend in
OnSuccessfulNavigation(), because the omnibox can call it directly.
(2) Trying to calculate the cases where we could delete ourself somewhat earlier
is more code and more error-prone than having a consistent lifetime.
(3) The actual cost of this thing living longer is trivial.  The number of
omnibox navigations in flight at any one time is not large enough for there to
be meaningful perf or memory impact from this.
(4) I intend to expand this class yet further in the future to accommodate other
users that need to monitor omnibox navigations, so just staying alive
consistently makes that easier.