Omnibox - Make Omnibox Extensions Ignore Stop() Calls Due to User Idleness

components/omnibox/autocomplete_provider.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_OMNIBOX_AUTOCOMPLETE_PROVIDER_H_
 #define COMPONENTS_OMNIBOX_AUTOCOMPLETE_PROVIDER_H_
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
@@ skipping 133 matching lines @@
   // tracking its matches for this query and whether it is done processing the
   // query.  When new matches are available or the provider finishes, it
   // calls the controller's OnProviderUpdate() method.  The controller can then
   // get the new matches using the provider's accessors.
   // Exception: Matches available immediately after starting the query (that
   // is, synchronously) do not cause any notifications to be sent.  The
   // controller is expected to check for these without prompting (since
   // otherwise, starting each provider running would result in a flurry of
   // notifications).
   //
-  // Once Stop() has been called, no more notifications should be sent.
+  // Once Stop() has been called with user_inactivity_timer=false (i.e., the
+  // user did an action to cancel this input), no more notifications should
+  // be sent.

[Peter Kasting, 2015/03/05 23:27:28]

Nit: How about:

Once Stop() has been called, no more notifications should usually be sent (see
comments on Stop() below).

[Mark P, 2015/03/06 21:24:34]

Took (roughly) your suggestions.  I like removing the redundancy between
comments (which your revision does).

   //
   // |minimal_changes| is an optimization that lets the provider do less work
   // when the |input|'s text hasn't changed.  See the body of
   // OmniboxPopupModel::StartAutocomplete().
   //
   // |called_due_to_focus| is true when Start() is being called in response to
   // the omnibox being focused, instead of due to e.g. user input changes.  Most
   // providers should not provide matches in this case.  Providers which want to
   // display matches on focus can use this flag to know when they can do so.
   virtual void Start(const AutocompleteInput& input,
                      bool minimal_changes,
                      bool called_due_to_focus) = 0;
 
-  // Called when a provider must not make any more callbacks for the current
-  // query. This will be called regardless of whether the provider is already
-  // done.  If the provider caches any results, it should clear the cache based
-  // on the value of |clear_cached_results|.
-  virtual void Stop(bool clear_cached_results);
+  // Called with user_inactivity_timer=false (i.e., the user did an action to
+  // cancel this input) when a provider must not make any more callbacks for
+  // the current query. When called with user_inactivity_timer=true, providers
+  // should generally not make any more callbacks for the current query, though
+  // they are allowed.  Stop() will be called regardless of whether the provider
+  // is already done.  If the provider caches any results, it should clear the
+  // cache based on the value of |clear_cached_results|.

[Peter Kasting, 2015/03/05 23:27:28]

Nit: How about:

Advises the provider to stop processing.  This may be called even if the
provider is already done.  If the provider caches any results, it should clear
the cache based on the value of |clear_cached_results|.  Normally, once this is
called, the provider should not send more notifications to the controller.

If |user_inactivity_timer| is true, Stop() is being called because it's been a
long time since the user started the current query, and returning further
asynchronous results would normally just be disruptive.  Most providers should
still stop processing in this case, but continuing is legal if there's a good
reason the user is likely to want even long-delayed asynchronous results, e.g.
the user has explicitly invoked a keyword extension and the extension is still
processing the request.

[Mark P, 2015/03/06 21:24:34]

Very nice.  Done.

+  virtual void Stop(bool clear_cached_results,
+                    bool user_inactivity_timer);
 
   // Returns the enum equivalent to the name of this provider.
   // TODO(derat): Make metrics use AutocompleteProvider::Type directly, or at
   // least move this method to the metrics directory.
   metrics::OmniboxEventProto_ProviderType AsOmniboxEventProviderType() const;
 
   // Called to delete a match and the backing data that produced it.  This
   // match should not appear again in this or future queries.  This can only be
   // called for matches the provider marks as deletable.  This should only be
   // called when no query is running.
@@ skipping 63 matching lines @@
   ACMatches matches_;
   bool done_;
 
   Type type_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(AutocompleteProvider);
 };
 
 #endif  // COMPONENTS_OMNIBOX_AUTOCOMPLETE_PROVIDER_H_