Omnibox: Query in Omnibox - Fix URL to query flicker on Desktop

On Desktop, with Query in Omnibox, the URL would flicker in for a second
while the search page was loading and before it got its security state.

This CL fixes that by ignoring the security state while the page is
loading. This is the same solution that the Android implementation has.

This CL also changes the ignore_security_state flag from a member
variable the caller toggles on and off to an additional parameter that's
passed in when the caller retrieves the query terms.

Bug: 874592
Change-Id: Ic5b8c78798b0119a63b5934c956a6395e4e6d769
Reviewed-on: https://chromium-review.googlesource.com/c/1274896Reviewed-by: Adrienne Porter Felt <felt@chromium.org>
Reviewed-by: Christopher Thompson <cthomp@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Troy Hildebrandt <thildebr@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608435}

chrome/android/java/src/org/chromium/chrome/browser/omnibox/QueryInOmnibox.java

@@ -16,28 +16,19 @@ public class QueryInOmnibox {
      *
      * @param profile The Profile associated with the tab.
      * @param securityLevel The {@link ConnectionSecurityLevel} of the tab.
+     * @param ignoreSecurityLevel When this is set to true, Query in Omnibox ignores the security
+     *                            level when determining whether to display search terms or not.
+     *                            Use this to avoid a flicker during page load for an SRP URL
+     *                            before the SSL state updates.
      * @param url The URL to extract search terms from.
      * @return The extracted search terms. Returns null if the Omnibox should not display the
      *         search terms.
      */
-    public static String getDisplaySearchTerms(
-            Profile profile, @ConnectionSecurityLevel int securityLevel, String url) {
-        return nativeGetDisplaySearchTerms(profile, securityLevel, url);
-    }
-
-    /**
-     * Sets a flag telling the model to ignore the security level in its check for whether to
-     * display search terms or not. This is useful for avoiding the flicker that occurs when
-     * loading a SRP URL before our SSL state updates.
-     *
-     * @param profile The Profile associated with the tab.
-     * @param ignore Whether or not we should ignore the security level.
-     */
-    public static void setIgnoreSecurityLevelForSearchTerms(Profile profile, boolean ignore) {
-        nativeSetIgnoreSecurityLevel(profile, ignore);
+    public static String getDisplaySearchTerms(Profile profile,
+            @ConnectionSecurityLevel int securityLevel, boolean ignoreSecurityLevel, String url) {
+        return nativeGetDisplaySearchTerms(profile, securityLevel, ignoreSecurityLevel, url);
     }
 
     private static native String nativeGetDisplaySearchTerms(
-            Profile profile, int securityLevel, String url);
-    private static native void nativeSetIgnoreSecurityLevel(Profile profile, boolean ignore);
+            Profile profile, int securityLevel, boolean ignoreSecurityLevel, String url);
 }

chrome/android/java/src/org/chromium/chrome/browser/toolbar/LocationBarModel.java

@@ -461,8 +461,12 @@ public class LocationBarModel implements ToolbarDataProvider {
             return null;
         }
 
+        boolean ignoreSecurityLevel = false;
+        if (mNativeLocationBarModelAndroid != 0) {
+            ignoreSecurityLevel = !nativeIsSecurityInfoInitialized(mNativeLocationBarModelAndroid);
+        }
         return QueryInOmnibox.getDisplaySearchTerms(
-                getProfile(), getSecurityLevel(), getCurrentUrl());
+                getProfile(), getSecurityLevel(), ignoreSecurityLevel, getCurrentUrl());
     }
 
     /** @return The formatted URL suitable for editing. */
@@ -481,4 +485,5 @@ public class LocationBarModel implements ToolbarDataProvider {
     private native void nativeDestroy(long nativeLocationBarModelAndroid);
     private native String nativeGetFormattedFullURL(long nativeLocationBarModelAndroid);
     private native String nativeGetURLForDisplay(long nativeLocationBarModelAndroid);
+    private native boolean nativeIsSecurityInfoInitialized(long nativeLocationBarModelAndroid);
 }

chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java

@@ -64,7 +64,6 @@ import org.chromium.chrome.browser.ntp.NewTabPage;
 import org.chromium.chrome.browser.offlinepages.OfflinePageBridge;
 import org.chromium.chrome.browser.omaha.UpdateMenuItemHelper;
 import org.chromium.chrome.browser.omnibox.LocationBar;
