[omnibox] Slight refactors to AutocompleteController::in_start_

This makes some minor refactors to the usage of
AutocompleteController::in_start_.

 1. It delays setting it to false until after the UpdateResult call
    within the synchronous pass. This is more accurate, and also allows
    in_start_ to be used as a synonoym of in_synchronous_pass_.

 2. It also moves in_start_ to be an early exit condition for
    OnProviderUpdate. It likely doesn't need to be checked there at
    all, since the comments indicate it's likely ALWAYS false when
    OnProviderUpdate is called.

These refactors help pave the way for the feature to preserve the
default match during the asynchronous pass, by clarifying in the code
when the synchronous vs. asynchronous pass occurs.

Bug: 398135
Change-Id: I8f8c61b8233b1c049338dac448b6ef8f1b0e3b93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789823Reviewed-by: Kevin Bailey <krb@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#694850}

components/omnibox/browser/autocomplete_controller.cc

@@ -369,8 +369,11 @@ void AutocompleteController::Start(const AutocompleteInput& input) {
         name, 1, 1000, 50, base::Histogram::kUmaTargetedHistogramFlag);
     counter->Add(static_cast<int>((end_time - start_time).InMilliseconds()));
   }
-  in_start_ = false;
+
+  // This will usually set |done_| to false, unless all of the providers are
+  // are finished after the synchronous pass we just completed.
   CheckIfDone();
+
   // The second true forces saying the default match has changed.
   // This triggers the edit model to update things such as the inline
   // autocomplete state.  In particular, if the user has typed a key
@@ -386,6 +389,8 @@ void AutocompleteController::Start(const AutocompleteInput& input) {
   // need the edit model to update the display.
   UpdateResult(false, true);
 
+  in_start_ = false;
+
   // Omnibox has dependencies that may be lazily initialized. This metric will
   // help tracking regression on the first use.
   if (first_query_) {
@@ -464,10 +469,20 @@ void AutocompleteController::ExpireCopiedEntries() {
 }
 
 void AutocompleteController::OnProviderUpdate(bool updated_matches) {
+  // Providers should only call this method during the asynchronous pass.
+  // There's no reason to call this during the synchronous pass, since we
+  // perform these operations anyways after all providers are started.
+  //
+  // This is not a DCHECK, because in the unusual case that a provider calls an
+  // asynchronous method, and that method early exits by calling the callback
+  // immediately, it's not necessarily a programmer error. We should just no-op.
+  if (in_start_)
+    return;
+
   CheckIfDone();
   // Multiple providers may provide synchronous results, so we only update the
   // results if we're not in Start().
-  if (!in_start_ && (updated_matches || done_))
+  if (updated_matches || done_)
     UpdateResult(false, false);
 }
 

components/omnibox/browser/autocomplete_controller.h

@@ -280,7 +280,8 @@ class AutocompleteController : public AutocompleteProviderListener,
   bool done_;
 
   // Are we in Start()? This is used to avoid updating |result_| and sending
-  // notifications until Start() has been invoked on all providers.
+  // notifications until Start() has been invoked on all providers. When this
+  // boolean is true, we are definitely within the synchronous pass.
   bool in_start_;
 
   // Indicate whether it is the first query since startup.

components/omnibox/browser/autocomplete_provider_listener.h

@@ -16,6 +16,9 @@ class AutocompleteProviderListener {
   // NOTE: Providers MUST only call this method while processing asynchronous
   // queries.  Do not call this for a synchronous query.
   //
+  // NOTE: If a provider has finished, it should set done() to true BEFORE
+  // calling this method.
+  //
   // NOTE: There's no parameter to tell the listener _which_ provider is
   // calling it.  Because the AutocompleteController (the typical listener)
   // doesn't cache the providers' individual matches locally, it has to get

components/omnibox/browser/clipboard_provider.cc

@@ -295,6 +295,10 @@ void ClipboardProvider::ConstructImageMatchCallback(
 
   match.transition = ui::PAGE_TRANSITION_GENERATED;
 
+  field_trial_triggered_ = true;
+  field_trial_triggered_in_session_ = true;
+  done_ = true;
+
   // Some users may be in a counterfactual study arm in which we perform all
   // necessary work but do not forward the autocomplete matches.
   bool in_counterfactual_group = base::GetFieldTrialParamByFeatureAsBool(
@@ -304,9 +308,6 @@ void ClipboardProvider::ConstructImageMatchCallback(
     AddCreatedMatchWithTracking(input, match, clipboard_contents_age);
     listener_->OnProviderUpdate(true);
   }
-  field_trial_triggered_ = true;
-  field_trial_triggered_in_session_ = true;
-  done_ = true;
 }
 
 void ClipboardProvider::AddProviderInfo(ProvidersInfo* provider_info) const {