Enable query extraction for Android

chrome/browser/ui/search/search.cc

Index: chrome/browser/ui/search/search.cc
diff --git a/chrome/browser/ui/search/search.cc b/chrome/browser/ui/search/search.cc
index e6d79bd0b396f29bd7fe61063768d7ef381625c2..0861971d2e6f598fc91e79de16655056bd774c90 100644
--- a/chrome/browser/ui/search/search.cc
+++ b/chrome/browser/ui/search/search.cc
@@ -29,7 +29,12 @@ namespace {
 // The first token is always GroupN for some integer N, followed by a
 // space-delimited list of key:value pairs which correspond to these flags:
 const char kEmbeddedPageVersionFlagName[] = "espv";
+
+#if defined(OS_IOS) || defined(OS_ANDROID)
+const int kEmbeddedPageVersionDefault = 1;
+#else
 const int kEmbeddedPageVersionDefault = 2;
+#endif
 
 const char kInstantExtendedActivationName[] = "instant";
 const chrome::search::InstantExtendedDefault kInstantExtendedActivationDefault =
@@ -138,12 +143,21 @@ InstantExtendedDefault GetInstantExtendedDefaultSetting() {
 }
 
 bool IsInstantExtendedAPIEnabled(const Profile* profile) {
+#if defined(OS_IOS) || defined(OS_ANDROID)
+  return false;
+#else
   return EmbeddedSearchPageVersion(profile) != 0;

[Peter Kasting, 2013/03/04 23:01:18]

Nit: If you change this as follows:

  // On desktop, query extraction is pat of instant extended, so if one is
  // enabled, the other is too.
  return IsQueryExtractionEnabled(profile);

Then you can simply IsQueryExtractionEnabled to just unconditionally do:

  return EmbeddedSearchPageVersion(profile) != 0;

...and remove all the ifdefs from there.

[Yusuf, 2013/03/04 23:09:37]

What I understood from Rohit's comment above was that we want
IsInstantExtendedAPIEnabled to return false on  mobile since this makes changes
that we dont want to the UI.

Rohit please correct me if this is wrong, but as I understand it, we do want
EnableQueryExtraction and EnableExtendedInstant to mean different things. So one
doesn't contain the other, they are mutually exclusive.


On 2013/03/04 23:01:18, Peter Kasting wrote:

[rohitrao (ping after 24h), 2013/03/04 23:16:35]

Peter was suggesting changing this method to read:

#if defined(OS_IOS) || defined(OS_ANDROID)
  return false;
#else
  return IsQueryExtractionEnabled(profile);
#endif

And simplifying IsQueryExtractionEnabled to be platform-independent:

return EmbeddedSearchPageVersion(profile) != 0;


This should work, but it seems backwards, in that on desktop, query extraction
is really a subfeature of instant-extended, but the code reverses that.  I don't
particularly care either way, though.

[Yusuf, 2013/03/04 23:36:36]

Oh, OK. Got it now. Fixed in latest patch.


On 2013/03/04 23:16:35, rohitrao wrote:

+#endif  // defined(OS_IOS) || defined(OS_ANDROID)
 }
 
 // Determine what embedded search page version to request from the user's
 // default search provider. If 0, the embedded search UI should not be enabled.
 uint64 EmbeddedSearchPageVersion(const Profile* profile) {
+#if defined(OS_IOS) || defined(OS_ANDROID)
+  const CommandLine* cl = CommandLine::ForCurrentProcess();
+  if (cl->HasSwitch(switches::kEnableQueryExtraction))

[Peter Kasting, 2013/03/04 23:01:18]

You didn't answer my previous question about whether we could eliminate this
separate flag name, and instead use switches::kEnableInstantExtendedAPI on
mobile (and just have the meaning be the current meaning of
kEnableQueryExtraction).  If so, the implementation of this function can be
simplified, and you can entirely eliminate EnableQueryExtractionForTesting() and
just replace calls to it with calls to EnableInstantExtendedAPIForTesting().

If this is technically possible but you worry it could be confusing, we could
actually make the unified flag name be kEnableQueryExtraction (and similarly
with the "enable for testing" function name) since, as I noted above, on
desktop, the idea of "query extraction enabled" and "instant extended enabled"
are always true/false at the same time.

[rohitrao (ping after 24h), 2013/03/04 23:16:35]

I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it
triggers other behavior that we do not want.

[Peter Kasting, 2013/03/05 00:19:45]

What other code does it trigger?  I assumed everyone who wanted this gated
things on these oracle functions.  Perhaps more code is checking flags directly
that should be using these oracles?

[rohitrao (ping after 24h), 2013/03/05 00:29:41]

IsInstantExtendedAPIEnabled() triggers things like:
- A different set of omnibox providers
- Different UI on desktop (different NTP, different omnibox popup, adding
recently closed to the wrench menu)

I think most code is properly gated on one of these two functions.  On mobile,
we want just the query extraction changes without all of the other instant
extended behavior.

[Peter Kasting, 2013/03/05 00:35:31]

Yeah, I'm not asking IsIntantExtendedAPIEnabled() to trigger for mobile.  My
proposal keeps it off.

What I proposed should have zero functional effect if other code does not check
the command line flags directly.  It just saves the confusion of two flags that
the same function checks via ifdefs and makes stuff simpler.

[Yusuf, 2013/03/05 05:57:24]

Uploaded a patch that removed kEnableQueryExtraction and uses
kEnableInstantExtendedAPI for mobile as well. Rohit please let me know if there
is anything I am missing.


On 2013/03/05 00:35:31, Peter Kasting wrote:

+    return kEmbeddedPageVersionDefault;
+#else
   if (!profile || profile->IsOffTheRecord())
     return 0;
 
@@ -162,19 +176,19 @@ uint64 EmbeddedSearchPageVersion(const Profile* profile) {
     // UI version.
     return kEmbeddedPageVersionDefault;
   }
-
+#endif  // defined(OS_IOS) || defined(OS_ANDROID)
   return 0;
 }
 
 bool IsQueryExtractionEnabled(const Profile* profile) {
-#if defined(OS_IOS)
+#if defined(OS_IOS) || defined(OS_ANDROID)
   const CommandLine* cl = CommandLine::ForCurrentProcess();
   return cl->HasSwitch(switches::kEnableQueryExtraction);
 #else
   // On desktop, query extraction is controlled by the instant-extended-api
   // flag.
   return IsInstantExtendedAPIEnabled(profile);
-#endif
+#endif  // defined(OS_IOS) || defined(OS_ANDROID)
 }
 
 string16 GetSearchTermsFromNavigationEntry(
@@ -262,12 +276,12 @@ void EnableInstantExtendedAPIForTesting() {
 }
 
 void EnableQueryExtractionForTesting() {
-#if defined(OS_IOS)
+#if defined(OS_IOS) || defined(OS_ANDROID)
   CommandLine* cl = CommandLine::ForCurrentProcess();
   cl->AppendSwitch(switches::kEnableQueryExtraction);
 #else
   EnableInstantExtendedAPIForTesting();
-#endif
+#endif  // defined(OS_IOS) || defined(OS_ANDROID)
 }
 
 bool ShouldAssignURLToInstantRendererImpl(const GURL& url,