Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
151813002:
Adds InstantExtendedEnabledParam to the search request URL iff the query extraction flag is enabled. (Closed)
|
Created:
6 years, 10 months ago by kmadhusu Modified:
6 years, 10 months ago CC:
chromium-reviews, James Su, Yusuf Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds InstantExtendedEnabledParam('&espv') to the search request URL iff the query extraction flag is enabled in field trials.
- When the query extraction is turned off, InstantExtended UI is not enabled in search results page. Therefore, InstantExtendedEnabledParam is unused.
- InstantExtendedEnabledParam() need not be a part of SearchTermsData. Therefore, moved InstantExtendedEnabledParam() from SearchTermsData to search.cc.
- Added unit tests.
BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251009
Patch Set 1 : '' #
Total comments: 2
Patch Set 2 : Addressed nit #Patch Set 3 : Fix compile error #
Total comments: 7
Patch Set 4 : '' #Patch Set 5 : Update comment #
Total comments: 8
Patch Set 6 : Fixed toolbar_model_android code #
Total comments: 4
Patch Set 7 : Addressed comments #
Total comments: 6
Patch Set 8 : Rebase #Patch Set 9 : Addressed comments #Patch Set 10 : Rebase #Patch Set 11 : Rebase #Patch Set 12 : Rebase #
Messages
Total messages: 29 (0 generated)
samarth@chromium.org: Please review changes in chrome/browser/search/*. Once this CL lands, I need to update chrome::GetSearchTermsFromURL() to extract search terms even if InstantExtendedEnabledParam() is not available in the URL. pkasting@chromium.org: Please review changes in - chrome/browser/autocomplete/*, - chrome/browser/ui/android/* and - chrome/browser/search_engines/*. beaudoin@chromium.org: Please review changes in history_url_provider.cc Thanks.
LGTM https://chromiumcodereview.appspot.com/151813002/diff/160001/chrome/browser/u... File chrome/browser/ui/android/toolbar/toolbar_model_android.cc (right): https://chromiumcodereview.appspot.com/151813002/diff/160001/chrome/browser/u... chrome/browser/ui/android/toolbar/toolbar_model_android.cc:46: chrome::InstantExtendedEnabledParam(false /* for_search (ignored) */); Nit: Omit comment -- since there's only one arg it's easy for readers to look up the param name if they don't know it.
pkasting@: Addressed nit. Thanks. https://chromiumcodereview.appspot.com/151813002/diff/160001/chrome/browser/u... File chrome/browser/ui/android/toolbar/toolbar_model_android.cc (right): https://chromiumcodereview.appspot.com/151813002/diff/160001/chrome/browser/u... chrome/browser/ui/android/toolbar/toolbar_model_android.cc:46: chrome::InstantExtendedEnabledParam(false /* for_search (ignored) */); On 2014/02/04 00:34:53, Peter Kasting wrote: > Nit: Omit comment -- since there's only one arg it's easy for readers to look up > the param name if they don't know it. Done.
LGTM small nit. https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... chrome/browser/search/search_unittest.cc:780: TemplateURL* template_url_; Nit: Make this a scoped_ptr<> so it's deallocated when test ends. https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... File chrome/browser/search_engines/search_terms_data.h (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... chrome/browser/search_engines/search_terms_data.h:79: // values, and NTPIsThemedParam() will return an empty string. Why did you remove the bit about ForceInstantResultsParam? Is it no longer true?
https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... chrome/browser/search/search_unittest.cc:780: TemplateURL* template_url_; On 2014/02/04 13:55:23, beaudoin wrote: > Nit: Make this a scoped_ptr<> so it's deallocated when test ends. I have a raw pointer because TemplateURLService took ownership of |template_url_| at line 776. https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... File chrome/browser/search_engines/search_terms_data.h (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... chrome/browser/search_engines/search_terms_data.h:79: // values, and NTPIsThemedParam() will return an empty string. On 2014/02/04 13:55:23, beaudoin wrote: > Why did you remove the bit about ForceInstantResultsParam? Is it no longer true? ForceInstantResultsParam() doesn't access Profile. Therefore, I updated the comment. It can also be moved out of SearchTermsData. I will do that clean up in a separate CL.
LGTM with nit on comment. https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... chrome/browser/search/search_unittest.cc:780: TemplateURL* template_url_; On 2014/02/04 16:52:26, kmadhusu wrote: > On 2014/02/04 13:55:23, beaudoin wrote: > > Nit: Make this a scoped_ptr<> so it's deallocated when test ends. > > I have a raw pointer because TemplateURLService took ownership of > |template_url_| at line 776. SG. Just update the comment to make it clearer: // |template_url_service| takes ownership of |template_url|. https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... File chrome/browser/search_engines/search_terms_data.h (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search_en... chrome/browser/search_engines/search_terms_data.h:79: // values, and NTPIsThemedParam() will return an empty string. On 2014/02/04 16:52:26, kmadhusu wrote: > On 2014/02/04 13:55:23, beaudoin wrote: > > Why did you remove the bit about ForceInstantResultsParam? Is it no longer > true? > > ForceInstantResultsParam() doesn't access Profile. Therefore, I updated the > comment. It can also be moved out of SearchTermsData. I will do that clean up in > a separate CL. > ack
https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... File chrome/browser/search/search_unittest.cc (right): https://codereview.chromium.org/151813002/diff/60006/chrome/browser/search/se... chrome/browser/search/search_unittest.cc:780: TemplateURL* template_url_; On 2014/02/04 16:57:07, beaudoin wrote: > On 2014/02/04 16:52:26, kmadhusu wrote: > > On 2014/02/04 13:55:23, beaudoin wrote: > > > Nit: Make this a scoped_ptr<> so it's deallocated when test ends. > > > > I have a raw pointer because TemplateURLService took ownership of > > |template_url_| at line 776. > > SG. Just update the comment to make it clearer: > // |template_url_service| takes ownership of |template_url|. Done.
On 2014/02/03 23:52:56, kmadhusu wrote: > samarth@chromium.org: Please review changes in chrome/browser/search/*. Once > this CL lands, I need to update chrome::GetSearchTermsFromURL() to extract > search terms even if InstantExtendedEnabledParam() is not available in the URL. Is that because you need it for Chrome Instant? Be careful here because in general, no "espv" means query extraction is disabled and so GetSearchTermsFromURL should not return anything. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.cc:366: #if !defined(OS_IOS) && !defined(OS_ANDROID) Why are these ifdefs necessary? IsQueryExtractionEnabled already returns true for mobile. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.h (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.h:79: // On mobile, |for_search| param is unused because the query extraction is This is confusing. Why isn't |for_search| set correctly on mobile? Looking at the callsites, I think toolbar_model_android.cc should be setting |for_search| to true.
On 2014/02/04 17:17:04, samarth wrote: > On 2014/02/03 23:52:56, kmadhusu wrote: > > samarth@chromium.org: Please review changes in chrome/browser/search/*. Once > > this CL lands, I need to update chrome::GetSearchTermsFromURL() to extract > > search terms even if InstantExtendedEnabledParam() is not available in the > URL. > > Is that because you need it for Chrome Instant? Be careful here because in > general, no "espv" means query extraction is disabled and so > GetSearchTermsFromURL should not return anything. Yes. InstantSearchPrerenderer calls GetSearchTermsFromURL(). I will either update chrome::GetSearchTermsFromURL() or create a new helper function to extract the search terms for InstantSearchPrerenderer.
yusufo@: Please review changes in toolbar_model_android.cc. tedchoc@ mentioned that the modified code is called on every page load, but only comes into effect when on the SRP w/query in the omnibox. Is there a way to find that the function is called for search requests? Thanks. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.cc:366: #if !defined(OS_IOS) && !defined(OS_ANDROID) On 2014/02/04 17:17:05, samarth wrote: > Why are these ifdefs necessary? IsQueryExtractionEnabled already returns true > for mobile. ifdefs are not required here. Since I passed "false" in toolbar_model_android.cc, I thought it would be better to add ifdefs here so that I can revisit the code and update later. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.h (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.h:79: // On mobile, |for_search| param is unused because the query extraction is On 2014/02/04 17:17:05, samarth wrote: > This is confusing. Why isn't |for_search| set correctly on mobile? Looking at > the callsites, I think toolbar_model_android.cc should be setting |for_search| > to true. I am not sure about the callers of that function. That's why I passed false to be on the safer side. Adding yusufo@ to review toolbar_model_android.cc changes.
On 2014/02/04 17:49:58, kmadhusu wrote: > yusufo@: Please review changes in toolbar_model_android.cc. tedchoc@ mentioned > that the modified code is called on every page load, but only comes into effect > when on the SRP w/query in the omnibox. Is there a way to find that the function > is called for search requests? > > Thanks. > > https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... > File chrome/browser/search/search.cc (right): > > https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... > chrome/browser/search/search.cc:366: #if !defined(OS_IOS) && > !defined(OS_ANDROID) > On 2014/02/04 17:17:05, samarth wrote: > > Why are these ifdefs necessary? IsQueryExtractionEnabled already returns true > > for mobile. > > ifdefs are not required here. Since I passed "false" in > toolbar_model_android.cc, I thought it would be better to add ifdefs here so > that I can revisit the code and update later. > > https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... > File chrome/browser/search/search.h (right): > > https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... > chrome/browser/search/search.h:79: // On mobile, |for_search| param is unused > because the query extraction is > On 2014/02/04 17:17:05, samarth wrote: > > This is confusing. Why isn't |for_search| set correctly on mobile? Looking at > > the callsites, I think toolbar_model_android.cc should be setting |for_search| > > to true. > > I am not sure about the callers of that function. That's why I passed false to > be on the safer side. Adding yusufo@ to review toolbar_model_android.cc changes. Yes, I see that the way this works on mobile is a little bit backwards wrt having a for_search parameter. We call this function and only if the url contains the InstantExtendedEnabledParam we check whether getSearchTerms is empty or not. It will still work both ways, but I do agree that it is a bit confusing.
https://codereview.chromium.org/151813002/diff/480001/chrome/browser/ui/andro... File chrome/browser/ui/android/toolbar/toolbar_model_android.cc (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/ui/andro... chrome/browser/ui/android/toolbar/toolbar_model_android.cc:45: env, chrome::InstantExtendedEnabledParam(false)); I do agree with samarth we should be returning true here. Since for mobile query extraction is always enabled, we will never end up returning empty string anyway.
https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.cc:366: #if !defined(OS_IOS) && !defined(OS_ANDROID) On 2014/02/04 17:49:59, kmadhusu wrote: > On 2014/02/04 17:17:05, samarth wrote: > > Why are these ifdefs necessary? IsQueryExtractionEnabled already returns true > > for mobile. > > ifdefs are not required here. Since I passed "false" in > toolbar_model_android.cc, I thought it would be better to add ifdefs here so > that I can revisit the code and update later. Removed. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... File chrome/browser/search/search.h (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/search/s... chrome/browser/search/search.h:79: // On mobile, |for_search| param is unused because the query extraction is On 2014/02/04 17:49:59, kmadhusu wrote: > On 2014/02/04 17:17:05, samarth wrote: > > This is confusing. Why isn't |for_search| set correctly on mobile? Looking at > > the callsites, I think toolbar_model_android.cc should be setting |for_search| > > to true. > > I am not sure about the callers of that function. That's why I passed false to > be on the safer side. Adding yusufo@ to review toolbar_model_android.cc changes. Fixed. https://codereview.chromium.org/151813002/diff/480001/chrome/browser/ui/andro... File chrome/browser/ui/android/toolbar/toolbar_model_android.cc (right): https://codereview.chromium.org/151813002/diff/480001/chrome/browser/ui/andro... chrome/browser/ui/android/toolbar/toolbar_model_android.cc:45: env, chrome::InstantExtendedEnabledParam(false)); On 2014/02/04 19:02:54, Yusuf wrote: > I do agree with samarth we should be returning true here. Since for mobile query > extraction is always enabled, we will never end up returning empty string > anyway. Done. As we discussed offline, I filed crbug.com/340766 and assigned to you.
https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... chrome/browser/search/search.cc:365: std::string InstantExtendedEnabledParam(bool for_search) { So SearchTermsData checked that EmbeddedSearchPageVersion() was non-zero. Shouldn't that check be here as well? (If yes, please also add a test to catch that case.) https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... File chrome/browser/search/search.h (right): https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... chrome/browser/search/search.h:77: // |for_search| is set to true for search requests and this |for_search| should be set to true for search requests, in which case this returns a non-empty string only if query extraction is enabled.
https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... chrome/browser/search/search.cc:365: std::string InstantExtendedEnabledParam(bool for_search) { On 2014/02/05 21:16:16, samarth wrote: > So SearchTermsData checked that EmbeddedSearchPageVersion() was non-zero. > Shouldn't that check be here as well? (If yes, please also add a test to catch > that case.) Good catch. Fixed. Added test. https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... File chrome/browser/search/search.h (right): https://codereview.chromium.org/151813002/diff/630001/chrome/browser/search/s... chrome/browser/search/search.h:77: // |for_search| is set to true for search requests and this On 2014/02/05 21:16:16, samarth wrote: > |for_search| should be set to true for search requests, in which case this > returns a non-empty string only if query extraction is enabled. Done.
lgtm But please wait to make sure Jered is OK with https://codereview.chromium.org/141893009/ before landing this. Thanks, Samarth https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... chrome/browser/search/search.cc:370: if (instant_extended_api_version) { Hmm I was going to say this should explicitly check version != kEmbeddedPageVersionDisabled but that no longer exists. So you don't need this check after all :)
CC'ing jered@. jered@: This is related to crrev.com/141893009. PTAL. Thanks. https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... chrome/browser/search/search.cc:370: if (instant_extended_api_version) { On 2014/02/07 00:25:31, samarth wrote: > Hmm I was going to say this should explicitly check version != > kEmbeddedPageVersionDisabled but that no longer exists. So you don't need this > check after all :) True. Current version of EmbeddedSearchPageVersion() doesn't return kEmbeddedPageVersionDisabled but it's better to have this check here to handle "espv:0" field trials flag.
Why is it good to move InstantExtendedEnabledParam() out of TemplateURL? It seems happy there, as it's surrounded by its friends who are doing the same kind of thing (computing search URL parameters)? https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... File chrome/browser/search_engines/search_terms_data.h (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... chrome/browser/search_engines/search_terms_data.h:79: // values, and NTPIsThemedParam() will return an empty string. Did you mean to leave out ForceInstantResultsParam()?
On 2014/02/07 17:48:11, Jered wrote: > Why is it good to move InstantExtendedEnabledParam() out of TemplateURL? Oops sorry meant SearchTermsData :-) > It > seems happy there, as it's surrounded by its friends who are doing the same kind > of thing (computing search URL parameters)? > > https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... > File chrome/browser/search_engines/search_terms_data.h (right): > > https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... > chrome/browser/search_engines/search_terms_data.h:79: // values, and > NTPIsThemedParam() will return an empty string. > Did you mean to leave out ForceInstantResultsParam()?
https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... chrome/browser/search/search.cc:370: if (instant_extended_api_version) { On 2014/02/07 00:44:05, kmadhusu wrote: > On 2014/02/07 00:25:31, samarth wrote: > > Hmm I was going to say this should explicitly check version != > > kEmbeddedPageVersionDisabled but that no longer exists. So you don't need this > > check after all :) > > True. Current version of EmbeddedSearchPageVersion() doesn't return > kEmbeddedPageVersionDisabled but it's better to have this check here to handle > "espv:0" field trials flag. espv:0 is explicitly not a supported config anymore.
On 2014/02/07 17:50:00, samarth wrote: > https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... > File chrome/browser/search/search.cc (right): > > https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... > chrome/browser/search/search.cc:370: if (instant_extended_api_version) { > On 2014/02/07 00:44:05, kmadhusu wrote: > > On 2014/02/07 00:25:31, samarth wrote: > > > Hmm I was going to say this should explicitly check version != > > > kEmbeddedPageVersionDisabled but that no longer exists. So you don't need > this > > > check after all :) > > > > True. Current version of EmbeddedSearchPageVersion() doesn't return > > kEmbeddedPageVersionDisabled but it's better to have this check here to handle > > "espv:0" field trials flag. > > espv:0 is explicitly not a supported config anymore. lgtm I am just hesitant to move more stuff to search.cc, but the argument that this won't be persisted is pretty convincing.
https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... File chrome/browser/search/search.cc (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search/s... chrome/browser/search/search.cc:370: if (instant_extended_api_version) { On 2014/02/07 17:50:00, samarth wrote: > On 2014/02/07 00:44:05, kmadhusu wrote: > > On 2014/02/07 00:25:31, samarth wrote: > > > Hmm I was going to say this should explicitly check version != > > > kEmbeddedPageVersionDisabled but that no longer exists. So you don't need > this > > > check after all :) > > > > True. Current version of EmbeddedSearchPageVersion() doesn't return > > kEmbeddedPageVersionDisabled but it's better to have this check here to handle > > "espv:0" field trials flag. > > espv:0 is explicitly not a supported config anymore. Removed the check. https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... File chrome/browser/search_engines/search_terms_data.h (right): https://codereview.chromium.org/151813002/diff/650001/chrome/browser/search_e... chrome/browser/search_engines/search_terms_data.h:79: // values, and NTPIsThemedParam() will return an empty string. On 2014/02/07 17:48:12, Jered wrote: > Did you mean to leave out ForceInstantResultsParam()? I removed it intentionally. ForceInstantResultsParam() doesn't access Profile. Therefore, I updated the comment. It can also be moved out of SearchTermsData. I will do that clean up in a separate CL.
The CQ bit was checked by kmadhusu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/151813002/940001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by kmadhusu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/151813002/940001
Message was sent while issue was closed.
Change committed as 251009 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
