Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
462963002:
[AiS] Reuse previous answer for verbatim. (Closed)
|
Created:
6 years, 4 months ago by groby-ooo-7-16 Modified:
6 years, 4 months ago CC:
chromium-reviews, James Su, Justin Donnelly Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[AiS] Reuse previous answer for verbatim.
Verbatim queries do not see an answer. If the previous query had an
answer with a suggestion that matches the current verbatim text, reuse
that answer.
BUG=401486
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289959
Patch Set 1 #
Total comments: 10
Messages
Total messages: 18 (0 generated)
PTAL - here's the fix we discussed re: verbatim matches & answers Also, hints as to how to make this testable are appreciated
Ping - would you mind giving this a look? (I've seen your calendar and am sorry :)
https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't get suggestions and hence, answers. This feels like the wrong place to do this. By doing this so late in the game, if you add logic mostly anywhere else in this class that depends on whether a suggestion has an answer, it'd be wrong. I suggest adding the answer information as soon as possible after the new suggest reply is received. I have an change that does some other processing immediately after a reply is received; you can probably use the same structure as soon as that goes in. I'll point you to the changelist when I upload it.
https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't get suggestions and hence, answers. On 2014/08/12 20:57:29, Mark P wrote: > This feels like the wrong place to do this. By doing this so late in the game, > if you add logic mostly anywhere else in this class that depends on whether a > suggestion has an answer, it'd be wrong. > > I suggest adding the answer information as soon as possible after the new > suggest reply is received. I have an change that does some other processing > immediately after a reply is received; you can probably use the same structure > as soon as that goes in. > > I'll point you to the changelist when I upload it. But I can't add it anywhere else - the answer needs to be attached to the verbatim result, and that's only generated here. In general, ConvertResultsToAutocompleteMatches() is where the old result gets replaced with the new result - so that seems the right place. Can you point me to your other change so I can get a better idea?
On 2014/08/12 22:09:21, groby wrote: > https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... > File chrome/browser/autocomplete/search_provider.cc (right): > > https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... > chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't > get suggestions and hence, answers. > On 2014/08/12 20:57:29, Mark P wrote: > > This feels like the wrong place to do this. By doing this so late in the > game, > > if you add logic mostly anywhere else in this class that depends on whether a > > suggestion has an answer, it'd be wrong. > > > > I suggest adding the answer information as soon as possible after the new > > suggest reply is received. I have an change that does some other processing > > immediately after a reply is received; you can probably use the same structure > > as soon as that goes in. > > > > I'll point you to the changelist when I upload it. > > But I can't add it anywhere else - the answer needs to be attached to the > verbatim result, and that's only generated here. Good point. > In general, ConvertResultsToAutocompleteMatches() is where the old result gets > replaced with the new result - so that seems the right place. > > Can you point me to your other change so I can get a better idea?
Adding msw@ for his opinion. https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't get suggestions and hence, answers. On 2014/08/12 22:09:20, groby wrote: > On 2014/08/12 20:57:29, Mark P wrote: > > This feels like the wrong place to do this. By doing this so late in the > game, > > if you add logic mostly anywhere else in this class that depends on whether a > > suggestion has an answer, it'd be wrong. > > > > I suggest adding the answer information as soon as possible after the new > > suggest reply is received. I have an change that does some other processing > > immediately after a reply is received; you can probably use the same structure > > as soon as that goes in. > > > > I'll point you to the changelist when I upload it. > > But I can't add it anywhere else - the answer needs to be attached to the > verbatim result, and that's only generated here. Good point. > In general, ConvertResultsToAutocompleteMatches() is where the old result gets > replaced with the new result - so that seems the right place. > > Can you point me to your other change so I can get a better idea? >
Sorry if my questions are dumb, I've been out of the SearchProvider loop for a while (pre-answers, for sure). https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't get suggestions and hence, answers. Why don't verbatim results get suggestions? Isn't that up to the provider? If I paste in [weather los angeles], does that not get an answer either? https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:738: // Scan previous matches if the last answer-bearing suggestion matches Why not just check |last_answer_seen_| instead of scanning?
https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:737: // Verbatim results don't get suggestions and hence, answers. On 2014/08/13 19:01:04, msw wrote: > Why don't verbatim results get suggestions? Isn't that up to the provider? > If I paste in [weather los angeles], does that not get an answer either? I'll answer on behalf of groby@: the server does not return the verbatim match as a suggestion, which means we don't get an answer for it. The other question you asked is interesting.
https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:738: // Scan previous matches if the last answer-bearing suggestion matches On 2014/08/13 19:01:04, msw wrote: > Why not just check |last_answer_seen_| instead of scanning? Because 1) last_answer_seen_ is only the query text plus answer type, not the result 2) last_answer_seen_ going away for a cache of answers seen.
lgtm baring one comment below Thanks Mike for your eyes. I appreciated it. --mark https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:738: // Scan previous matches if the last answer-bearing suggestion matches On 2014/08/13 20:07:17, groby wrote: > On 2014/08/13 19:01:04, msw wrote: > > Why not just check |last_answer_seen_| instead of scanning? > > Because > 1) last_answer_seen_ is only the query text plus answer type, not the result > 2) last_answer_seen_ going away for a cache of answers seen. If this is happening, can you add to the bug that's tracking the task (create a cache of answers seen) [create a bug if you don't have one already] a note that once you have the cache you should revise this code to use the cache and skip the scan.
Traveling right now, so I'll get to update this tomorrow, I hope. Thank you for the feedback, Mike & Mark! https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:738: // Scan previous matches if the last answer-bearing suggestion matches On 2014/08/13 20:12:27, Mark P wrote: > On 2014/08/13 20:07:17, groby wrote: > > On 2014/08/13 19:01:04, msw wrote: > > > Why not just check |last_answer_seen_| instead of scanning? > > > > Because > > 1) last_answer_seen_ is only the query text plus answer type, not the result > > 2) last_answer_seen_ going away for a cache of answers seen. > > If this is happening, can you add to the bug that's tracking the task (create a > cache of answers seen) > [create a bug if you don't have one already] > a note that once you have the cache you should revise this code to use the cache > and skip the scan. The cache still can't hold the answers content. The data is too volatile to keep around for longer than a query. (I.e if I searched "goog stock" 30 minutes ago, and re-search it again, I probably don't want to have a stock quote from 30 minutes ago) Which means I need to track a timeout here, too. Let me add that :)
https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/462963002/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/search_provider.cc:738: // Scan previous matches if the last answer-bearing suggestion matches On 2014/08/13 23:39:21, groby wrote: > On 2014/08/13 20:12:27, Mark P wrote: > > On 2014/08/13 20:07:17, groby wrote: > > > On 2014/08/13 19:01:04, msw wrote: > > > > Why not just check |last_answer_seen_| instead of scanning? > > > > > > Because > > > 1) last_answer_seen_ is only the query text plus answer type, not the result > > > 2) last_answer_seen_ going away for a cache of answers seen. > > > > If this is happening, can you add to the bug that's tracking the task (create > a > > cache of answers seen) > > [create a bug if you don't have one already] > > a note that once you have the cache you should revise this code to use the > cache > > and skip the scan. > > The cache still can't hold the answers content. The data is too volatile to keep > around for longer than a query. (I.e if I searched "goog stock" 30 minutes ago, > and re-search it again, I probably don't want to have a stock quote from 30 > minutes ago) > > Which means I need to track a timeout here, too. Let me add that :) When the user ceases to edit something in the omnibox (press escape, etc.), SearchProvider clears all its stored information. I'd argue that you should _not_ put a timeout on top of that. The amount of time the user spends editing is fairly small, and if you think you want a timeout while editing is in progress, I disagree--let's talk about it before you do it.
Ah - I was not aware of that part. In this case, I'll submit as-is.
Oh, the cache bug is 401758. But, again - that cache only carries the query completion, not the answers content. We need the content to make this work. Let me know if that means you'd like me to take a different approach.
this change still lgtm As for caching, I didn't review that code, so I'll leave it up the actual reviewer to decide if the policy of not storing answers content there makes sense. --mark
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/462963002/1
Message was sent while issue was closed.
Committed patchset #1 (1) as 289959 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
