Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12930011:
History: Include search term in queries to history server. (Closed)
|
Created:
7 years, 9 months ago by Patrick Dubroy Modified:
7 years, 9 months ago CC:
chromium-reviews, browser-components-watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHistory: Include search term in queries to history server.
History server now supports searches using q=your+query syntax. When
searching history, combine local results with results from the server.
Fixed bug where server results would be ignored if there were no local
results.
TBR=brettw@chromium.org
BUG=180970
TEST=Open chrome://history in a signed-in profile, and type a search
into the search box. Results should include vists from other devices.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190503
Patch Set 1 : #
Total comments: 4
Patch Set 2 : Address joao's comments. #
Total comments: 6
Patch Set 3 : Address dbeam's comments. #Patch Set 4 : Remove unnecessary check #
Total comments: 4
Patch Set 5 : Saved by dbeam. #
Total comments: 1
Messages
Total messages: 18 (0 generated)
Drew, please take a look.
+joao, since atwilson is OOO
lgtm. Please see comments inline https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:825: int local_result_count = query_results_.size(); |local_result_count| used to be calculated after removing the older entries. I'm not familiar with this code, do you really mean to calculate it before removing older entries now? https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:829: // set the are older than that, and then combine the results. "from either set the are older than that" -> "from either set _that_ are older than that"
https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:825: int local_result_count = query_results_.size(); On 2013/03/25 13:13:59, Joao da Silva wrote: > |local_result_count| used to be calculated after removing the older entries. I'm > not familiar with this code, do you really mean to calculate it before removing > older entries now? Good catch, fixed. https://codereview.chromium.org/12930011/diff/2001/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:829: // set the are older than that, and then combine the results. On 2013/03/25 13:13:59, Joao da Silva wrote: > "from either set the are older than that" -> "from either set _that_ are older > than that" Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dubroy@chromium.org/12930011/8002
Presubmit check for 12930011-8002 failed and returned exit status 1.
INFO:root:Found 2 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/webui/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/browser/ui/webui/history_ui.cc
chrome/browser/history/web_history_service.cc
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dubroy@chromium.org/12930011/8002
+estade for OWNERS rubber stamp.
+dbeam for owners review
https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:824: if (web_history_query_results_.size()) { size() -> !empty() https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:839: if (query_results_.size()) { ^ wont this always be non-empty now?
https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:824: if (web_history_query_results_.size()) { On 2013/03/25 21:09:19, Dan Beam wrote: > size() -> !empty() Done. https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:839: if (query_results_.size()) { On 2013/03/25 21:09:19, Dan Beam wrote: > ^ wont this always be non-empty now? Nope, RemoveOlderEntries might actually make it empty.
https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:839: if (query_results_.size()) { On 2013/03/25 21:18:49, dubroy wrote: > On 2013/03/25 21:09:19, Dan Beam wrote: > > ^ wont this always be non-empty now? > > Nope, RemoveOlderEntries might actually make it empty. but there's a "querty_results_.insert()" call right above this
https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/8002/chrome/browser/ui/webui/hi... chrome/browser/ui/webui/history_ui.cc:839: if (query_results_.size()) { On 2013/03/25 21:27:02, Dan Beam wrote: > On 2013/03/25 21:18:49, dubroy wrote: > > On 2013/03/25 21:09:19, Dan Beam wrote: > > > ^ wont this always be non-empty now? > > > > Nope, RemoveOlderEntries might actually make it empty. > > but there's a "querty_results_.insert()" call right above this Yeah, you're right. Fixed.
https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (left): https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:833: int local_result_count = query_results_.size(); ^ it seems like this is the right logic, right? don't log this if there were no results before we add a synthetic one?
https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:834: } nit: \n after conditionals (from C++ style guide)
https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (left): https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:833: int local_result_count = query_results_.size(); On 2013/03/25 21:38:58, Dan Beam wrote: > ^ it seems like this is the right logic, right? don't log this if there were no > results before we add a synthetic one? Damn, you're right. Fixed. https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... File chrome/browser/ui/webui/history_ui.cc (right): https://codereview.chromium.org/12930011/diff/21002/chrome/browser/ui/webui/h... chrome/browser/ui/webui/history_ui.cc:834: } On 2013/03/25 21:40:44, Dan Beam wrote: > nit: \n after conditionals (from C++ style guide) Done.
lgtm https://codereview.chromium.org/12930011/diff/26001/chrome/browser/history/we... File chrome/browser/history/web_history_service.cc (right): https://codereview.chromium.org/12930011/diff/26001/chrome/browser/history/we... chrome/browser/history/web_history_service.cc:249: std::string url = GetQueryUrl(text_query, options); nit: this could probably be const[-ref]
Message was sent while issue was closed.
Committed patchset #5 manually as r190503 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
