Experimental AutocompleteProvider for zerosuggest.

chrome/browser/ui/omnibox/omnibox_edit_model.cc

Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index bf5901d5c47ac8eb5291de1fbe61987dff7a69fa..5fa9c306459bf38a8d5941c0380a18e6f98791dc 100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -440,13 +440,15 @@ void OmniboxEditModel::StartAutocomplete(
 }
 
 void OmniboxEditModel::StopAutocomplete() {
-  if (popup_->IsOpen() && !in_revert_) {
-    InstantController* instant = controller_->GetInstant();
-    if (instant && !instant->commit_on_pointer_release())
-      instant->Hide();
-  }
+  if (popup_->IsOpen()) {
+    if (!in_revert_) {
+      InstantController* instant = controller_->GetInstant();
+      if (instant && !instant->commit_on_pointer_release())
+        instant->Hide();
+    }
 
-  autocomplete_controller_->Stop(true);
+    autocomplete_controller_->Stop(true);

[Peter Kasting, 2012/08/14 19:20:02]

Nit: I think this change is safe, but it's not totally clear to me why you need
to make it.

[Jered, 2012/08/14 22:27:11]

OmniboxView::CloseOmniboxPopup() is sometimes called when the omnibox popup
isn't even open. OmniboxView doesn't seem to know whether the popup is really
open, but it calls this method which does, so side-effects in this method need
to be guarded by a test to IsOpen.

Previously, stopping autocomplete in this case was a no-op, but with
zero-suggest it would stop suggestions fetching immediately after starting (and
then the popup never opens).

[Peter Kasting, 2012/08/15 05:30:39]

Like when?  Perhaps those calls are bugs and should be removed instead of
changing the code here.
This is definitely a bad effect that we should fix, but I'm not sure whether the
fix you've made is correct.  Knowing the call chain that triggers this would be
useful.

[Jered, 2012/08/15 16:32:12]

It seems to happen all over the place in Mac UI code. I am not sure about other
platforms, I could check gtk but don't have easy access to Windows for testing.
But glancing at the code, I think the assumption made is that it's ok to close
the popup if already closed and this method should not do anything if that's the
case. Here are some call chains I observed:

1. When opening chrome, Browser::UpdateToolbar ->
BrowserWindowCocoa::UpdateToolbar -> LocationBarViewMac::Update ->
OmniboxViewMac::RevertAll -> OmniboxViewMac::CloseOmniboxPopup (twice).
2. On new tab, -[AutocompleteTextField windowDidResignKey:] ->
OmniboxViewMac::ClosePopup(), then UpdateToolbar() twice.
3. On navigating, -[AutocompleteTextFieldEditor mouseDown:] ->
OmniboxViewMac::ClosePopup(), then OmniboxViewMac::OnDidEndEditing() ->
OmniboxViewMac::ClosePopup(), then chrome::Navigate() -> ... (twice), then
Browser::UpdateToolbar -> ... (twice). 

[Peter Kasting, 2012/08/15 17:35:31]

The call chains you wrote about don't seem like they would cause a case like you
describe above where we'd have zerosuggestions in progress which would get
stopped.  They seem like cases where the zerosuggest provider ought not to be
running anyway.  Am I wrong, or was there another case or cases that explicitly
broke the zerosuggestion flow?

[Jered, 2012/08/15 18:15:21]

I think it was the Mac mousedown bug. We got focus, started, then mousedown
happened next and stopped it. I reverted this change because the method is
called "StopAutocomplete", it should actually stop it. I will look for a more
specific fix.

+  }
 }
 
 bool OmniboxEditModel::CanPasteAndGo(const string16& text) const {
@@ -531,9 +533,12 @@ void OmniboxEditModel::OpenMatch(const AutocompleteMatch& match,
         base::TimeTicks::Now() - time_user_first_modified_omnibox_,
         0,  // inline autocomplete length; possibly set later
         result());
-    DCHECK(user_input_in_progress_) << "We didn't get here through the "
-        "expected series of calls.  time_user_first_modified_omnibox_ is "
-        "not set correctly and other things may be wrong.";
+    DCHECK(user_input_in_progress_ ||
+           match.type == AutocompleteMatch::SEARCH_ZEROSUGGEST ||

[Peter Kasting, 2012/08/14 19:20:02]

Nit: Don't look at the match type, look at match.provider.name.

[Jered, 2012/08/14 22:27:11]

Done.

+           match.type == AutocompleteMatch::SEARCH_ZEROSUGGEST_URL)
+        << "We didn't get here through the expected series of calls. "
+        << "time_user_first_modified_omnibox_ is not set correctly and other "
+        << "things may be wrong.";
     if (index != OmniboxPopupModel::kNoMatch)
       log.selected_index = index;
     else if (!has_temporary_text_)
@@ -695,6 +700,10 @@ void OmniboxEditModel::OnSetFocus(bool control_down) {
   if (instant)
     instant->OnAutocompleteGotFocus();
 
+  // TODO(jered): Remove this call once suggestions are decoupled from typing
+  // in the omnibox. Then we can just run zero-suggest as a normal provider.

[Peter Kasting, 2012/08/14 19:20:02]

Nit: It's not obvious to me what this comment means.  When you say "decoupled
from typing", I'm thinking of the idea to remove the "what you typed" suggestion
from the dropdown.  That won't have any effect on whether you need to make a
call here to start zerosuggest.  Won't you still need a call here regardless?  I
mean, ultimately if we want to show suggestions on focus, then we'll need to do
something to show suggestions when we get focus.

[Jered, 2012/08/14 22:27:11]

I reworded this to say what I mean, does this makes sense?

+  autocomplete_controller_->StartZeroSuggest(PermanentURL(), user_text_);
+
   NotifySearchTabHelper();
 }
 
@@ -704,6 +713,8 @@ void OmniboxEditModel::OnWillKillFocus(gfx::NativeView view_gaining_focus) {
   if (InstantController* instant = controller_->GetInstant())
     instant->OnAutocompleteLostFocus(view_gaining_focus);
 
+  // TODO(jered): Rip this out along with StartZeroSuggest.
+  autocomplete_controller_->StopZeroSuggest();

[Peter Kasting, 2012/08/14 19:20:02]

This seems unnecessary if you revert your change to StopAutocomplete() above,
since then losing focus will call through to that and unconditionally Stop() all
the providers.

[Jered, 2012/08/14 22:27:11]

Please see comment above.

   NotifySearchTabHelper();
 }