-import org.chromium.chrome.browser.omnibox.QueryInOmnibox;
 import org.chromium.chrome.browser.omnibox.UrlFocusChangeListener;
 import org.chromium.chrome.browser.partnercustomizations.HomepageManager;
 import org.chromium.chrome.browser.partnercustomizations.HomepageManager.HomepageStateListener;
@@ -422,7 +421,6 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF
                 if (mLocationBarModel.getTab() == null) return;
 
                 assert tab == mLocationBarModel.getTab();
-                QueryInOmnibox.setIgnoreSecurityLevelForSearchTerms(tab.getProfile(), false);
                 mLocationBar.updateSecurityIcon();
                 mLocationBar.setUrlToPageUrl();
             }
@@ -461,14 +459,8 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF
                 finishLoadProgress(false);
             }
 
-            @Override
-            public void onPageLoadStarted(Tab tab, String url) {
-                QueryInOmnibox.setIgnoreSecurityLevelForSearchTerms(tab.getProfile(), true);
-            }
-
             @Override
             public void onPageLoadFinished(Tab tab) {
-                QueryInOmnibox.setIgnoreSecurityLevelForSearchTerms(tab.getProfile(), false);
                 if (tab.isShowingErrorPage()) {
                     handleIPHForErrorPageShown(tab);
                     return;
@@ -492,7 +484,6 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF
             @Override
             public void onLoadStarted(Tab tab, boolean toDifferentDocument) {
                 if (!toDifferentDocument) return;
-                QueryInOmnibox.setIgnoreSecurityLevelForSearchTerms(tab.getProfile(), true);
                 updateButtonStatus();
                 updateTabLoadingState(true);
             }
@@ -500,7 +491,6 @@ public class ToolbarManager implements ScrimObserver, ToolbarTabController, UrlF
             @Override
             public void onLoadStopped(Tab tab, boolean toDifferentDocument) {
                 if (!toDifferentDocument) return;
-                QueryInOmnibox.setIgnoreSecurityLevelForSearchTerms(tab.getProfile(), false);
                 updateTabLoadingState(true);
 
                 // If we made some progress, fast-forward to complete, otherwise just dismiss any

chrome/browser/android/omnibox/query_in_omnibox_android.cc

@@ -15,6 +15,7 @@ JNI_QueryInOmnibox_GetDisplaySearchTerms(JNIEnv* env,
                                          const JavaParamRef<jclass>&,
                                          const JavaParamRef<jobject>& j_profile,
                                          jint j_security_level,
+                                         jboolean j_ignore_security_level,
                                          const JavaParamRef<jstring>& j_url) {
   Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
   security_state::SecurityLevel security_level =
@@ -24,20 +25,10 @@ JNI_QueryInOmnibox_GetDisplaySearchTerms(JNIEnv* env,
   base::string16 search_terms;
   bool should_display =
       QueryInOmniboxFactory::GetForProfile(profile)->GetDisplaySearchTerms(
-          security_level, url, &search_terms);
+          security_level, j_ignore_security_level, url, &search_terms);
 
   if (!should_display)
     return nullptr;
 
   return base::android::ConvertUTF16ToJavaString(env, search_terms);
 }
-
-void JNI_QueryInOmnibox_SetIgnoreSecurityLevel(
-    JNIEnv* env,
-    const JavaParamRef<jclass>&,
-    const JavaParamRef<jobject>& j_profile,
-    jboolean ignore) {
-  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
-  QueryInOmniboxFactory::GetForProfile(profile)->set_ignore_security_level(
-      ignore);
-}

chrome/browser/ui/android/toolbar/location_bar_model_android.cc

@@ -44,6 +44,12 @@ ScopedJavaLocalRef<jstring> LocationBarModelAndroid::GetURLForDisplay(
       env, location_bar_model_->GetURLForDisplay());
 }
 
+jboolean LocationBarModelAndroid::IsSecurityInfoInitialized(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& obj) {
+  return location_bar_model_->IsSecurityInfoInitialized();
+}
+
 content::WebContents* LocationBarModelAndroid::GetActiveWebContents() const {
   JNIEnv* env = base::android::AttachCurrentThread();
   ScopedJavaLocalRef<jobject> jweb_contents =

chrome/browser/ui/android/toolbar/location_bar_model_android.h

@@ -31,6 +31,9 @@ class LocationBarModelAndroid : public ChromeLocationBarModelDelegate {
   base::android::ScopedJavaLocalRef<jstring> GetURLForDisplay(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj);
+  jboolean IsSecurityInfoInitialized(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj);
 
   // ChromeLocationBarModelDelegate:
   content::WebContents* GetActiveWebContents() const override;

components/omnibox/browser/omnibox_edit_model.cc

@@ -1377,9 +1377,10 @@ bool OmniboxEditModel::GetQueryInOmniboxSearchTerms(
     return false;
 
   LocationBarModel* location_bar_model = controller()->GetLocationBarModel();
+  bool ignore_security_level = !location_bar_model->IsSecurityInfoInitialized();
   return query_in_omnibox->GetDisplaySearchTerms(
       location_bar_model->GetSecurityLevel(false /* ignore_editing */),
-      location_bar_model->GetURL(), search_terms);
+      ignore_security_level, location_bar_model->GetURL(), search_terms);
 }
 
 // static

components/omnibox/browser/query_in_omnibox.cc

@@ -33,12 +33,13 @@ QueryInOmnibox::QueryInOmnibox()
 
 bool QueryInOmnibox::GetDisplaySearchTerms(
     security_state::SecurityLevel security_level,
+    bool ignore_security_level,
     const GURL& url,
     base::string16* search_terms) {
   if (!base::FeatureList::IsEnabled(omnibox::kQueryInOmnibox))
     return false;
 
-  if (!ignore_security_level_ &&
+  if (!ignore_security_level &&
       !SecurityLevelSafeForQueryInOmnibox(security_level)) {
     return false;
   }

components/omnibox/browser/query_in_omnibox.h

@@ -23,11 +23,14 @@ class QueryInOmnibox : public KeyedService {
 
   // Returns true if the toolbar should display the search terms. When this
   // method returns true, the extracted search terms will be filled into
-  // |search_terms| if its not nullptr.
+  // |search_terms| if its not nullptr. |ignore_security_level| can be set to
+  // true to avoid a flicker during page loading for a search results page
+  // before the SSL state updates.
   //
   // This method will return false if any of the following are true:
   //  - Query in Omnibox is disabled
   //  - |security_level| is insufficient to show search terms instead of the URL
+  //    and |ignore_security_level| is false.
   //  - |url| is not from the default search provider
   //  - There are no search terms to extract from |url|
   //  - The extracted search terms look too much like a URL (could confuse user)
@@ -36,16 +39,10 @@ class QueryInOmnibox : public KeyedService {
   // to check the display status only. Virtual for testing purposes.
   virtual bool GetDisplaySearchTerms(
       security_state::SecurityLevel security_level,
+      bool ignore_security_level,
       const GURL& url,
       base::string16* search_terms);
 
-  //  Sets a flag telling the model to ignore the security level in its check
-  // for whether to display search terms or not. This is useful for avoiding the
-  // flicker that occurs when loading a SRP URL before our SSL state updates.
-  void set_ignore_security_level(bool ignore_security_level) {
-    ignore_security_level_ = ignore_security_level;
-  }
-
  protected:
   // For testing only.
   QueryInOmnibox();
@@ -56,10 +53,6 @@ class QueryInOmnibox : public KeyedService {
   // or if the extracted search terms look too much like a URL.
   base::string16 ExtractSearchTermsInternal(const GURL& url);
 
-  // If true, the security level preconditions are ignored for displaying the
-  // query in the omnibox.
-  bool ignore_security_level_ = false;
-
   // Because extracting search terms from a URL string is relatively expensive,
   // and we want to support cheap calls to GetDisplaySearchTerms, cache the
   // result of the last-parsed URL string.

components/omnibox/browser/query_in_omnibox_unittest.cc

@@ -34,40 +34,44 @@ class QueryInOmniboxTest : public testing::Test {
 };
 
 TEST_F(QueryInOmniboxTest, FeatureFlagWorks) {
-  EXPECT_FALSE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE, kValidSearchResultsPage, nullptr));
+  EXPECT_FALSE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
+                                     false, kValidSearchResultsPage, nullptr));
 
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(omnibox::kQueryInOmnibox);
 
-  EXPECT_TRUE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE, kValidSearchResultsPage, nullptr));
+  EXPECT_TRUE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
+                                     false, kValidSearchResultsPage, nullptr));
 }
 
 TEST_F(QueryInOmniboxTest, SecurityLevel) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(omnibox::kQueryInOmnibox);
 
