Enable query extraction for Android

chrome/browser/ui/search/search.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/ui/search/search.h"
 
 #include "base/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/string_split.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 11 matching lines @@
 namespace {
 
 // Configuration options for Embedded Search.
 // InstantExtended field trials are named in such a way that we can parse out
 // the experiment configuration from the trial's group name in order to give
 // us maximum flexability in running experiments.
 // Field trial groups should be named things like "Group7 espv:2 instant:1".
 // 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 =
     chrome::search::INSTANT_DEFAULT_ON;
 
 // Constants for the field trial name and group prefix.
 const char kInstantExtendedFieldTrialName[] = "InstantExtended";
 const char kGroupNumberPrefix[] = "Group";
 
 // If the field trial's group name ends with this string its configuration will
@@ skipping 88 matching lines @@
                                kInstantExtendedActivationName,
                                kInstantExtendedActivationDefault,
                                flags);
     return InstantExtendedDefaultFromInt64(trial_default);
   }
 
   return kInstantExtendedActivationDefault;
 }
 
 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;
 
   FieldTrialFlags flags;
   if (GetFieldTrialInfo(
           base::FieldTrialList::FindFullName(kInstantExtendedFieldTrialName),
           &flags, NULL)) {
     return GetUInt64ValueForFlagWithDefault(kEmbeddedPageVersionFlagName,
                                             kEmbeddedPageVersionDefault,
                                             flags);
   }
 
   const CommandLine* cl = CommandLine::ForCurrentProcess();
   if (cl->HasSwitch(switches::kEnableInstantExtendedAPI)) {
     // The user has manually flipped the about:flags switch - give the default
     // 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(
     const content::NavigationEntry* entry) {
   string16 search_terms;
   if (entry)
     entry->GetExtraData(kInstantExtendedSearchTermsKey, &search_terms);
   return search_terms;
 }
 
@@ skipping 67 matching lines @@
              IsInstantExtendedAPIEnabled(profile),
              template_url);
 }
 
 void EnableInstantExtendedAPIForTesting() {
   CommandLine* cl = CommandLine::ForCurrentProcess();
   cl->AppendSwitch(switches::kEnableInstantExtendedAPI);
 }
 
 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,
                                           bool extended_api_enabled,
                                           TemplateURL* template_url) {
   if (!url.is_valid())
     return false;
 
   if (url.SchemeIs(chrome::kChromeSearchScheme))
     return true;
@@ skipping 84 matching lines @@
 // Given a FieldTrialFlags object, returns the boolean value of the provided
 // flag.
 bool GetBoolValueForFlagWithDefault(const std::string& flag,
                                     bool default_value,
                                     const FieldTrialFlags& flags) {
   return !!GetUInt64ValueForFlagWithDefault(flag, default_value ? 1 : 0, flags);
 }
 
 }  // namespace search
 }  // namespace chrome