Only use verbatimrelevance with other suggestions available. (issue 10559053)

[m...@chromium.org]

Reviewers: Peter Kasting,

Message:
Hey Peter, please take a look; thanks!

Description:
Only use verbatimrelevance with other suggestions available.

BUG=125871,132942
TEST=Updated unit test; manual.


Please review this at https://chromiumcodereview.appspot.com/10559053/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M chrome/browser/autocomplete/search_provider.cc
M chrome/browser/autocomplete/search_provider_unittest.cc


Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc
b/chrome/browser/autocomplete/search_provider.cc
index
9d1281b3cc74ca5850ce6cc2c06643119c647690..16dbce440abc007f2407ed3e69a82b7c35f72781
100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -930,8 +930,14 @@ void SearchProvider::AddSuggestResultsToMap(const
SuggestResults& results,
}

int SearchProvider::GetVerbatimRelevance() const {
- if (verbatim_relevance_ >= 0 && !input_.prevent_inline_autocomplete())
+ // Use the server suggested verbatim relevance score if it is
non-negative,
+ // inline autocomplete isn't prevented (happens on backspace, etc.), and
+ // there is at least one other suggestion from the default provider.
+ if (verbatim_relevance_ >= 0 && !input_.prevent_inline_autocomplete() &&
+ (!default_suggest_results_.empty() ||
+ !default_navigation_results_.empty())) {
return verbatim_relevance_;
+ }
return CalculateRelevanceForVerbatim();
}

Index: chrome/browser/autocomplete/search_provider_unittest.cc
diff --git a/chrome/browser/autocomplete/search_provider_unittest.cc
b/chrome/browser/autocomplete/search_provider_unittest.cc
index
227c163a084100aaad205ad369bd235ed3e48fde..00d96a55c95229fe82420e5cc71491de67eaf0fe
100644
--- a/chrome/browser/autocomplete/search_provider_unittest.cc
+++ b/chrome/browser/autocomplete/search_provider_unittest.cc
@@ -854,6 +854,12 @@ TEST_F(SearchProviderTest, SuggestRelevanceExperiment)
{
"{\"google:suggestrelevance\":[9998, 9997, 9999],"
"\"google:verbatimrelevance\":0}]",
{ "a2", "a", "a1", kNotApplicable } },
+
+ // Ensure that verbatim is always generated without other suggestions.
+ { "[\"a\",[],[],[],{\"google:verbatimrelevance\":1}]",
+ { "a", kNotApplicable, kNotApplicable, kNotApplicable } },
+ { "[\"a\",[],[],[],{\"google:verbatimrelevance\":0}]",
+ { "a", kNotApplicable, kNotApplicable, kNotApplicable } },
};

for (size_t i = 0; i < ARRAYSIZE_UNSAFE(cases); i++) {

[pkas...@chromium.org]

Rubber-stamp LGTM


https://chromiumcodereview.appspot.com/10559053/diff/1/chrome/browser/autocomplete/search_provider.cc
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10559053/diff/1/chrome/browser/autocomplete/search_provider.cc#newcode935
chrome/browser/autocomplete/search_provider.cc:935: // there is at least

one other suggestion from the default provider.

Nit: As before, I suggest giving some explicit scenarios that this
prevents as examples to help readers understand why this is important.

https://chromiumcodereview.appspot.com/10559053/

[m...@chromium.org]

https://chromiumcodereview.appspot.com/10559053/diff/1/chrome/browser/autocomplete/search_provider.cc
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/10559053/diff/1/chrome/browser/autocomplete/search_provider.cc#newcode935
chrome/browser/autocomplete/search_provider.cc:935: // there is at least
one other suggestion from the default provider.

On 2012/06/19 01:10:57, Peter Kasting wrote:
> Nit: As before, I suggest giving some explicit scenarios that this
prevents as
> examples to help readers understand why this is important.

Done.

https://chromiumcodereview.appspot.com/10559053/

[commi...@chromium.org]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/m...@chromium.org/10559053/3002


https://chromiumcodereview.appspot.com/10559053/

[commi...@chromium.org]

Try job failure for 10559053-3002 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=33483


https://chromiumcodereview.appspot.com/10559053/

[commi...@chromium.org]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/m...@chromium.org/10559053/13001


https://chromiumcodereview.appspot.com/10559053/

[commi...@chromium.org]

Try job failure for 10559053-13001 (retry) on win_rel for step
"chrome_frame_net_tests".
It's a second try, previously, steps "browser_tests, chrome_frame_net_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=38595


https://chromiumcodereview.appspot.com/10559053/

[m...@chromium.org]

FLAKY_ProxyTunnelRedirectTest chrome_frame_net_tests failing for this job
and:
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/38607
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/38606
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/38605
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/38604

Landing manually.

https://chromiumcodereview.appspot.com/10559053/