-  EXPECT_TRUE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE, kValidSearchResultsPage, nullptr));
+  EXPECT_TRUE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
+                                     false, kValidSearchResultsPage, nullptr));
   EXPECT_TRUE(
       model()->GetDisplaySearchTerms(security_state::SecurityLevel::EV_SECURE,
-                                     kValidSearchResultsPage, nullptr));
+                                     false, kValidSearchResultsPage, nullptr));
 
   // Insecure levels should not be allowed to display search terms.
-  EXPECT_FALSE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::NONE, kValidSearchResultsPage, nullptr));
   EXPECT_FALSE(
-      model()->GetDisplaySearchTerms(security_state::SecurityLevel::DANGEROUS,
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::NONE, false,
                                      kValidSearchResultsPage, nullptr));
+  EXPECT_FALSE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::DANGEROUS,
+                                     false, kValidSearchResultsPage, nullptr));
 
   // But respect the flag on to ignore the security level.
-  model()->set_ignore_security_level(true);
-  EXPECT_TRUE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::NONE, kValidSearchResultsPage, nullptr));
   EXPECT_TRUE(
-      model()->GetDisplaySearchTerms(security_state::SecurityLevel::DANGEROUS,
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::NONE, true,
                                      kValidSearchResultsPage, nullptr));
