Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12319124:
Enable query extraction for Android (Closed)
|
Created:
7 years, 10 months ago by Yusuf Modified:
7 years, 9 months ago CC:
chromium-reviews, Ramya, nilesh Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEnable query extraction for Android
This adds the related switch to Android command line and also
implements InstantExtendedEnabledParam() for iOS and Android in a such a
way to add the necessary Replacement param if query extraction is enabled
and the base url is secure.
BUG=177333
TBR=bartfab@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186788
Patch Set 1 #
Total comments: 7
Patch Set 2 : Changed embeddedPageVersion for mobile and cleaned up the rest #
Total comments: 3
Patch Set 3 : Added ifdefs back and made IsExtendedInstantEnabled clearer #
Total comments: 2
Patch Set 4 : Fixed ifdef comments #
Total comments: 10
Patch Set 5 : Changes IsEnabled calls and rebased #Patch Set 6 : Removed kEnableQueryExtraction #
Total comments: 5
Patch Set 7 : Nit fixes #Patch Set 8 : Rebased #Patch Set 9 : Rebased #Patch Set 10 : Fixed rebasing errors #Patch Set 11 : Fixed build errors #
Messages
Total messages: 48 (0 generated)
PTAL. Let me know if there is anyone else I should add to review.
On 2013/02/26 23:37:43, Yusuf wrote: > PTAL. Let me know if there is anyone else I should add to review. +sreeram
On 2013/02/27 00:45:54, dhollowa wrote: > On 2013/02/26 23:37:43, Yusuf wrote: > > PTAL. Let me know if there is anyone else I should add to review. > > +sreeram
lgtm
lgtm
lgtm
pkasting@ for chrome/browser/ui|search_engines|google tedchoc@ for chrome/browser/android OWNERS approval
lgtm for androidy-bits
pkasting@, ping for taking a look at this. Thanks!
https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... File chrome/browser/search_engines/search_terms_data.cc (right): https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:144: #if defined(OS_IOS) || defined(OS_ANDROID) You're omitting the threading assertion and the profile_ NULL-check, both of which seem necessary. https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:146: GURL(GoogleBaseURLValue()).SchemeIsSecure()) Shouldn't we be checking scheme security elsewhere? Instant extended certainly doesn't check it here. https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:148: std::string(google_util::kInstantExtendedDefaultAPIVersion) + "&"; If you're already modifying UIThreadSearchTermsData::InstantExtendedEnabledParam() as your way of accomplishing query extraction, why not modify chrome::search::EmbeddedSearchPageVersion() to return the version number you want? Then you could avoid the #ifs here and, if I'm not mistaken, in the other places you're touching as well.
https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... File chrome/browser/search_engines/search_terms_data.cc (right): https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:144: #if defined(OS_IOS) || defined(OS_ANDROID) On 2013/03/01 21:44:44, Peter Kasting wrote: > You're omitting the threading assertion and the profile_ NULL-check, both of > which seem necessary. Done. https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:146: GURL(GoogleBaseURLValue()).SchemeIsSecure()) You are right. Apparently it is already checked in getSearchTerms which we will be using, so removing this from here. On 2013/03/01 21:44:44, Peter Kasting wrote: > Shouldn't we be checking scheme security elsewhere? Instant extended certainly > doesn't check it here. https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:148: std::string(google_util::kInstantExtendedDefaultAPIVersion) + "&"; On 2013/03/01 21:44:44, Peter Kasting wrote: > If you're already modifying > UIThreadSearchTermsData::InstantExtendedEnabledParam() as your way of > accomplishing query extraction, why not modify > chrome::search::EmbeddedSearchPageVersion() to return the version number you > want? Then you could avoid the #ifs here and, if I'm not mistaken, in the other > places you're touching as well. Done.
Also please sync (to get asvitkine's changes to search.cc). https://codereview.chromium.org/12319124/diff/10001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/10001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:154: if (cl->HasSwitch(switches::kEnableQueryExtraction)); Trailing semicolon oops. https://codereview.chromium.org/12319124/diff/10001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:175: #endif // defined(OS_IOS) || defined(OS_ANDROID) Two spaces before "//".
https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... File chrome/browser/search_engines/search_terms_data.cc (right): https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... chrome/browser/search_engines/search_terms_data.cc:146: GURL(GoogleBaseURLValue()).SchemeIsSecure()) We have to be a little careful here, because we don't want to append espv=1 for HTTP urls. While getSearchTerms() will check for https before performing terms extraction, if we blindly add espv=1, then the server might incorrectly return the 1993 ui for HTTP searches.
On 2013/03/01 22:38:25, rohitrao wrote: > https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... > File chrome/browser/search_engines/search_terms_data.cc (right): > > https://codereview.chromium.org/12319124/diff/1/chrome/browser/search_engines... > chrome/browser/search_engines/search_terms_data.cc:146: > GURL(GoogleBaseURLValue()).SchemeIsSecure()) > We have to be a little careful here, because we don't want to append espv=1 for > HTTP urls. While getSearchTerms() will check for https before performing terms > extraction, if we blindly add espv=1, then the server might incorrectly return > the 1993 ui for HTTP searches. This is a problem that the instant extended folk have to avoid as well (and I believe have avoided). We should make sure the solution used for this case is the same as the one they use if possible. (I'm not even sure offhand if this function can be called in the HTTP case, for example.)
https://codereview.chromium.org/12319124/diff/10001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/10001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:154: if (cl->HasSwitch(switches::kEnableQueryExtraction)); Do we need to use a distinct command-line flag from kEnableInstantExtendedAPI? If not, we could simplify this function more by just #ifing out the first portion for mobile.
> This is a problem that the instant extended folk have to avoid as well (and I > believe have avoided). We should make sure the solution used for this case is > the same as the one they use if possible. Agreed. Sreeram or David, do you know how desktop avoids appending espv=1 for http searches? One more thing to watch out for: we explicitly need IsInstantExtendedAPIEnabled() to return false for mobile, because that function guards a bunch of behavior that we don't want (for example, a different set of omnibox providers). That was part of the reason I split out a separate flag in the first place.
On 2013/03/01 22:44:30, rohitrao wrote:
> > This is a problem that the instant extended folk have to avoid as well (and
I
> > believe have avoided). We should make sure the solution used for this case
is
> > the same as the one they use if possible.
>
> Agreed. Sreeram or David, do you know how desktop avoids appending espv=1 for
> http searches?
We can't totally prevent it. SearchTermsData::InstantExtendedParam() doesn't
check for HTTPS or anything, so if {google:baseURL} is HTTP, it will still add
espv=1 to it. However, we have three lines of defense:
1. Before Instant uses the resulting URL to load its hidden overlay, it fixes it
up by forcing the scheme to HTTPS (see InstantController::GetInstantURL()).
2. Instant can also work with existing tabs. If the user visits
http://...?espv=1 manually, Instant will refuse to connect to that page using
the searchbox API.
3. I believe the server also ignores or forces an HTTPS redirect before
respecting espv=1.
> One more thing to watch out for: we explicitly need
> IsInstantExtendedAPIEnabled() to return false for mobile, because that
function
> guards a bunch of behavior that we don't want (for example, a different set of
> omnibox providers). That was part of the reason I split out a separate flag
in
> the first place.
This is a good point. As this CL stands, this part is broken.
Upload another version with a few more ifdefs than I would have preferred, but this keeps InstantExtendedEnabledParam the same and shared, and it also makes clear that we want extendedInstant disabled for mobile. PTAL. (Assuming we are OK with adding espv=1 when using http worries)
I'm uncomfortable with adding espv=1 for http queries, because the resulting
search results ui will be completely broken (we won't show the query anywhere).
We need to either strip it out or not append it.
The first two of Sreeram's defenses won't apply on mobile, because there's no
Instant. We could probably change the server to ignore espv for HTTP queries --
I don't know if that's more or less horrible than checking the scheme on the
client.
Another option is to add a separate {google:mobileQueryExtraction} templateurl
parameter that will map to "espv=1" on mobile. That way we wouldn't overload
any of this EmbeddedSearchPage/InstantExtended logic at all. Peter, would you
prefer that approach? (The implementation of
UISearchTermsData::MobileQueryExtractionParameter() would check the scheme and
would return nothing on desktop OSes.)
On 2013/03/01 23:38:48, rohitrao wrote:
> We could probably change the server to ignore espv for HTTP queries --
> I don't know if that's more or less horrible than checking the scheme on the
> client.
Seems like you should do this anyway. You may get bogus client requests, you
should do the right thing when it happens.
> Another option is to add a separate {google:mobileQueryExtraction} templateurl
> parameter that will map to "espv=1" on mobile.
I'd rather avoid this if we can.
Ok, I'll check with the GWS folks next week and work on getting the server to ignore espv on HTTP requests.
On 2013/03/01 23:38:48, rohitrao wrote: > I'm uncomfortable with adding espv=1 for http queries, because the resulting > search results ui will be completely broken (we won't show the query anywhere). Note that this problem exists on Desktop too, in a slightly different way. Even if you are on an HTTPS espv=1 search results page, if the thing you searched for is a URL, then we'll show the full "https://google.com/?espv=1&q=facebook.com" URL in the omnibox, to prevent URL spoofing attacks. So, perhaps we'll fix that by having the search results page show an in-page searchbox whenever it detects that the query is URL-shaped. Similarly, it can do that if the client is Android and it's not HTTPS.
https://codereview.chromium.org/12319124/diff/17001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/17001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:150: #endif // defined(OS_IOS) || defined(OS_ANDROID) Nit: I meant two spaces _before_ the "//", not after the "//".
So we agree that http concerns will be addressed on the server side and this patch stays as is? Or is there anything else? https://codereview.chromium.org/12319124/diff/17001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/17001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:150: #endif // defined(OS_IOS) || defined(OS_ANDROID) :) Sorry about that.Fixed now. On 2013/03/02 00:16:18, sreeram wrote: > Nit: I meant two spaces _before_ the "//", not after the "//".
lgtm
pkasting@ can you have a final look at this then?
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:149: return EmbeddedSearchPageVersion(profile) != 0; 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. https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) 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.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:149: return EmbeddedSearchPageVersion(profile) != 0; 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: > 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.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:149: return EmbeddedSearchPageVersion(profile) != 0; 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. https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it triggers other behavior that we do not want.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:149: return EmbeddedSearchPageVersion(profile) != 0; Oh, OK. Got it now. Fixed in latest patch. On 2013/03/04 23:16:35, rohitrao wrote: > 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.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) On 2013/03/04 23:16:35, rohitrao wrote: > I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it > triggers other behavior that we do not want. 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?
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) On 2013/03/05 00:19:45, Peter Kasting wrote: > On 2013/03/04 23:16:35, rohitrao wrote: > > I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it > > triggers other behavior that we do not want. > > 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? 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.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) On 2013/03/05 00:29:41, rohitrao wrote: > On 2013/03/05 00:19:45, Peter Kasting wrote: > > On 2013/03/04 23:16:35, rohitrao wrote: > > > I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it > > > triggers other behavior that we do not want. > > > > 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? > > 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) 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.
https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/8005/chrome/browser/ui/search/s... chrome/browser/ui/search/search.cc:158: if (cl->HasSwitch(switches::kEnableQueryExtraction)) 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: > On 2013/03/05 00:29:41, rohitrao wrote: > > On 2013/03/05 00:19:45, Peter Kasting wrote: > > > On 2013/03/04 23:16:35, rohitrao wrote: > > > > I don't want to use switches::kEnableInstantExtendedAPI on mobile, as it > > > > triggers other behavior that we do not want. > > > > > > 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? > > > > 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) > > 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.
Yeah, this is exactly what I was suggesting. Rohit, are you OK with this? Code-wise I think it's as simple as you can get. I realize that making a flag mean slightly different things on mobile and desktop is a little weird, but getting down to two ifdefs in search.cc seems like a good payoff, and if we ever want to bump mobile up to the full extended experience it'd be really trivial. https://codereview.chromium.org/12319124/diff/22001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/12319124/diff/22001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:515: // Enable Instant extended API. Nit: You might want to add: On mobile, this merely enables query extraction, not the rest of the instant-extended functionality.
LGTM It feels weird to reuse the flag with a different meaning on mobile, but fewer #ifdefs is always nice.
https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:163: // On desktop, query extraction is part of instant extended, so if one is Nit: instant -> instant
https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:163: // On desktop, query extraction is part of instant extended, so if one is On 2013/03/05 17:45:55, sreeram wrote: > Nit: instant -> instant Gah. I meant: instant -> Instant
https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... File chrome/browser/ui/search/search.cc (right): https://codereview.chromium.org/12319124/diff/22001/chrome/browser/ui/search/... chrome/browser/ui/search/search.cc:163: // On desktop, query extraction is part of instant extended, so if one is On 2013/03/05 17:46:24, sreeram wrote: > On 2013/03/05 17:45:55, sreeram wrote: > > Nit: instant -> instant > > Gah. I meant: instant -> Instant Done. https://codereview.chromium.org/12319124/diff/22001/chrome/common/chrome_swit... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/12319124/diff/22001/chrome/common/chrome_swit... chrome/common/chrome_switches.cc:515: // Enable Instant extended API. On 2013/03/05 06:19:57, Peter Kasting wrote: > Nit: You might want to add: > > On mobile, this merely enables query extraction, not the rest of the > instant-extended functionality. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12319124/36001
Failed to apply patch for chrome/browser/ui/search/search.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/search/search.cc
Hunk #1 succeeded at 46 (offset 12 lines).
Hunk #2 FAILED at 245.
Hunk #3 FAILED at 277.
Hunk #4 FAILED at 352.
3 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/search.cc.rej
Patch: 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
14309b995e9cf3124a3405c0a504b1641ca85a05..522b1313c80a438cd7af8363d5c00539526fdeeb
100644
--- a/chrome/browser/ui/search/search.cc
+++ b/chrome/browser/ui/search/search.cc
@@ -34,7 +34,11 @@ namespace {
// space-delimited list of key:value pairs which correspond to these flags:
const char kEmbeddedPageVersionFlagName[] = "espv";
const uint64 kEmbeddedPageVersionDisabled = 0;
+#if defined(OS_IOS) || defined(OS_ANDROID)
+const uint64 kEmbeddedPageVersionDefault = 1;
+#else
const uint64 kEmbeddedPageVersionDefault = 2;
+#endif
const char kInstantExtendedActivationName[] = "instant";
const InstantExtendedDefault kInstantExtendedActivationDefault =
@@ -241,7 +245,13 @@ InstantExtendedDefault GetInstantExtendedDefaultSetting() {
}
bool IsInstantExtendedAPIEnabled(const Profile* profile) {
- return EmbeddedSearchPageVersion(profile) != kEmbeddedPageVersionDisabled;
+#if defined(OS_IOS) || defined(OS_ANDROID)
+ return false;
+#else
+ // On desktop, query extraction is part of Instant extended, so if one is
+ // enabled, the other is too.
+ return IsQueryExtractionEnabled(profile);
+#endif // defined(OS_IOS) || defined(OS_ANDROID)
}
// Determine what embedded search page version to request from the user's
@@ -273,19 +283,11 @@ uint64 EmbeddedSearchPageVersion(const Profile* profile) {
kEmbeddedPageVersionDefault,
flags);
}
-
return kEmbeddedPageVersionDisabled;
}
bool IsQueryExtractionEnabled(const Profile* profile) {
-#if defined(OS_IOS)
- 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
+ return EmbeddedSearchPageVersion(profile) != kEmbeddedPageVersionDisabled;
}
string16 GetSearchTermsFromNavigationEntry(
@@ -348,15 +350,6 @@ void EnableInstantExtendedAPIForTesting() {
cl->AppendSwitch(switches::kEnableInstantExtendedAPI);
}
-void EnableQueryExtractionForTesting() {
-#if defined(OS_IOS)
- CommandLine* cl = CommandLine::ForCurrentProcess();
- cl->AppendSwitch(switches::kEnableQueryExtraction);
-#else
- EnableInstantExtendedAPIForTesting();
-#endif
-}
-
bool ShouldAssignURLToInstantRendererImpl(const GURL& url,
bool extended_api_enabled,
TemplateURL* template_url) {
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12319124/49001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12319124/60002
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
Adding bartfab@ to TBR for the chrome/browser/policy change.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12319124/63001
Message was sent while issue was closed.
Change committed as 186788 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
