Omnibox: In Reorder Mode, SearchProvider Doesn't Need to Remove Stale Results

Omnibox: In Reorder Mode, SearchProvider Doesn't Need to Remove Stale Results

BUG=309465
R=msw@chromium.org, pkasting@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234661

[Mark P, 2013-11-09 00:00:50]

Peter, please take a look at your leisure.

Mike, you are CCed as an FYI to give you a chance to object if you see something
I don't.

--mark

[Peter Kasting, 2013-11-09 00:06:19]

LGTM

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:879: // In theory it would be
better to run the removal algorithm like that in
Nit: I think changing "an" to "the removal" here actually made this less
grammatically correct and harder to understand.

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:902: }
Tiny nit: Maybe this would match the comments better:

  if (!omnibox_will_reorder_for_legal_default_match) {
    RemoveStaleResults(...);
    if (!keyword_input_.text().empty()) {
      RemoveStaleResults(...);
    }
  }
  if (keyword_input_.text().empty()) {
    ...
  }

[msw, 2013-11-11 18:26:24]

LGTM

[Mark P, 2013-11-11 18:46:14]

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:879: // In theory it would be
better to run the removal algorithm like that in
On 2013/11/09 00:06:20, Peter Kasting wrote:
> Nit: I think changing "an" to "the removal" here actually made this less
> grammatically correct and harder to understand.

I thought "an algorithm" was vague (which algorithm?), so I changed it
originally.
Changed it back per your request.

https://codereview.chromium.org/66503003/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:902: }
On 2013/11/09 00:06:20, Peter Kasting wrote:
> Tiny nit: Maybe this would match the comments better:
> 
>   if (!omnibox_will_reorder_for_legal_default_match) {
>     RemoveStaleResults(...);
>     if (!keyword_input_.text().empty()) {
>       RemoveStaleResults(...);
>     }
>   }
>   if (keyword_input_.text().empty()) {
>     ...
>   }

Sure, if you want.  I considered both options and couldn't decide which I liked
better.  I ended up going with this one because it only tested
keyword_input_.text().empty() once and I thought maybe that meant it was better
than one that tested it twice.  I'm switching to your suggestion; it's easier to
read (even though for some reason I picked the other one originally).

[commit-bot: I haz the power, 2013-11-11 18:48:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/66503003/70001

[commit-bot: I haz the power, 2013-11-11 19:20:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2013-11-11 19:23:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/66503003/70001

[commit-bot: I haz the power, 2013-11-11 19:44:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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.

[commit-bot: I haz the power, 2013-11-11 19:57:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/66503003/70001

[commit-bot: I haz the power, 2013-11-11 20:37:56]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
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.

[commit-bot: I haz the power, 2013-11-11 20:50:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/66503003/70001

[Mark P, 2013-11-12 23:23:50]

Committed patchset #3 manually as r234661 (presubmit successful).