+  EXPECT_TRUE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::DANGEROUS,
+                                     true, kValidSearchResultsPage, nullptr));
 }
 
 TEST_F(QueryInOmniboxTest, DefaultSearchProviderWithAndWithoutQuery) {
@@ -75,15 +79,16 @@ TEST_F(QueryInOmniboxTest, DefaultSearchProviderWithAndWithoutQuery) {
   scoped_feature_list.InitAndEnableFeature(omnibox::kQueryInOmnibox);
 
   base::string16 result;
-  EXPECT_TRUE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE, kValidSearchResultsPage, &result));
+  EXPECT_TRUE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
+                                     false, kValidSearchResultsPage, &result));
   EXPECT_EQ(base::ASCIIToUTF16("foo query"), result);
 
   const GURL kDefaultSearchProviderURLWithNoQuery(
       "https://www.google.com/maps");
   result.clear();
   EXPECT_FALSE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE,
+      security_state::SecurityLevel::SECURE, false,
       kDefaultSearchProviderURLWithNoQuery, &result));
   EXPECT_EQ(base::string16(), result);
 }
@@ -95,9 +100,9 @@ TEST_F(QueryInOmniboxTest, NonDefaultSearchProvider) {
   const GURL kNonDefaultSearchProvider(
       "https://search.yahoo.com/search?ei=UTF-8&fr=crmas&p=foo+query");
   base::string16 result;
-  EXPECT_FALSE(
-      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
-                                     kNonDefaultSearchProvider, &result));
+  EXPECT_FALSE(model()->GetDisplaySearchTerms(
+      security_state::SecurityLevel::SECURE, false, kNonDefaultSearchProvider,
+      &result));
   EXPECT_EQ(base::string16(), result);
 }
 
@@ -108,7 +113,8 @@ TEST_F(QueryInOmniboxTest, LookalikeURL) {
   const GURL kLookalikeURLQuery(
       "https://www.google.com/search?q=lookalike.com");
   base::string16 result;
-  EXPECT_FALSE(model()->GetDisplaySearchTerms(
-      security_state::SecurityLevel::SECURE, kLookalikeURLQuery, &result));
+  EXPECT_FALSE(
+      model()->GetDisplaySearchTerms(security_state::SecurityLevel::SECURE,
+                                     false, kLookalikeURLQuery, &result));
   EXPECT_EQ(base::string16(), result);
 }

components/omnibox/browser/test_omnibox_client.cc

@@ -29,6 +29,7 @@ class FakeQueryInOmnibox : public QueryInOmnibox {
 
   // QueryInOmnibox:
   bool GetDisplaySearchTerms(security_state::SecurityLevel security_level,
+                             bool ignore_security_level,
                              const GURL& url,
                              base::string16* search_terms) override {
     if (fake_search_terms_.empty())