Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
471673002:
Omnibox: Prevent Asynchronous Suggestions from Changing Default Match (Closed)
|
Created:
6 years, 4 months ago by Mark P Modified:
6 years, 3 months ago Reviewers:
msw CC:
chromium-reviews, James Su, Peter Kasting, Maria Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionOmnibox: Prevent Asynchronous Suggestions from Changing Default Match
Calls to the suggest server may normally result in a new inline
autocompletion. This can be disruptive because it means pressing enter
may bring the user to different places depending on how long he/she
waits after typing the last key.
This change prevents new suggestions from becoming the default match.
In other words, the default match is only allowed to change on a
keystroke, not due to a reply coming back from the server.
The consequence of this change is that if previously we'd show an
inline suggestion on a server reply, now we only show it one keystroke
later. I think this trade-off (one keystroke versus inconsistent
omnibox behavior) is a good one to make.
We still end up with default matches (inline autocompletions within
the omnibox) from the suggest server after this change. Here's an
example of why:
User types "facebo"
We send a suggest server request.
Server asynchronously returns "facebook" as a top suggestion,
beating the server-provided verbatim score for "facebo".
We decide not to show it within the omnibox. It's instead shown
somewhere in the dropdown.
User types "o".
We send a suggest server request.
We reuse our cached suggestions and suggestion scores. <<< THE KEY
We show "facebook" as an inline suggestion because it beats
the default verbatim score that gets assigned to "faceboo".
(This is the score that we assign by default without having
yet received the most recent suggest server response.)
We receive the response, which includes "facebook" as a top
suggestion, beating the server-provided verbatim score
for "faceboo".
We show "facebook" as an inline suggestion. i.e., we decide
not to demote it because it was already being shown inline
TESTED:
unit tests plus interactive tests (facebook.com/l, google.com/a)
BUG=398135
R=msw@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290058
Patch Set 1 #Patch Set 2 : more cleanup #Patch Set 3 : more cleanup #Patch Set 4 : more cleanup #Patch Set 5 : add clarifying comment #
Total comments: 16
Patch Set 6 : Mike's suggestions #Patch Set 7 : last remaining test should pass after refactor changelist is in #Patch Set 8 : rebase #Patch Set 9 : better resolve rebase #
Total comments: 54
Patch Set 10 : rebase on top of head, including necessary refactoring changelist #Patch Set 11 : resolve rebase issue #Patch Set 12 : more comment cleanup after rebasing #
Total comments: 5
Patch Set 13 : Mike's pile of comments #Patch Set 14 : added comment by PersistInlineSuggestions() #
Total comments: 15
Patch Set 15 : retore dropped comment #
Total comments: 22
Patch Set 16 : Mike's comments #
Messages
Total messages: 25 (0 generated)
Mike, You and Peter are the people most familiar with the "allowed to be default match" code, and you especially with SearchProvider constraints. Peter's on vacation. Can you please review this? I'm hoping to get it into M-38. It will give more confident that I can continue the rollout of the change that makes inlining query suggestions from the server more common. Maria - you are CCed at the most who refactored SearchProvider and suggested the HandleReceivedResult(), so I thought you might want to see what I'm doing. thanks all, mark
"the default match is only allowed to change on a keystroke, not due to a reply coming back from the server" Did you discuss this with Peter? It sounds like a big change to land right before the branch... There may be weird consequences, mightn't some people type 'f' wait for 'foo.com' inline, and then hit enter? I'm not so sure I'll do this review justice by branch, but it's 2nd in my queue.
On 2014/08/13 23:19:01, msw wrote: > "the default match is only allowed to change on a keystroke, not due to a reply > coming back from the server" > Did you discuss this with Peter? This was his idea. Please read the bug thread for background. > It sounds like a big change to land right > before the branch... > There may be weird consequences, mightn't some people type 'f' wait for > 'foo.com' inline, and then hit enter? Maaaaybe. I'm skeptical that someone will expected a string such as "f" to have an inline autocompletion if they've never typed it before. (On what basis would they guess that the suggest server will do that?) And if they've typed it before, then we'll be able to inline it (synchronously) from history. --mark > I'm not so sure I'll do this review justice by branch, but it's 2nd in my queue.
Before/while I look closer at the SearchProvider::HandleReceivedResults impl and the test changes, I have some preliminary feedback: On 2014/08/14 00:59:45, Mark P wrote: > On 2014/08/13 23:19:01, msw wrote: > > There may be weird consequences, mightn't some people type 'f' wait for > > 'foo.com' inline, and then hit enter? > > Maaaaybe. I'm skeptical that someone will expected a string such as "f" to > have an inline autocompletion if they've never typed it before. (On what > basis would they guess that the suggest server will do that?) And if they've > typed it before, then we'll be able to inline it (synchronously) from history. That's fair... history, bookmark, etc. suggestions are more important here. Still, search providers *must* now 'predict' keystrokes to ever get inlined. It's arguably okay, since we still show 'live' suggestions in the dropdown. Again, this is just a bit drastic to land at 100% right before a branch. If the related "variable verbatim" work metrics are good, why do this now? Why not impose this restriction after x ms from the last keystroke? Couldn't search provider results arrive without noticeable delay? (I'll read through the bug to see why but wanted to respond asap) https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:249: match.allowed_to_be_default_match = Should this also consider never_allowed_to_be_default_match()? https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:401: HandleReceivedResults(is_keyword); Is HandleReceivedResults really needed if we call UpdateMatches()? https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:223: // function provides the opportunity to do post-processing on the received nit: s/provides the opportunity to do/allows/ https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:225: virtual void HandleReceivedResults(bool is_keyword); optional nit: OnParseSuggestResults, OnSuggestResultsParsed https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:335: // Results from the default and keyword search providers. The orig results nit: s/orig/original/ in comments and identifiers. https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:343: SearchSuggestionParser::Results orig_default_results_; Why do we need a copy of the originals? Can't we modify a single set as new responses are received and when keys are pressed? Explain in the comment. https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... File components/autocomplete/search_suggestion_parser.h (left): https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... components/autocomplete/search_suggestion_parser.h:259: DISALLOW_COPY_AND_ASSIGN(Results); Why are you removing this? To allow the assignment of |default_results_ = orig_default_results_;|, etc.? I think that's unnecessary in my suggested feature implementation. This deserved an explicit CL description comment, or at least a CL review comment explanation... Copying is bad, m'kay? :) https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... File components/autocomplete/search_suggestion_parser.h (right): https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... components/autocomplete/search_suggestion_parser.h:100: // If true, this result should never be allowed to be the default match. Hmm, seems like this identifier and the relevant code could be a bit clearer if this were |received_since_last_keystroke_|, always set true on receipt, and set to false for those results which survive the next keystroke's SortAndCull (or whatever the corresponding result/match cleanup for re-use is called nowadays).
On Wed, Aug 13, 2014 at 7:57 PM, <msw@chromium.org> wrote: > Before/while I look closer at the SearchProvider::HandleReceivedResults > impl and > the test changes, I have some preliminary feedback: > > > On 2014/08/14 00:59:45, Mark P wrote: > >> On 2014/08/13 23:19:01, msw wrote: >> > There may be weird consequences, mightn't some people type 'f' wait for >> > 'foo.com' inline, and then hit enter? >> > > Maaaaybe. I'm skeptical that someone will expected a string such as "f" >> to >> have an inline autocompletion if they've never typed it before. (On what >> basis would they guess that the suggest server will do that?) And if >> they've >> typed it before, then we'll be able to inline it (synchronously) from >> history. >> > > That's fair... history, bookmark, etc. suggestions are more important here. > Still, search providers *must* now 'predict' keystrokes to ever get > inlined. > It's arguably okay, since we still show 'live' suggestions in the dropdown. > Search provider (like other providers) tries to predict keystrokes. If the prediction is not good, the result shouldn't be inlined in any case (neither instantly nor on the next keystroke). > > Again, this is just a bit drastic to land at 100% right before a branch. > If the related "variable verbatim" work metrics are good, why do this now? > Variable verbatim metrics do look good. We were in the process of launching it--it hit 10% of stable if I recall correctly--before Peter made a fuss. His points were valid, including the one that one shouldn't have to worry about how fast one is pressing enter. Because I agree with this attitude, I think it's a tad silly to rollout verbatim verbatim to more people, some of whom might begin to subtly distrust the omnibox's, when I know I'm going to submit a fix (this one) to make the omnibox feel more consistent, less flaky. That's why I'm paused the rollout pending this change. > Why not impose this restriction after x ms from the last keystroke? > Couldn't search provider results arrive without noticeable delay? > (I'll read through the bug to see why but wanted to respond asap) > See bug. As for your comments below about possibly not needing to copy Results variables around, I think you're right. I think this is a remnant of a previous way I implemented the change (which involved revising relevance scores, an action which is harder to roll back) and when I switch to this default-match implementation, I didn't rethink the necessity of copying-and-modifying. I'll seem about revising to remove this aspect of the implementation. --mark > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.cc > File chrome/browser/autocomplete/base_search_provider.cc (right): > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.cc#newcode249 > chrome/browser/autocomplete/base_search_provider.cc:249: > match.allowed_to_be_default_match = > Should this also consider never_allowed_to_be_default_match()? > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.cc#newcode401 > chrome/browser/autocomplete/base_search_provider.cc:401: > HandleReceivedResults(is_keyword); > Is HandleReceivedResults really needed if we call UpdateMatches()? > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.h > File chrome/browser/autocomplete/base_search_provider.h (right): > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.h#newcode223 > chrome/browser/autocomplete/base_search_provider.h:223: // function > provides the opportunity to do post-processing on the received > nit: s/provides the opportunity to do/allows/ > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/base_search_provider.h#newcode225 > chrome/browser/autocomplete/base_search_provider.h:225: virtual void > HandleReceivedResults(bool is_keyword); > optional nit: OnParseSuggestResults, OnSuggestResultsParsed > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/search_provider.h > File chrome/browser/autocomplete/search_provider.h (right): > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/search_provider.h#newcode335 > chrome/browser/autocomplete/search_provider.h:335: // Results from the > default and keyword search providers. The orig results > nit: s/orig/original/ in comments and identifiers. > > https://codereview.chromium.org/471673002/diff/80001/ > chrome/browser/autocomplete/search_provider.h#newcode343 > chrome/browser/autocomplete/search_provider.h:343: > SearchSuggestionParser::Results orig_default_results_; > Why do we need a copy of the originals? Can't we modify a single set as > new responses are received and when keys are pressed? Explain in the > comment. > > https://codereview.chromium.org/471673002/diff/80001/ > components/autocomplete/search_suggestion_parser.h > File components/autocomplete/search_suggestion_parser.h (left): > > https://codereview.chromium.org/471673002/diff/80001/ > components/autocomplete/search_suggestion_parser.h#oldcode259 > components/autocomplete/search_suggestion_parser.h:259: > DISALLOW_COPY_AND_ASSIGN(Results); > Why are you removing this? To allow the assignment of |default_results_ > = orig_default_results_;|, etc.? I think that's unnecessary in my > suggested feature implementation. This deserved an explicit CL > description comment, or at least a CL review comment explanation... > Copying is bad, m'kay? :) > > https://codereview.chromium.org/471673002/diff/80001/ > components/autocomplete/search_suggestion_parser.h > File components/autocomplete/search_suggestion_parser.h (right): > > https://codereview.chromium.org/471673002/diff/80001/ > components/autocomplete/search_suggestion_parser.h#newcode100 > components/autocomplete/search_suggestion_parser.h:100: // If true, this > result should never be allowed to be the default match. > Hmm, seems like this identifier and the relevant code could be a bit > clearer if this were |received_since_last_keystroke_|, always set true > on receipt, and set to false for those results which survive the next > keystroke's SortAndCull (or whatever the corresponding result/match > cleanup for re-use is called nowadays). > > https://codereview.chromium.org/471673002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I heavily restructured this according to your suggestions. Per your suggestion, I managed to mostly remove the "orig" fields so we don't have two copies of the Results. Instead, I simply keep track of last inline autocompletion (the query suggestion or navsuggestion). That's much more lightweight. But it doesn't work, at least how I implemented it. See below. You can take another look now if you want. *However*, a few tests now fail. :-( I see that the problem arises in the following situation. Previous situation: 1. User types "a" 2. Suggest server says "suppress verbatim" (verbatim_relevance=0). Returns a query suggestion "abc" with a high score. 3. User sees "abc" inlined. No mention of the verbatim "a". 4. User types "b". 5. User sees "abc" inlined from cached results. Now, what we want it is: 1. User types "a" 2. Suggest server says "suppress verbatim" (verbatim_relevance=0). Returns a query suggestion "abc" with a high score. 2.5. Client says, screw you, we're not going to inline asynchronously. 3. User sees the verbatim query in the omnibox and "abc" as a suggestion in the dropdown. 4. User types "b". 5. User sees "abc" inlined from cached results. However, with the new approach (no old and new copy of the results, and all we know is what was previously inline autocompleted), we see: 1. User types "a" 2. Suggest server says "suppress verbatim" (verbatim_relevance=0). Returns a query suggestion "abc" with a high score. 2.5. Client says, screw you, we're not going to let you inline asynchronously. 2.6. Client notices it now has no results that allowed to be the default match. 2.7. Client reverts to non-server-provided scores. 3. User sees the verbatim query in the omnibox and "abc" as a suggestion in the dropdown. 4. User types "b". 5. User sees "ab" in the omnibox; "abc" is only a suggestion in the dropdown because it has a lesser client-controlled score. I feel like there's a way to fix this, though don't know how off the top of my head and I'm afraid I'm heading home now. I'm unlikely to be able to get a (correct) revised version for you in time for you to review it on Friday. :-( We'll see. --mark https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:249: match.allowed_to_be_default_match = On 2014/08/14 02:57:22, msw wrote: > Should this also consider never_allowed_to_be_default_match()? No. Something equivalent to verbatim is always allowed to be the default match. It shouldn't depend on whether it was received since the last keystroke or not (that doesn't even make sense for verbatim). https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.cc:401: HandleReceivedResults(is_keyword); On 2014/08/14 02:57:22, msw wrote: > Is HandleReceivedResults really needed if we call UpdateMatches()? Huh. I guess not. https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/base_search_provider.h (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:223: // function provides the opportunity to do post-processing on the received On 2014/08/14 02:57:22, msw wrote: > nit: s/provides the opportunity to do/allows/ Removed this function. Now obsolete. https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/base_search_provider.h:225: virtual void HandleReceivedResults(bool is_keyword); On 2014/08/14 02:57:22, msw wrote: > optional nit: OnParseSuggestResults, OnSuggestResultsParsed Now obsolete. https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:335: // Results from the default and keyword search providers. The orig results On 2014/08/14 02:57:23, msw wrote: > nit: s/orig/original/ in comments and identifiers. Now obsolete. (Got rid of the orig members.) https://codereview.chromium.org/471673002/diff/80001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:343: SearchSuggestionParser::Results orig_default_results_; On 2014/08/14 02:57:23, msw wrote: > Why do we need a copy of the originals? Can't we modify a single set as new > responses are received and when keys are pressed? Explain in the comment. Removed copy of the originals. Good idea. https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... File components/autocomplete/search_suggestion_parser.h (left): https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... components/autocomplete/search_suggestion_parser.h:259: DISALLOW_COPY_AND_ASSIGN(Results); On 2014/08/14 02:57:23, msw wrote: > Why are you removing this? Restored. I explained why I did this in a previous e-mail. With my new approach, this is not necessary. > To allow the assignment of |default_results_ = > orig_default_results_;|, etc.? I think that's unnecessary in my suggested > feature implementation. This deserved an explicit CL description comment, or at > least a CL review comment explanation... Copying is bad, m'kay? :) https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... File components/autocomplete/search_suggestion_parser.h (right): https://codereview.chromium.org/471673002/diff/80001/components/autocomplete/... components/autocomplete/search_suggestion_parser.h:100: // If true, this result should never be allowed to be the default match. On 2014/08/14 02:57:23, msw wrote: > Hmm, seems like this identifier and the relevant code could be a bit clearer if > this were |received_since_last_keystroke_| Done (received_after_last_keystroke). I agree that this makes the explanation of what's going on clearer. > always set true on receipt Done. > and set > to false for those results which survive the next keystroke's SortAndCull (or > whatever the corresponding result/match cleanup for re-use is called nowadays). Now the logic to has moved from Start() (when minimal_changes is false) to RemoveAllStaleResults(), which I renamed to RemoveOrReviseOldResults().
On 2014/08/15 00:05:56, Mark P wrote: > However, with the new approach (no old and new copy of the results, and all we > know is what was previously inline autocompleted), we see: > 1. User types "a" > 2. Suggest server says "suppress verbatim" (verbatim_relevance=0). Returns a > query suggestion "abc" with a high score. > 2.5. Client says, screw you, we're not going to let you inline asynchronously. > 2.6. Client notices it now has no results that allowed to be the default match. > 2.7. Client reverts to non-server-provided scores. > 3. User sees the verbatim query in the omnibox and "abc" as a suggestion in the > dropdown. > 4. User types "b". > 5. User sees "ab" in the omnibox; "abc" is only a suggestion in the dropdown > because it has a lesser client-controlled score. > > I feel like there's a way to fix this, though don't know how off the top of my > head and I'm afraid I'm heading home now. [Quick reply, haven't looked too closely, about to head to a concert] Is the extra async-inlinable result you're keeping around wrong? Is there only one async-inlinable suggestion from the server that we'd need to keep for the next keystroke's new behavior? (or, do we really need the full set of prior results, because maybe the top async-inlinable result is now invalid/culled and we actually need a lesser-ranked async-inlinable result?) Is it possible that we're clobbering the inlinable bit on the async result, when instead we should be just relying on the received_after_last_keystroke bit? Does the re-scoring step (2.7) make re-using the modified results on the next keystroke impossible/worse? Perhaps we really do need to keep an unmodified copy of the entire set of async results for the next keystroke... I'll take a closer look tomorrow.
On Thu, Aug 14, 2014 at 6:04 PM, <msw@chromium.org> wrote: > On 2014/08/15 00:05:56, Mark P wrote: > >> However, with the new approach (no old and new copy of the results, and >> all we >> know is what was previously inline autocompleted), we see: >> 1. User types "a" >> 2. Suggest server says "suppress verbatim" (verbatim_relevance=0). >> Returns a >> query suggestion "abc" with a high score. >> 2.5. Client says, screw you, we're not going to let you inline >> > asynchronously. > >> 2.6. Client notices it now has no results that allowed to be the default >> > match. > >> 2.7. Client reverts to non-server-provided scores. >> 3. User sees the verbatim query in the omnibox and "abc" as a suggestion >> in >> > the > >> dropdown. >> 4. User types "b". >> 5. User sees "ab" in the omnibox; "abc" is only a suggestion in the >> dropdown >> because it has a lesser client-controlled score. >> > > I feel like there's a way to fix this, though don't know how off the top >> of my >> head and I'm afraid I'm heading home now. >> > > [Quick reply, haven't looked too closely, about to head to a concert] > Is the extra async-inlinable result you're keeping around wrong? Is there > only > one async-inlinable suggestion from the server that we'd need to keep for > the > next keystroke's new behavior? (or, do we really need the full set of prior > results, because maybe the top async-inlinable result is now > invalid/culled and > we actually need a lesser-ranked async-inlinable result?) Is it possible > that > we're clobbering the inlinable bit on the async result, when instead we > should > be just relying on the received_after_last_keystroke bit? Does the > re-scoring > step (2.7) make re-using the modified results on the next keystroke > impossible/worse? Perhaps we really do need to keep an unmodified copy of > the > entire set of async results for the next keystroke... I'll take a closer > look > tomorrow. > I figured out how to fix this, and the fix is easy. The fix is for step 2.7. If we lack a default match, we don't have to revert all suggestions to local scores. The only thing we need to do is revert the suppress verbatim suggested relevance score (verbatim_relevance=0). We can keep the server scores of all other suggestions, provided we guarantee that at least one default match appears in SearchProvider's final result set regardless of how it scores relative to other suggestions. New patchset coming soon. Maybe there is hope for this changelist after all. --mark > https://codereview.chromium.org/471673002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I still need to test this after the refactoring changelist, but
I think this changelist is good to go pending that.
Note that the current patch under review includes the
refactoring changelist. i.e., you'll see familiar diffs from
that, plus this actual change.
Feel free to review.
No need to review base_search_provider.{h,cc} -- all the
changes in those files are from the refactoring change,
not this new change.
In a few moments, I'm going to add a few code review
tool comments to make this easier to review.
--mark
On 2014/08/15 19:57:56, Mark P wrote:
> I still need to test this after the refactoring changelist, but
> I think this changelist is good to go pending that.
>
> Note that the current patch under review includes the
> refactoring changelist. i.e., you'll see familiar diffs from
> that, plus this actual change.
>
> Feel free to review.
>
> No need to review base_search_provider.{h,cc} -- all the
> changes in those files are from the refactoring change,
> not this new change.
No need to review zero_suggestion_provider.{h,cc} either.
All those changes also are from the refactoring change,
not this new change.
--mark
>
> In a few moments, I'm going to add a few code review
> tool comments to make this easier to review.
>
> --mark
Comments to help the review. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (left): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:436: const TemplateURL* keyword_url = providers_.GetKeywordProviderURL(); This was clobbered in the refactoring changelist. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:851: bool SearchProvider::HasKeywordDefaultMatchInKeywordMode() const { Deleted in refactoring changelist. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (left): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:148: static void RemoveStaleResults( This function implementation was clobbered a long time ago. Apparently I forgot to delete the declaration. Now I'm doing so. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:210: void RemoveAllStaleResults(); RemoveOrReviseOldResults() remained to RemoveOrReviseOldResults() https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:213: void ApplyCalculatedRelevance(); No longer called in the .cc file after this change.
https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:667: void SearchProvider::PersistGoodResultsForOneProvider( Why do we need this at all? Isn't "RemoveOrReviseOldResultsForOneProvider" sufficient? If this is needed, consider another function name that better matches the operation here, like: PromoteAsyncInlineSuggestions or similar. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:670: // having been received on a previous keystroke (as indeed it must have nit: s/on a previous/before the last/ https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:671: // because we prohibit newly-received results from becoming the inline I don't know how this (indeed it must have...) comment explains why the result must is guaranteed to have been received before the last keystroke... Wouldn't that actually be contingent on when this function is called? Can you try to clarify this parenthesized portion, maybe move it to the callsite, or perhaps remove it altogether? https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:870: // Guarantee that if there's a legal default match anywhere in the result Why is this needed now? I thought the |allowed_to_be_default_match| behavior made this type of provider-side fixup unnecessary. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:874: if (default_match != matches.end()) { nit: remove curly braces. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:905: ACMatches::const_iterator SearchProvider::FindTopMatch() const { Remove this, use FindTopMatch(&matches_) at callsites. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1020: // History results are synchronous; they are received at the last keystroke, nit: s/upon/with/ remove ", not after it" https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (left): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:148: static void RemoveStaleResults( On 2014/08/15 20:04:38, Mark P wrote: > This function implementation was clobbered a long time ago. Apparently I forgot > to delete the declaration. Now I'm doing so. > Acknowledged. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:210: void RemoveAllStaleResults(); On 2014/08/15 20:04:38, Mark P wrote: > RemoveOrReviseOldResults() remained to RemoveOrReviseOldResults() Acknowledged. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:213: void ApplyCalculatedRelevance(); On 2014/08/15 20:04:38, Mark P wrote: > No longer called in the .cc file after this change. Acknowledged. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:153: // A helper function for RemoveOrReviseOldResults() that acts only on results nit: remove "that acts only on results from one provider" https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:155: static void RemoveOrReviseOldResultsForOneProvider( Consider UpdateOldResults, CleanupStaleResults, PromoteAsyncResults or similar. The function name should better match what's going on. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:156: bool minimal_changes, SearchSuggestionParser::Results* results); nit: one param per line https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:158: // Returns an iterator to the first match in |matches| which might nit: remove "an iterator to " for a one-liner. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:213: void RemoveOrReviseOldResults(bool minimal_changes); The name here could probably better match the operation, like the helper. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:360: // The inlined query suggestion (if any), left blank if none. nit: remove "(if any)" here and below; "left blank if none" should suffice. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:360: // The inlined query suggestion (if any), left blank if none. nit: should this be "inline" instead of "inlined" here and below? (if it means the suggested inline [query/nav] that may not [yet] actually be inlined). https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:362: // The inlined navsuggestion (if any), left blank/invalid if none. nit: "navigation suggestion" https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:363: GURL inlined_navsuggestion_; nit: inline_navigation_suggestion_match_contents_ (or similar). https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:52: received_after_last_keystroke_(true), Is there any case beyond history that generates synchronous search suggestions? https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.h:101: // keystroke, otherwise it must have come from prior cached results. nit: or from a synchronous provider, right?
https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:258: // We only allow inlinable navsuggestions that were received before the Do we actually want to check received_after_last_keystroke() here, not assign the inline_autocompletion for potential later use, and set allowed_to_be_default_match in a way that might hinder subsequent passes? It seems like we should only enforce !received_after_last_keystroke with the other constraints enforcement, (or with the higher-level default match selection?). If this *is* the right place to do this, why isn't it also done above with the match.allowed_to_be_default_match assignment on line 246? https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1275: !navigation.received_after_last_keystroke() && ditto to my other comment in this round.
All updated, including both batches of your comments. I was right that all tests will pass after rebasing on top of the change. Please take a look. thanks, mark https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:667: void SearchProvider::PersistGoodResultsForOneProvider( On 2014/08/15 21:04:37, msw wrote: > Why do we need this at all? Isn't "RemoveOrReviseOldResultsForOneProvider" > sufficient? If this is needed, consider another function name that better > matches the operation here, like: PromoteAsyncInlineSuggestions or similar. This is definitely needed. Without this function the behavior is correct when typing "a", getting suggest results, adding "b", and then displaying cached results (which we are allowed to inline). But when the suggest reply for "b" returns, we suddenly forget that we were inlining things before. Hence, we need this function to allow us to keep inlining what we were before. I kind of like the current name. PromoteAsyncInlineSuggestions() isn't right because we're not promoting anything. Renamed to a combination of the two: PersistInlineSuggestions() https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:670: // having been received on a previous keystroke (as indeed it must have On 2014/08/15 21:04:37, msw wrote: > nit: s/on a previous/before the last/ Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:671: // because we prohibit newly-received results from becoming the inline On 2014/08/15 21:04:37, msw wrote: > I don't know how this (indeed it must have...) comment explains why the result > must is guaranteed to have been received before the last keystroke... Wouldn't > that actually be contingent on when this function is called? Can you try to > clarify this parenthesized portion, maybe move it to the callsite, or perhaps > remove it altogether? Removed the comment. I think the comment by the function declaration is clear enough, especially in combination with the new function name. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:870: // Guarantee that if there's a legal default match anywhere in the result On 2014/08/15 21:04:37, msw wrote: > Why is this needed now? I thought the |allowed_to_be_default_match| behavior > made this type of provider-side fixup unnecessary. This is needed because AutocompleteResult enforces the behavior: find the top default match and pull it to the top. However, it can only do that if there is a default match to be found. This change makes sure SearchProvider returns one. Previously, we trusted SearchProvider to return one by clobbering relevance scores willy-nilly and then relying on local scoring having the default match beat the suggestions. Now we return the top default match regardless of its relevance score. We don't need to clobber relevance scores as wildly as before. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:874: if (default_match != matches.end()) { On 2014/08/15 21:04:37, msw wrote: > nit: remove curly braces. Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:905: ACMatches::const_iterator SearchProvider::FindTopMatch() const { On 2014/08/15 21:04:38, msw wrote: > Remove this, use FindTopMatch(&matches_) at callsites. I can't do that because some callsites are const functions and the new FindTopMatch I created (needs to) return a non-const iterator. I don't want to have to remove consts from various functions or do a const-cast. Instead, I'm left with the (chosen, in my opinion least bad) option of semi-duplicating (not exactly duplicates because of the const_iterator) fragment of code. I'm open to being pushed in a different direction. (or having this cleaned up later) https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1020: // History results are synchronous; they are received at the last keystroke, On 2014/08/15 21:04:37, msw wrote: > nit: s/upon/with/ remove ", not after it" Did the latter. The former fails to apply. There is no "upon" in the comment. Left the comment with "at" because I think it's better than "with". https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:153: // A helper function for RemoveOrReviseOldResults() that acts only on results On 2014/08/15 21:04:38, msw wrote: > nit: remove "that acts only on results from one provider" Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:155: static void RemoveOrReviseOldResultsForOneProvider( On 2014/08/15 21:04:38, msw wrote: > Consider UpdateOldResults, CleanupStaleResults, PromoteAsyncResults or similar. > The function name should better match what's going on. Chose your first suggestion. Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:156: bool minimal_changes, SearchSuggestionParser::Results* results); On 2014/08/15 21:04:39, msw wrote: > nit: one param per line Now moot. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:158: // Returns an iterator to the first match in |matches| which might On 2014/08/15 21:04:38, msw wrote: > nit: remove "an iterator to " for a one-liner. Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:213: void RemoveOrReviseOldResults(bool minimal_changes); On 2014/08/15 21:04:38, msw wrote: > The name here could probably better match the operation, like the helper. Named it UpdateAllOldResults(), to make it parallel with the helper UpdateOldResults() yet distinguish it from the helper. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:360: // The inlined query suggestion (if any), left blank if none. On 2014/08/15 21:04:38, msw wrote: > nit: remove "(if any)" here and below; "left blank if none" should suffice. Done in both locations. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:360: // The inlined query suggestion (if any), left blank if none. On 2014/08/15 21:04:38, msw wrote: > nit: should this be "inline" instead of "inlined" here and below? (if it means > the suggested inline [query/nav] that may not [yet] actually be inlined). I think "inlined" is better. This result is actually inlined (or at least SearchProvider thinks it is, assuming it's not beaten by something from another provider). This is no "yet" involved here. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:362: // The inlined navsuggestion (if any), left blank/invalid if none. On 2014/08/15 21:04:38, msw wrote: > nit: "navigation suggestion" Done. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:363: GURL inlined_navsuggestion_; On 2014/08/15 21:04:38, msw wrote: > nit: inline_navigation_suggestion_match_contents_ (or similar). Switched to inlined_navigation_suggestion. Left inlineD per my comment above. Would prefer to do navigation suggestion comparisons using the URL. After all, that's what we think of first when we think of a navsuggestion. (Likewise, when we think of a query suggestion, we think of the search string, and that's what's stored in match_contents and displayed to the user.) https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:52: received_after_last_keystroke_(true), On 2014/08/15 21:04:39, msw wrote: > Is there any case beyond history that generates synchronous search suggestions? Good point. I used code search and discovered Shortcuts does as well. Fixed it. (Shortcuts creates suggestions with the CreateSearchSuggestion helper function that has a ton of default values. I fixed shortcuts by setting the right default value there. Shortcuts is the only caller of that helper function with that pile of default values.) FYI, this would not have mattered in practice because no shortcuts results empirically score high enough to inline at the moment. https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.h (right): https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.h:101: // keystroke, otherwise it must have come from prior cached results. On 2014/08/15 21:04:39, msw wrote: > nit: or from a synchronous provider, right? Nice clarification. Added. https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:258: // We only allow inlinable navsuggestions that were received before the On 2014/08/15 21:27:59, msw wrote: > If this *is* the right place to do this, why isn't it also done above with the > match.allowed_to_be_default_match assignment on line 246? Line 246 is fine. Verbatim matches are always allow to be default. It makes little sense to ask when they were returned. (I think you asked about this earlier.) > Do we actually want to check received_after_last_keystroke() here, not assign > the inline_autocompletion for potential later use, and set > allowed_to_be_default_match in a way that might hinder subsequent passes? It > seems like we should only enforce !received_after_last_keystroke with the other > constraints enforcement, (or with the higher-level default match selection?). > I'd rather not do late enforcement. It's kind of a pain. I think if we know we're not going to allow something to be the default match, then we shouldn't fill in information like inline_autocomplete because it'll be misleading. Just because we (you) think in the future that we may want to renege on this allow-to-be-default-match decision and want to correct it later in the code flow, doesn't mean we should build it late correction when it's not currently necessary. https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1275: !navigation.received_after_last_keystroke() && On 2014/08/15 21:27:59, msw wrote: > ditto to my other comment in this round. ditto my other answer. :-)
https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:215: // hence can remain as the inline autocompletion). All other results I just expanded this comment. This seems like the best place for a comment line this. I might rename PersistInlineSuggestions() simply to PersistSuggestions(). What do you think?
On 2014/08/15 23:01:06, Mark P wrote: > https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... > File chrome/browser/autocomplete/search_provider.h (right): > > https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... > chrome/browser/autocomplete/search_provider.h:215: // hence can remain as the > inline autocompletion). All other results > I just expanded this comment. This seems like the best place for a comment line > this. > > I might rename PersistInlineSuggestions() simply to PersistSuggestions(). What > do you think? FYI, if you're bothering to review the test changes (not just the new test I added), here's what's happening: 1. pull the topmost match that has allowed_to_be_default_match to the top of the list of matches 2. change all inlineable matches aside from verbatim and aside from the top-most match to have allowed_to_be_default_match false. This is because we're only allowing things to be the default match if they could've been inlined at some point, so all inlineable things that are outscored by another inlineable thing are now marked as alllowed_to_be_default_match = false 3. change some of the relevance values from 1, 2, 3 to 10, 20, 30 because now I use a relevance value of 1 in search provider (when fixing broken constraints) and I don't want these tests be affected by how the tiebreaking happens to work. I think that covers everything. --mark
My biggest current worry is rotate... https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:667: void SearchProvider::PersistGoodResultsForOneProvider( On 2014/08/15 22:09:21, Mark P wrote: > On 2014/08/15 21:04:37, msw wrote: > > Why do we need this at all? Isn't "RemoveOrReviseOldResultsForOneProvider" > > sufficient? If this is needed, consider another function name that better > > matches the operation here, like: PromoteAsyncInlineSuggestions or similar. > > This is definitely needed. Without this function the behavior is correct when > typing "a", getting suggest results, adding "b", and then displaying cached > results (which we are allowed to inline). But when the suggest reply for "b" > returns, we suddenly forget that we were inlining things before. Hence, we need > this function to allow us to keep inlining what we were before. > > I kind of like the current name. PromoteAsyncInlineSuggestions() isn't right > because we're not promoting anything. Renamed to a combination of the two: > PersistInlineSuggestions() The new name is good, thanks for explaining the need for this to me. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:870: // Guarantee that if there's a legal default match anywhere in the result On 2014/08/15 22:09:21, Mark P wrote: > On 2014/08/15 21:04:37, msw wrote: > > Why is this needed now? I thought the |allowed_to_be_default_match| behavior > > made this type of provider-side fixup unnecessary. > > This is needed because AutocompleteResult enforces the behavior: find the top > default match and pull it to the top. However, it can only do that if there is > a default match to be found. This change makes sure SearchProvider returns one. > Previously, we trusted SearchProvider to return one by clobbering relevance > scores willy-nilly and then relying on local scoring having the default match > beat the suggestions. Now we return the top default match regardless of its > relevance score. We don't need to clobber relevance scores as wildly as before. My question is "why do we need to rotate the matches list?". I guess it's because the constraint enforcement might now leave the "top match" (highest-scored-inlinable match) scored lowly and ordered lower down the |matches| list than kMaxMatches, so the step below would otherwise omit the top result. Then, wouldn't rotating the list potentially use more lowly ranked/ordered results instead of the prior sorting? If so, how is that not caught by a unit test? (ie. a test should ensure that we have an inlinable match, even if it's the lowest-ranked of N>kMaxMatches results). Instead of rotating, can we just yank the "top match" to the top of the |matches| list? https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:905: ACMatches::const_iterator SearchProvider::FindTopMatch() const { On 2014/08/15 22:09:21, Mark P wrote: > On 2014/08/15 21:04:38, msw wrote: > > Remove this, use FindTopMatch(&matches_) at callsites. > > I can't do that because some callsites are const functions and the new > FindTopMatch I created (needs to) return a non-const iterator. I don't want to > have to remove consts from various functions or do a const-cast. Instead, I'm > left with the (chosen, in my opinion least bad) option of semi-duplicating (not > exactly duplicates because of the const_iterator) fragment of code. > > I'm open to being pushed in a different direction. > (or having this cleaned up later) I guess leave the duplication for now, unless you can remove the std::rotate step, and thus no longer need a non-const iterator. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1020: // History results are synchronous; they are received at the last keystroke, On 2014/08/15 22:09:21, Mark P wrote: > On 2014/08/15 21:04:37, msw wrote: > > nit: s/upon/with/ remove ", not after it" > > Did the latter. > > The former fails to apply. There is no "upon" in the comment. Left the > comment with "at" because I think it's better than "with". I meant to encourage "upon the last keystroke", but what you have is fine. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:360: // The inlined query suggestion (if any), left blank if none. On 2014/08/15 22:09:22, Mark P wrote: > On 2014/08/15 21:04:38, msw wrote: > > nit: should this be "inline" instead of "inlined" here and below? (if it means > > the suggested inline [query/nav] that may not [yet] actually be inlined). > > I think "inlined" is better. This result is actually inlined (or at least > SearchProvider thinks it is, assuming it's not beaten by something from another > provider). This is no "yet" involved here. Acknowledged. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:363: GURL inlined_navsuggestion_; On 2014/08/15 22:09:22, Mark P wrote: > On 2014/08/15 21:04:38, msw wrote: > > nit: inline_navigation_suggestion_match_contents_ (or similar). > > Switched to inlined_navigation_suggestion. > Left inlineD per my comment above. > Would prefer to do navigation suggestion comparisons using the URL. After all, > that's what we think of first when we think of a navsuggestion. (Likewise, when > we think of a query suggestion, we think of the search string, and that's what's > stored in match_contents and displayed to the user.) Acknowledged. https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... File components/omnibox/search_suggestion_parser.cc (right): https://codereview.chromium.org/471673002/diff/160001/components/omnibox/sear... components/omnibox/search_suggestion_parser.cc:52: received_after_last_keystroke_(true), On 2014/08/15 22:09:22, Mark P wrote: > On 2014/08/15 21:04:39, msw wrote: > > Is there any case beyond history that generates synchronous search > suggestions? > > Good point. I used code search and discovered Shortcuts does as well. Fixed > it. > > (Shortcuts creates suggestions with the CreateSearchSuggestion helper function > that has a ton of default values. I fixed shortcuts by setting the right > default value there. Shortcuts is the only caller of that helper function with > that pile of default values.) > > FYI, this would not have mattered in practice because no shortcuts results > empirically score high enough to inline at the moment. Acknowledged. https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/220001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:258: // We only allow inlinable navsuggestions that were received before the On 2014/08/15 22:09:22, Mark P wrote: > On 2014/08/15 21:27:59, msw wrote: > > If this *is* the right place to do this, why isn't it also done above with the > > match.allowed_to_be_default_match assignment on line 246? > > Line 246 is fine. Verbatim matches are always allow to be default. It makes > little sense to ask when they were returned. (I think you asked about this > earlier.) > > > Do we actually want to check received_after_last_keystroke() here, not assign > > the inline_autocompletion for potential later use, and set > > allowed_to_be_default_match in a way that might hinder subsequent passes? It > > seems like we should only enforce !received_after_last_keystroke with the > other > > constraints enforcement, (or with the higher-level default match selection?). > > > > I'd rather not do late enforcement. It's kind of a pain. I think if we know > we're not going to allow something to be the default match, then we shouldn't > fill in information like inline_autocomplete because it'll be misleading. Just > because we (you) think in the future that we may want to renege on this > allow-to-be-default-match decision and want to correct it later in the code > flow, doesn't mean we should build it late correction when it's not currently > necessary. Ah, I get it now, I mistakenly thought that marking this *match* as |allowed_to_be_default_match == false| would prevent the re-used suggest *result* from being inlined after the next keystroke. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:119: // These calls use a number of default values. For instance, they assumes nit: "they assume" https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:120: // that if this match is from a keyword provider than the user is in keyword nit: s/than/, then/ https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:474: // Record the inlined suggestion (if any) for future use. nit: s/inlined/top/ https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:483: if (AutocompleteMatch::IsSearchType(first_match->type)) { optional nit: remove curly braces. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1260: // the last keystroke. (We don't want asynchronous inline autocompletions.) nit: "keystroke, to prevent asynchronous inline autocompletion changes." https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:215: // hence can remain as the inline autocompletion). All other results On 2014/08/15 23:01:06, Mark P wrote: > I just expanded this comment. This seems like the best place for a comment line > this. > > I might rename PersistInlineSuggestions() simply to PersistSuggestions(). What > do you think? I'd prefer "PersistTopSuggestions", matching my member name suggestions below. Suggested comment (use this or let it inspire a rewrite): "Given new asynchronous results, ensure that we don't clobber the current top results, which were determined synchronously on the last keystroke." Then, in the function body (or maybe here, I guess): "Mark any results matching the current top results as having been received prior to the last keystroke. That prevents asynchronous updates from clobbering top results, which may be used for inline autocompletion. Other results don't need similar changes, because they shouldn't be promoted asynchronously anyway." https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:364: // The inlined query suggestion, left blank if none. nit: consider s/inlined/top/ for comments and identifiers.
https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:870: // Guarantee that if there's a legal default match anywhere in the result On 2014/08/15 23:31:10, msw wrote: > On 2014/08/15 22:09:21, Mark P wrote: > > On 2014/08/15 21:04:37, msw wrote: > > > Why is this needed now? I thought the |allowed_to_be_default_match| behavior > > > made this type of provider-side fixup unnecessary. > > > > This is needed because AutocompleteResult enforces the behavior: find the top > > default match and pull it to the top. However, it can only do that if there > is > > a default match to be found. This change makes sure SearchProvider returns > one. > > Previously, we trusted SearchProvider to return one by clobbering relevance > > scores willy-nilly and then relying on local scoring having the default match > > beat the suggestions. Now we return the top default match regardless of its > > relevance score. We don't need to clobber relevance scores as wildly as > before. > > My question is "why do we need to rotate the matches list?". I guess it's > because the constraint enforcement might now leave the "top match" > (highest-scored-inlinable match) scored lowly and ordered lower down the > |matches| list than kMaxMatches, so the step below would otherwise omit the top > result. Then, wouldn't rotating the list potentially use more lowly > ranked/ordered results instead of the prior sorting? If so, how is that not > caught by a unit test? (ie. a test should ensure that we have an inlinable > match, even if it's the lowest-ranked of N>kMaxMatches results). Instead of > rotating, can we just yank the "top match" to the top of the |matches| list? This call to rotate does a remove from current location and insert at the beginning. In other word, it just yanks it (as you request). Made more explicit in the comment. https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:905: ACMatches::const_iterator SearchProvider::FindTopMatch() const { On 2014/08/15 23:31:10, msw wrote: > On 2014/08/15 22:09:21, Mark P wrote: > > On 2014/08/15 21:04:38, msw wrote: > > > Remove this, use FindTopMatch(&matches_) at callsites. > > > > I can't do that because some callsites are const functions and the new > > FindTopMatch I created (needs to) return a non-const iterator. I don't want > to > > have to remove consts from various functions or do a const-cast. Instead, I'm > > left with the (chosen, in my opinion least bad) option of semi-duplicating > (not > > exactly duplicates because of the const_iterator) fragment of code. > > > > I'm open to being pushed in a different direction. > > (or having this cleaned up later) > > I guess leave the duplication for now, unless you can remove the std::rotate > step, and thus no longer need a non-const iterator. Acknowledged. (I can't remove the rotate call.) https://codereview.chromium.org/471673002/diff/160001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1020: // History results are synchronous; they are received at the last keystroke, On 2014/08/15 23:31:10, msw wrote: > On 2014/08/15 22:09:21, Mark P wrote: > > On 2014/08/15 21:04:37, msw wrote: > > > nit: s/upon/with/ remove ", not after it" > > > > Did the latter. > > > > The former fails to apply. There is no "upon" in the comment. Left the > > comment with "at" because I think it's better than "with". > > I meant to encourage "upon the last keystroke", but what you have is fine. Switched to "upon". https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/base_search_provider.cc (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:119: // These calls use a number of default values. For instance, they assumes On 2014/08/15 23:31:10, msw wrote: > nit: "they assume" Done. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/base_search_provider.cc:120: // that if this match is from a keyword provider than the user is in keyword On 2014/08/15 23:31:10, msw wrote: > nit: s/than/, then/ Done. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:474: // Record the inlined suggestion (if any) for future use. On 2014/08/15 23:31:11, msw wrote: > nit: s/inlined/top/ Done. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:483: if (AutocompleteMatch::IsSearchType(first_match->type)) { On 2014/08/15 23:31:10, msw wrote: > optional nit: remove curly braces. Done. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.cc:1260: // the last keystroke. (We don't want asynchronous inline autocompletions.) On 2014/08/15 23:31:11, msw wrote: > nit: "keystroke, to prevent asynchronous inline autocompletion changes." Done. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:215: // hence can remain as the inline autocompletion). All other results On 2014/08/15 23:31:11, msw wrote: > On 2014/08/15 23:01:06, Mark P wrote: > > I just expanded this comment. This seems like the best place for a comment > line > > this. > > > > I might rename PersistInlineSuggestions() simply to PersistSuggestions(). > What > > do you think? > > I'd prefer "PersistTopSuggestions", matching my member name suggestions below. > > Suggested comment (use this or let it inspire a rewrite): > "Given new asynchronous results, ensure that we don't clobber the current top > results, which were determined synchronously on the last keystroke." > > Then, in the function body (or maybe here, I guess): > "Mark any results matching the current top results as having been received prior > to the last keystroke. That prevents asynchronous updates from clobbering top > results, which may be used for inline autocompletion. Other results don't need > similar changes, because they shouldn't be promoted asynchronously anyway." Done. Nice comments. https://codereview.chromium.org/471673002/diff/260001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider.h:364: // The inlined query suggestion, left blank if none. On 2014/08/15 23:31:11, msw wrote: > nit: consider s/inlined/top/ for comments and identifiers. Done.
https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:444: // Send the query twice in order to bypass checks about whether a single nit: Can you pull anything out of this loop? (|fetcher| decl/init?) https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1205: // Send the query twice in order to bypass checks about whether a single nit: Can you pull anything out of this loop? (fetcher decls/inits?) https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1468: // Send the query twice in order to bypass checks about whether a single If we do this, then why do we need to alter the values above? Shouldn't a second query/result pass yield the same inline results as we previously expected? (or would that be a |minimal_changes| type pass, which would alter |received_after_last_keystroke|, and then why bother with the second query?) Ditto elsewhere with changed expectations and two query passes. Perhaps update the comment to avoid confusion like mine? https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1630: // It'll then appear first. nit: "The keyword verbatim match will then appear first." https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2002: { { "ab1", true }, { "ab2", false }, { "ab", true }, kEmptyMatch } }, Why is "ab2" false? Seems like it could be inlinable to me... (many dittos) https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2032: "{\"google:suggestrelevance\":[1302, 1301]," nit: be consistent about suggestrelevance / verbatimrelevance ordering. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2079: // Then, send the query "ab" and get the provided JSON reply, Before sending the second query, can you ensure that only the verbatim result is allowed to be default?
The code lgtm, but I'd like myself and Peter to take a closer look at the test changes...
Message was sent while issue was closed.
Committed patchset #16 manually as 290058 (presubmit successful).
Message was sent while issue was closed.
Comments on your requests for test improvements. Please answer at your leisure. Seriously, there's no hurry whatsoever. --mark https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:444: // Send the query twice in order to bypass checks about whether a single On 2014/08/15 23:59:14, msw wrote: > nit: Can you pull anything out of this loop? (|fetcher| decl/init?) Not really. We need the GetFetchByID call to be run anew after the QueryForInput. I guess I can put the declaration but not initialization about the loop but that doesn't appeal to me much. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1205: // Send the query twice in order to bypass checks about whether a single On 2014/08/15 23:59:14, msw wrote: > nit: Can you pull anything out of this loop? (fetcher decls/inits?) See reply above. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1468: // Send the query twice in order to bypass checks about whether a single On 2014/08/15 23:59:14, msw wrote: > If we do this, then why do we need to alter the values above? Shouldn't a second > query/result pass yield the same inline results as we previously expected? It will yield the same top-allowed-to-be-default match as before, and the same relative order of all other matches. However, these two sets may change place in relation to each other. In particular, we move the top-allowed-to-be-default match to the top of the list. > (or > would that be a |minimal_changes| type pass, which would alter > |received_after_last_keystroke|, and then why bother with the second query?) > Ditto elsewhere with changed expectations and two query passes. Perhaps update > the comment to avoid confusion like mine? Sending the query only once would change more allowed_to_be_default_values. In particular, all non-verbatim matches inlineable matches would not longer be allowed to be the default match. Sending the query twice means that they are eligible. In retrospect, I only need to all QueryForInput() twice; I don't need to provide two suggest server replies. I'm mainly examining the *synchronous* results after the second query/keystroke, generated from the cached results from the earlier query. However, I like the idea of examining the results after the second query-response. If I didn't do this, I wouldn't be testing the necessity of PersistTopResults(). I don't know if this is all worth explaining (even in a test). I think something like my original comment works okay, or maybe this rephrasing: // Send the query twice in order to have a synchronous pass after the first reply is received. This is necessary because SearchProvider doesn't allow an asynchronous reply to change the default match. What do you think about this comment? Does it suffice? P.S. What do you think about running many of these "send the query twice" tests twice, once looking at the matches after the second synchronous request, once looking at the matches after the second (duplicate) reply? This tests both before- and after- the states. (They should be identical because the responses are identical.) Is this worth it? https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1630: // It'll then appear first. On 2014/08/15 23:59:14, msw wrote: > nit: "The keyword verbatim match will then appear first." Done. (You'll see this fixed in the patch I will later upload to clean up the tests.) https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2002: { { "ab1", true }, { "ab2", false }, { "ab", true }, kEmptyMatch } }, On 2014/08/15 23:59:14, msw wrote: > Why is "ab2" false? Seems like it could be inlinable to me... (many dittos) Only the top inlineable match is marked as allowed to be the default match. This is because the other match ("ab2" in this example) was not displayed at the top match on the synchronous pass, and hence when we get the second suggest server reply back, we mark it as newly-received / not-allowed-to-be-default-match. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2032: "{\"google:suggestrelevance\":[1302, 1301]," On 2014/08/15 23:59:14, msw wrote: > nit: be consistent about suggestrelevance / verbatimrelevance ordering. Fixed. This was the only one I found out of order. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2079: // Then, send the query "ab" and get the provided JSON reply, On 2014/08/15 23:59:14, msw wrote: > Before sending the second query, can you ensure that only the verbatim result is > allowed to be default? Done. You'll see this in a new changelist.
Message was sent while issue was closed.
https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:444: // Send the query twice in order to bypass checks about whether a single On 2014/08/16 00:37:02, Mark P wrote: > On 2014/08/15 23:59:14, msw wrote: > > nit: Can you pull anything out of this loop? (|fetcher| decl/init?) > > Not really. We need the GetFetchByID call to be run anew after the > QueryForInput. I guess I can put the declaration but not initialization about > the loop but that doesn't appeal to me much. Acknowledged. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1205: // Send the query twice in order to bypass checks about whether a single On 2014/08/16 00:37:02, Mark P wrote: > On 2014/08/15 23:59:14, msw wrote: > > nit: Can you pull anything out of this loop? (fetcher decls/inits?) > > See reply above. Acknowledged. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1368: { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":10}]", Another qq, why did you need to change the verbatim and suggested relevance scores in some places? https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1468: // Send the query twice in order to bypass checks about whether a single On 2014/08/16 00:37:02, Mark P wrote: > On 2014/08/15 23:59:14, msw wrote: > > If we do this, then why do we need to alter the values above? Shouldn't a > second > > query/result pass yield the same inline results as we previously expected? > > It will yield the same top-allowed-to-be-default match as before, and the same > relative order of all other matches. However, these two sets may change place > in relation to each other. In particular, we move the top-allowed-to-be-default > match to the top of the list. > > > (or > > would that be a |minimal_changes| type pass, which would alter > > |received_after_last_keystroke|, and then why bother with the second query?) > > Ditto elsewhere with changed expectations and two query passes. Perhaps update > > the comment to avoid confusion like mine? > > Sending the query only once would change more allowed_to_be_default_values. In > particular, all non-verbatim matches inlineable matches would not longer be > allowed to be the default match. Sending the query twice means that they are > eligible. > > In retrospect, I only need to all QueryForInput() twice; I don't need to provide > two suggest server replies. I'm mainly examining the *synchronous* results > after the second query/keystroke, generated from the cached results from the > earlier query. > > However, I like the idea of examining the results after the second > query-response. If I didn't do this, I wouldn't be testing the necessity of > PersistTopResults(). > > I don't know if this is all worth explaining (even in a test). I think > something like my original comment works okay, or maybe this rephrasing: > // Send the query twice in order to have a synchronous pass after the first > reply is received. This is necessary because SearchProvider doesn't allow an > asynchronous reply to change the default match. > > What do you think about this comment? Does it suffice? I do like that comment. > P.S. What do you think about running many of these "send the query twice" tests > twice, once looking at the matches after the second synchronous request, once > looking at the matches after the second (duplicate) reply? This tests both > before- and after- the states. (They should be identical because the responses > are identical.) Is this worth it? I think adding a new test that explicitly checks for that could be good. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:2002: { { "ab1", true }, { "ab2", false }, { "ab", true }, kEmptyMatch } }, On 2014/08/16 00:37:02, Mark P wrote: > On 2014/08/15 23:59:14, msw wrote: > > Why is "ab2" false? Seems like it could be inlinable to me... (many dittos) > > Only the top inlineable match is marked as allowed to be the default match. > This is because the other match ("ab2" in this example) was not displayed at the > top match on the synchronous pass, and hence when we get the second suggest > server reply back, we mark it as newly-received / > not-allowed-to-be-default-match. Acknowledged.
Message was sent while issue was closed.
Hi Mike, This change that we hurried to submit before the branch point did not make the cut. It was also actually reverted, but even if it hadn't been reverted it wouldn't have made the cut. I think it's about time to finish polishing it and attempting to submit it again. Here are the replies to your last set of comments about improving tests. I posted the new code in a new changelist: https://codereview.chromium.org/481693004/ It has no differences from the most recent patchset here except in search_provider_unittest.cc and chrome/browser/ui/search/instant_extended_interactive_uitest.cc. Please take a look at that other changelist, mark https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1368: { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":10}]", On 2014/08/16 01:10:17, msw wrote: > Another qq, why did you need to change the verbatim and suggested relevance > scores in some places? You'll notice that when we roll-back the request from the server to suppress verbatim, we make the new relevance score 1. Because I didn't want any tests to depend on the tie-breaking between a server-provided score of "1" and a rolled-back score of "1", I made all the low-value tests here slightly bigger values. https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1468: // Send the query twice in order to bypass checks about whether a single On 2014/08/16 01:10:17, msw wrote: > On 2014/08/16 00:37:02, Mark P wrote: > > On 2014/08/15 23:59:14, msw wrote: > > > If we do this, then why do we need to alter the values above? Shouldn't a > > second > > > query/result pass yield the same inline results as we previously expected? > > > > It will yield the same top-allowed-to-be-default match as before, and the same > > relative order of all other matches. However, these two sets may change place > > in relation to each other. In particular, we move the > top-allowed-to-be-default > > match to the top of the list. > > > > > (or > > > would that be a |minimal_changes| type pass, which would alter > > > |received_after_last_keystroke|, and then why bother with the second query?) > > > Ditto elsewhere with changed expectations and two query passes. Perhaps > update > > > the comment to avoid confusion like mine? > > > > Sending the query only once would change more allowed_to_be_default_values. > In > > particular, all non-verbatim matches inlineable matches would not longer be > > allowed to be the default match. Sending the query twice means that they are > > eligible. > > > > In retrospect, I only need to all QueryForInput() twice; I don't need to > provide > > two suggest server replies. I'm mainly examining the *synchronous* results > > after the second query/keystroke, generated from the cached results from the > > earlier query. > > > > However, I like the idea of examining the results after the second > > query-response. If I didn't do this, I wouldn't be testing the necessity of > > PersistTopResults(). > > > > I don't know if this is all worth explaining (even in a test). I think > > something like my original comment works okay, or maybe this rephrasing: > > // Send the query twice in order to have a synchronous pass after the first > > reply is received. This is necessary because SearchProvider doesn't allow an > > asynchronous reply to change the default match. > > > > What do you think about this comment? Does it suffice? > > I do like that comment. Replaced all places where I had the previous comment explaining about sending a query twice with the new comment. > > > P.S. What do you think about running many of these "send the query twice" > tests > > twice, once looking at the matches after the second synchronous request, once > > looking at the matches after the second (duplicate) reply? This tests both > > before- and after- the states. (They should be identical because the > responses > > are identical.) Is this worth it? > > I think adding a new test that explicitly checks for that could be good. I extended DontInlineAutocompleteAsynchronously to test results of both asynchronous passes and synchronous passes. It now tests my "P.S." suggestion and more.
Message was sent while issue was closed.
https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... File chrome/browser/autocomplete/search_provider_unittest.cc (right): https://codereview.chromium.org/471673002/diff/280001/chrome/browser/autocomp... chrome/browser/autocomplete/search_provider_unittest.cc:1368: { "[\"a\",[\"a1\"],[],[],{\"google:verbatimrelevance\":10}]", On 2014/08/25 22:45:08, Mark P wrote: > On 2014/08/16 01:10:17, msw wrote: > > Another qq, why did you need to change the verbatim and suggested relevance > > scores in some places? > > You'll notice that when we roll-back the request from the server to suppress > verbatim, we make the new relevance score 1. Because I didn't want any tests to > depend on the tie-breaking between a server-provided score of "1" and a > rolled-back score of "1", I made all the low-value tests here slightly bigger > values. Acknowledged. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
