Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(295)

Issue 10860068: Fix Omnibox search provider's confusing internal variable semantics (Closed)

Created:
8 years, 4 months ago by Mark P
Modified:
8 years, 3 months ago
Reviewers:
msw, Peter Kasting
CC:
chromium-reviews, James Su
Visibility:
Public.

Description

Fix Omnibox search provider's confusing internal variable semantics As a side effect, the count in the histogram Omnibox.SuggestRequests's number of requests invalidated bucket will be more accurate. BUG= TEST=by hand using about:histograms Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153416

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 4

Patch Set 3 : More minor revisions. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -10 lines) Patch
M chrome/browser/autocomplete/search_provider.h View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/autocomplete/search_provider.cc View 1 2 2 chunks +3 lines, -9 lines 1 comment Download

Messages

Total messages: 13 (0 generated)
Mark P
8 years, 4 months ago (2012-08-21 16:51:12 UTC) #1
msw
Perhaps add a private util SuggestResultsPending() or similar that checks both? https://chromiumcodereview.appspot.com/10860068/diff/1/chrome/browser/autocomplete/search_provider.h File chrome/browser/autocomplete/search_provider.h (right): ...
8 years, 4 months ago (2012-08-21 17:12:28 UTC) #2
Mark P
Please take another look. > Perhaps add a private util SuggestResultsPending() > or similar that ...
8 years, 4 months ago (2012-08-21 17:47:51 UTC) #3
msw
https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/autocomplete/search_provider.cc File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/autocomplete/search_provider.cc#newcode1239 chrome/browser/autocomplete/search_provider.cc:1239: // We're done when the timer isn't running and ...
8 years, 4 months ago (2012-08-21 18:30:27 UTC) #4
Mark P
https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/autocomplete/search_provider.cc File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10860068/diff/5001/chrome/browser/autocomplete/search_provider.cc#newcode1239 chrome/browser/autocomplete/search_provider.cc:1239: // We're done when the timer isn't running and ...
8 years, 4 months ago (2012-08-21 19:13:41 UTC) #5
msw
LGTM; thanks for the cleanup! https://chromiumcodereview.appspot.com/10860068/diff/10001/chrome/browser/autocomplete/search_provider.cc File chrome/browser/autocomplete/search_provider.cc (right): https://chromiumcodereview.appspot.com/10860068/diff/10001/chrome/browser/autocomplete/search_provider.cc#newcode573 chrome/browser/autocomplete/search_provider.cc:573: LogOmniboxSuggestRequest(REQUEST_INVALIDATED); FYI, we may ...
8 years, 4 months ago (2012-08-21 19:16:18 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/10860068/10001
8 years, 4 months ago (2012-08-21 19:19:51 UTC) #7
commit-bot: I haz the power
Presubmit check for 10860068-10001 failed and returned exit status 1. Running presubmit commit checks ...
8 years, 4 months ago (2012-08-21 19:19:54 UTC) #8
Mark P
Peter, Can I get your OWNERS approval on this? thanks, mark
8 years, 4 months ago (2012-08-21 19:36:55 UTC) #9
Peter Kasting
LGTM, but your change description is slightly inaccurate now.
8 years, 4 months ago (2012-08-25 17:45:51 UTC) #10
Mark P
> LGTM, but your change description is > slightly inaccurate now. Revised description. Submitting.
8 years, 4 months ago (2012-08-26 01:24:56 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/10860068/10001
8 years, 4 months ago (2012-08-26 01:25:09 UTC) #12
commit-bot: I haz the power
8 years, 3 months ago (2012-08-26 12:21:37 UTC) #13
Change committed as 153416

Powered by Google App Engine
This is Rietveld 408576698