Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10832256:
Experimental AutocompleteProvider for zerosuggest. (Closed)
|
Created:
8 years, 4 months ago by Jered Modified:
8 years, 4 months ago CC:
chromium-reviews, James Su Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionExperimental AutocompleteProvider for zerosuggest.
Zerosuggest is an experimental feature which would offer query suggestions when
a user focuses in the omnibox prior to typing any text. The goal of this change
is to allow us to do a limited dogfood of this feature in canary to assess data
quality and UX in situ.
This experimental implementation uses the existing AutocompleteProvider
framework and attempts to make minimal changes to omnibox interaction. The
provider is behind a new --zerosuggest-url-prefix flag which must be explicitly
pointed to a suggestion server for testing. The provider goes to some pains to
mimic the current focus behavior of the omnibox by inserting a placeholder
suggestion for the current URL which is filled and completed inline. "Enter"
still refreshes the page, and the url can still be copy+pasted or replaced;
however, backspace does not work, and it's complicated to deal with the case
where the user unfocuses the omnibox and then focuses again.
BUG=
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase. #Patch Set 3 : Rip out timer. #
Total comments: 12
Patch Set 4 : Rename flag and add comments. #
Total comments: 2
Patch Set 5 : Zerosuggest->ZeroSuggest. #Patch Set 6 : Hyphenate flag name. #
Total comments: 30
Patch Set 7 : Address comments. #Patch Set 8 : Fix constant name. #
Total comments: 2
Patch Set 9 : Qualify todo. #Patch Set 10 : Address comments. #
Total comments: 15
Patch Set 11 : Rebase. #Patch Set 12 : Address comments, avoid stale matches. #Messages
Total messages: 31 (0 generated)
This tries too hard to use the current query-running model for a UX that it was never designed to support. The way you have to fake up a match for the current omnibox text is evidence of this. There are two routes to go here, not necessarily mutually exclusive. One is to change the omnibox dropdown design so that it's merely additional suggestions, and there's no match for the current typing. David wants to do this, and doing this would more easily allow you to open a dropdown without interfering with the current typing. The other change would be to actually add plumbing for zero-suggest mode to basically be a whole different animal as far as the edit, popup, and controller are concerned. This allows you to preserve the existing meanings of edit model variables like "is a user edit in progress" and show the zero-suggest results without interfering with anything else. It's not clear to me if something like this is still useful/necessary if the above change is made. https://chromiumcodereview.appspot.com/10832256/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/zerosuggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/zerosuggest_provider.cc:58: // Stop() should cancel this timer. Find a different solution. Providers must not know about this kind of thing. The fix should be in the Mac omnibox code.
Yeah, we know that this isn't the best way of building this UI. It's not intended to be - we're planning on doing the former of your two proposals, but that's going to take quite a bit of time to do right, and in the meantime we'd like to dogfood an experience for data validation and iteration. All this code is just intended for is that dogfood - it will never be flipped on by default and will be pulled out once we have a better supporting structure in place. We'd just really like to do data iteration in parallel with the omnibox work. Does that seem reasonable?
That should be OK, I think. You might want comments in various places like TODO: Rip this out and replace with blah blah when blah.
I still think the focus thingy should be investigated and fixed in the mac code itself though, so that you don't call into the provider unless you know you should.
On 2012/08/10 21:54:29, Peter Kasting wrote: > I still think the focus thingy should be investigated and fixed in the mac code > itself though, so that you don't call into the provider unless you know you > should. Yeah, I agree with that.
On 2012/08/10 21:54:29, Peter Kasting wrote: > I still think the focus thingy should be investigated and fixed in the mac code > itself though, so that you don't call into the provider unless you know you > should. Thanks for the quick replies. I'll find a real fix for the UI bug, and add some "rip this out" TODOs to emphasize this is not the right way. I'm happy to help code the right way once we figure that out, too.
Added a bunch of TODOs that should scare people away from thinking this is for real. https://chromiumcodereview.appspot.com/10832256/diff/1/chrome/browser/autocom... File chrome/browser/autocomplete/zerosuggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/1/chrome/browser/autocom... chrome/browser/autocomplete/zerosuggest_provider.cc:58: // Stop() should cancel this timer. On 2012/08/10 21:33:33, Peter Kasting wrote: > Find a different solution. Providers must not know about this kind of thing. > The fix should be in the Mac omnibox code. Done. This problem vanished after my rebase. Lucky me.
FYI, there are some issues if you turn this on along with the extended api for instant but we can address those in a follow-up change. Thanks, Samarth https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.h:92: SEARCH_ZEROSUGGEST_URL, // A URL suggested by zerosuggest. Is this just future-proofing in case we do suggest URLs? (As far as I know, we don't at the moment.) https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... File chrome/browser/autocomplete/zerosuggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) You should also consider checking the scheme for url_prefix and only sending requests over HTTPS. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:82: const bool request_succeeded = If json_data is empty at this point, do we still consider request to have succeeded? https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:164: AutocompleteMatch match(this, 2000, false, Make 2000 a constant, and add TODO about getting scores from the backend. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:183: // TODO(jered): Rip out user_text_is_url logic when AddMatchForCurrentURL goes. nit: long line https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... chrome/common/chrome_switches.cc:1355: // If nonempty, fetch experimental zerosuggest suggestions by appending to this You already do mention "experimental" here but perhaps it would be good to make it even clearer in general by naming the switch experimental-zerosuggest-url-prefix or (shorter) experimental-url-suggest-prefix or something else along those lines.
https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/autocomplete_match.h:92: SEARCH_ZEROSUGGEST_URL, // A URL suggested by zerosuggest. On 2012/08/13 17:42:37, samarth wrote: > Is this just future-proofing in case we do suggest URLs? (As far as I know, we > don't at the moment.) Yeah. For now it's only used for the placeholder suggestion for the current url, so that it looks like a nav suggestion. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... File chrome/browser/autocomplete/zerosuggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/13 17:42:37, samarth wrote: > You should also consider checking the scheme for url_prefix and only sending > requests over HTTPS. I'm not sure we need that for testing. Seems like it'd confuse us but not have much security benefit. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:82: const bool request_succeeded = On 2012/08/13 17:42:37, samarth wrote: > If json_data is empty at this point, do we still consider request to have > succeeded? Yep, but results_updated will be true only if the json parses (it won't if it's empty). This is mostly ripped off from SearchProvider. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:164: AutocompleteMatch match(this, 2000, false, On 2012/08/13 17:42:37, samarth wrote: > Make 2000 a constant, and add TODO about getting scores from the backend. Done. I added a comment about why 2000 here, and added a TODO for the other relevance magic number below. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... chrome/browser/autocomplete/zerosuggest_provider.cc:183: // TODO(jered): Rip out user_text_is_url logic when AddMatchForCurrentURL goes. On 2012/08/13 17:42:37, samarth wrote: > nit: long line Done. https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... chrome/common/chrome_switches.cc:1355: // If nonempty, fetch experimental zerosuggest suggestions by appending to this On 2012/08/13 17:42:37, samarth wrote: > You already do mention "experimental" here but perhaps it would be good to make > it even clearer in general by naming the switch > experimental-zerosuggest-url-prefix or (shorter) experimental-url-suggest-prefix > or something else along those lines. Let's just go with --experimental-zerosuggest-url-prefix, I don't expect that I will be typing it out much.
+sky for chrome/ Peter/David, can you guys give this another look? On 2012/08/13 22:23:59, Jered wrote: > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > File chrome/browser/autocomplete/autocomplete_match.h (right): > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > chrome/browser/autocomplete/autocomplete_match.h:92: SEARCH_ZEROSUGGEST_URL, // > A URL suggested by zerosuggest. > On 2012/08/13 17:42:37, samarth wrote: > > Is this just future-proofing in case we do suggest URLs? (As far as I know, we > > don't at the moment.) > > Yeah. For now it's only used for the placeholder suggestion for the current url, > so that it looks like a nav suggestion. > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > File chrome/browser/autocomplete/zerosuggest_provider.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > chrome/browser/autocomplete/zerosuggest_provider.cc:45: if (spec.empty() || > url.scheme() != chrome::kHttpScheme) > On 2012/08/13 17:42:37, samarth wrote: > > You should also consider checking the scheme for url_prefix and only sending > > requests over HTTPS. > > I'm not sure we need that for testing. Seems like it'd confuse us but not have > much security benefit. > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > chrome/browser/autocomplete/zerosuggest_provider.cc:82: const bool > request_succeeded = > On 2012/08/13 17:42:37, samarth wrote: > > If json_data is empty at this point, do we still consider request to have > > succeeded? > > Yep, but results_updated will be true only if the json parses (it won't if it's > empty). This is mostly ripped off from SearchProvider. > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > chrome/browser/autocomplete/zerosuggest_provider.cc:164: AutocompleteMatch > match(this, 2000, false, > On 2012/08/13 17:42:37, samarth wrote: > > Make 2000 a constant, and add TODO about getting scores from the backend. > > Done. I added a comment about why 2000 here, and added a TODO for the other > relevance magic number below. > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/browser/aut... > chrome/browser/autocomplete/zerosuggest_provider.cc:183: // TODO(jered): Rip out > user_text_is_url logic when AddMatchForCurrentURL goes. > On 2012/08/13 17:42:37, samarth wrote: > > nit: long line > > Done. > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... > File chrome/common/chrome_switches.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/12001/chrome/common/chro... > chrome/common/chrome_switches.cc:1355: // If nonempty, fetch experimental > zerosuggest suggestions by appending to this > On 2012/08/13 17:42:37, samarth wrote: > > You already do mention "experimental" here but perhaps it would be good to > make > > it even clearer in general by naming the switch > > experimental-zerosuggest-url-prefix or (shorter) > experimental-url-suggest-prefix > > or something else along those lines. > > Let's just go with --experimental-zerosuggest-url-prefix, I don't expect that I > will be typing it out much.
c/b c/c LGTM I did not look at the omnibox changes though, wait for Peter on that. https://chromiumcodereview.appspot.com/10832256/diff/17/chrome/browser/autoco... File chrome/browser/autocomplete/zerosuggest_provider.h (right): https://chromiumcodereview.appspot.com/10832256/diff/17/chrome/browser/autoco... chrome/browser/autocomplete/zerosuggest_provider.h:49: class ZerosuggestProvider : public AutocompleteProvider, How come this isn't ZeroSuggest?
https://chromiumcodereview.appspot.com/10832256/diff/17/chrome/browser/autoco... File chrome/browser/autocomplete/zerosuggest_provider.h (right): https://chromiumcodereview.appspot.com/10832256/diff/17/chrome/browser/autoco... chrome/browser/autocomplete/zerosuggest_provider.h:49: class ZerosuggestProvider : public AutocompleteProvider, On 2012/08/14 17:14:38, sky wrote: > How come this isn't ZeroSuggest? Done.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/autocomplete_match.h:92: SEARCH_ZEROSUGGEST_URL, // A URL suggested by zerosuggest. SEARCH_ZEROSUGGEST_URL is a contradiction. It's not clear to me why you need new types anyway. SEARCH_SUGGEST and NAVSUGGEST seem like existing types that would serve you fine. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:42: const GURL& url, Nit: Can fit on previous line, then indent next arg even https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:44: const std::string& spec = url.possibly_invalid_spec(); Shouldn't |url| always be valid here? Seems like you can DCHECK is_valid() and then just get the spec. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) How can the spec be empty? If it can, then my comment about validity above is wrong. Nit: {}, or move comment above conditional (3 places) https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:76: if (base::CodepageToUTF16(json_data, charset.c_str(), Why do charset -> utf16 -> utf8 instead of just charset -> utf8? Will this even ever be non-UTF8 to begin with? I thought most Google services spoke UTF8 exclusively. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:167: AutocompleteMatch match(this, kMaxRelevance, false, The relevance scores you use here and below mean that if these zerosuggest matches stick around as the user starts typing, they'll outrank everything from every other provider. That seems wrong and will lead to a really bad UX (e.g. you'll completely disable inline autocompletion). It seems like you should either have zerosuggest results disappear entirely once the user starts typing, or be ranked very low so everything else outranks them, or something. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); Nit: I think this change is safe, but it's not totally clear to me why you need to make it. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:537: match.type == AutocompleteMatch::SEARCH_ZEROSUGGEST || Nit: Don't look at the match type, look at match.provider.name. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:704: // in the omnibox. Then we can just run zero-suggest as a normal provider. Nit: It's not obvious to me what this comment means. When you say "decoupled from typing", I'm thinking of the idea to remove the "what you typed" suggestion from the dropdown. That won't have any effect on whether you need to make a call here to start zerosuggest. Won't you still need a call here regardless? I mean, ultimately if we want to show suggestions on focus, then we'll need to do something to show suggestions when we get focus. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:717: autocomplete_controller_->StopZeroSuggest(); This seems unnecessary if you revert your change to StopAutocomplete() above, since then losing focus will call through to that and unconditionally Stop() all the providers. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/common/chrom... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/common/chrom... chrome/common/chrome_switches.cc:680: // this prefix of a URL. This is pretty convoluted. Are you trying to avoid publicizing the URL you're testing with? If not it seems like it'd be much simpler to simply have an on/off flag and hardcode the relevant URL to fetch in zerosuggest_provider.cc.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/common/chrom... File chrome/common/chrome_switches.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/common/chrom... chrome/common/chrome_switches.cc:680: // this prefix of a URL. On 2012/08/14 19:20:02, Peter Kasting wrote: > This is pretty convoluted. Are you trying to avoid publicizing the URL you're > testing with? If not it seems like it'd be much simpler to simply have an > on/off flag and hardcode the relevant URL to fetch in zerosuggest_provider.cc. Both the data and the backend are experimental at this point, so we don't want to check in a reference to the URL. There's a higher bar to meet to bring up a publicly available backend.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/autocomplete_match.h (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/autocomplete_match.h:92: SEARCH_ZEROSUGGEST_URL, // A URL suggested by zerosuggest. On 2012/08/14 19:20:02, Peter Kasting wrote: > SEARCH_ZEROSUGGEST_URL is a contradiction. > > It's not clear to me why you need new types anyway. SEARCH_SUGGEST and > NAVSUGGEST seem like existing types that would serve you fine. Done. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:42: const GURL& url, On 2012/08/14 19:20:02, Peter Kasting wrote: > Nit: Can fit on previous line, then indent next arg even Done. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:44: const std::string& spec = url.possibly_invalid_spec(); On 2012/08/14 19:20:02, Peter Kasting wrote: > Shouldn't |url| always be valid here? Seems like you can DCHECK is_valid() and > then just get the spec. It may be empty. But invalid URLs will not be worth looking up either. I've changed this to check is_valid before getting the spec. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/14 19:20:02, Peter Kasting wrote: > How can the spec be empty? If it can, then my comment about validity above is > wrong. It's empty on initial focus. Revised. > > Nit: {}, or move comment above conditional (3 places) Done. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:76: if (base::CodepageToUTF16(json_data, charset.c_str(), On 2012/08/14 19:20:02, Peter Kasting wrote: > Why do charset -> utf16 -> utf8 instead of just charset -> utf8? > > Will this even ever be non-UTF8 to begin with? I thought most Google services > spoke UTF8 exclusively. This code comes from SearchProvider. Our test service certainly uses UTF-8, so I've removed this for now. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:167: AutocompleteMatch match(this, kMaxRelevance, false, On 2012/08/14 19:20:02, Peter Kasting wrote: > The relevance scores you use here and below mean that if these zerosuggest > matches stick around as the user starts typing, they'll outrank everything from > every other provider. That seems wrong and will lead to a really bad UX (e.g. > you'll completely disable inline autocompletion). > > It seems like you should either have zerosuggest results disappear entirely once > the user starts typing, or be ranked very low so everything else outranks them, > or something. This placeholder nav suggestion will get dropped when the user starts typing. I changed zero suggestions to have really low relevance after the user starts typing (but not so low that they disappear) https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); On 2012/08/14 19:20:02, Peter Kasting wrote: > Nit: I think this change is safe, but it's not totally clear to me why you need > to make it. OmniboxView::CloseOmniboxPopup() is sometimes called when the omnibox popup isn't even open. OmniboxView doesn't seem to know whether the popup is really open, but it calls this method which does, so side-effects in this method need to be guarded by a test to IsOpen. Previously, stopping autocomplete in this case was a no-op, but with zero-suggest it would stop suggestions fetching immediately after starting (and then the popup never opens). https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:537: match.type == AutocompleteMatch::SEARCH_ZEROSUGGEST || On 2012/08/14 19:20:02, Peter Kasting wrote: > Nit: Don't look at the match type, look at match.provider.name. Done. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:704: // in the omnibox. Then we can just run zero-suggest as a normal provider. On 2012/08/14 19:20:02, Peter Kasting wrote: > Nit: It's not obvious to me what this comment means. When you say "decoupled > from typing", I'm thinking of the idea to remove the "what you typed" suggestion > from the dropdown. That won't have any effect on whether you need to make a > call here to start zerosuggest. Won't you still need a call here regardless? I > mean, ultimately if we want to show suggestions on focus, then we'll need to do > something to show suggestions when we get focus. I reworded this to say what I mean, does this makes sense? https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:717: autocomplete_controller_->StopZeroSuggest(); On 2012/08/14 19:20:02, Peter Kasting wrote: > This seems unnecessary if you revert your change to StopAutocomplete() above, > since then losing focus will call through to that and unconditionally Stop() all > the providers. Please see comment above.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/14 22:27:11, Jered wrote: > On 2012/08/14 19:20:02, Peter Kasting wrote: > > How can the spec be empty? If it can, then my comment about validity above is > > wrong. > It's empty on initial focus. Wow, really? That seems odd. The caller passes in PermanentURL(), right? Shouldn't that be chrome://newtab/ or something? Maybe it's passing in a display URL instead of the true underlying URL, which should always be non-empty. I'm not sure which is actually more correct. It depends whether you want "what the user sees" or "where the user is". Certainly I would imagine there are likely useful zerosuggestions if the user is currently on the new tab page -- but knowing what they are may not be a job for a server-side request, and may instead require querying the sorts of backends that give us the NTP thumbnails, recently closed tabs, etc. What is your plan regarding locally-sourced zerosuggestions? It this something you're looking at? https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); On 2012/08/14 22:27:11, Jered wrote: > On 2012/08/14 19:20:02, Peter Kasting wrote: > > Nit: I think this change is safe, but it's not totally clear to me why you > need > > to make it. > > OmniboxView::CloseOmniboxPopup() is sometimes called when the omnibox popup > isn't even open. Like when? Perhaps those calls are bugs and should be removed instead of changing the code here. > Previously, stopping autocomplete in this case was a no-op, but with > zero-suggest it would stop suggestions fetching immediately after starting (and > then the popup never opens). This is definitely a bad effect that we should fix, but I'm not sure whether the fix you've made is correct. Knowing the call chain that triggers this would be useful. https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:703: // having a special entry point for zero-suggest. Ah. That's what you meant. Hmm. I wonder if we really want to call Start() here or if we'll still want to preserve a distinction in the API. Oh well, that probably is not perfectly decidable yet, so this comment is fine. Or maybe you could add "We may want to" in front of the rest of the comment.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/15 05:30:39, Peter Kasting wrote: > On 2012/08/14 22:27:11, Jered wrote: > > On 2012/08/14 19:20:02, Peter Kasting wrote: > > > How can the spec be empty? If it can, then my comment about validity above > is > > > wrong. > > It's empty on initial focus. > > Wow, really? That seems odd. The caller passes in PermanentURL(), right? Yep. > Shouldn't that be chrome://newtab/ or something? Maybe it's passing in a > display URL instead of the true underlying URL, which should always be > non-empty. The value is permanent_text_ from the omnibox edit model, which says it is "the URL of the currently displayed page". It comes from ToolbarModel::GetText() which sets it to blank if ToolbarModel::ShouldDisplayURL() is false, which happens for extension pages and for WebUIs that set HideURL(). > I'm not sure which is actually more correct. It depends whether you > want "what the user sees" or "where the user is". We want the latter, where the user is. This seems to be PermanentURL() in all cases except those where the URL is set to blank. > Certainly I would imagine > there are likely useful zerosuggestions if the user is currently on the new tab > page -- but knowing what they are may not be a job for a server-side request, > and may instead require querying the sorts of backends that give us the NTP > thumbnails, recently closed tabs, etc. Yeah quite possibly. > > What is your plan regarding locally-sourced zerosuggestions? It this something > you're looking at? We haven't looked at this. We'd have to think carefully about how this would work given other ntp changes we're looking at. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); On 2012/08/15 05:30:39, Peter Kasting wrote: > On 2012/08/14 22:27:11, Jered wrote: > > On 2012/08/14 19:20:02, Peter Kasting wrote: > > > Nit: I think this change is safe, but it's not totally clear to me why you > > need > > > to make it. > > > > OmniboxView::CloseOmniboxPopup() is sometimes called when the omnibox popup > > isn't even open. > > Like when? Perhaps those calls are bugs and should be removed instead of > changing the code here. > > > Previously, stopping autocomplete in this case was a no-op, but with > > zero-suggest it would stop suggestions fetching immediately after starting > (and > > then the popup never opens). > > This is definitely a bad effect that we should fix, but I'm not sure whether the > fix you've made is correct. Knowing the call chain that triggers this would be > useful. It seems to happen all over the place in Mac UI code. I am not sure about other platforms, I could check gtk but don't have easy access to Windows for testing. But glancing at the code, I think the assumption made is that it's ok to close the popup if already closed and this method should not do anything if that's the case. Here are some call chains I observed: 1. When opening chrome, Browser::UpdateToolbar -> BrowserWindowCocoa::UpdateToolbar -> LocationBarViewMac::Update -> OmniboxViewMac::RevertAll -> OmniboxViewMac::CloseOmniboxPopup (twice). 2. On new tab, -[AutocompleteTextField windowDidResignKey:] -> OmniboxViewMac::ClosePopup(), then UpdateToolbar() twice. 3. On navigating, -[AutocompleteTextFieldEditor mouseDown:] -> OmniboxViewMac::ClosePopup(), then OmniboxViewMac::OnDidEndEditing() -> OmniboxViewMac::ClosePopup(), then chrome::Navigate() -> ... (twice), then Browser::UpdateToolbar -> ... (twice). https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:703: // having a special entry point for zero-suggest. On 2012/08/15 05:30:39, Peter Kasting wrote: > Ah. That's what you meant. > > Hmm. I wonder if we really want to call Start() here or if we'll still want to > preserve a distinction in the API. Oh well, that probably is not perfectly > decidable yet, so this comment is fine. Or maybe you could add "We may want to" > in front of the rest of the comment. Done.
On 2012/08/15 16:32:12, Jered wrote: > https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... > File chrome/browser/autocomplete/zero_suggest_provider.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... > chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || > url.scheme() != chrome::kHttpScheme) > On 2012/08/15 05:30:39, Peter Kasting wrote: > > On 2012/08/14 22:27:11, Jered wrote: > > > On 2012/08/14 19:20:02, Peter Kasting wrote: > > > > How can the spec be empty? If it can, then my comment about validity > above > > is > > > > wrong. > > > It's empty on initial focus. > > > > Wow, really? That seems odd. The caller passes in PermanentURL(), right? > Yep. > > > Shouldn't that be chrome://newtab/ or something? Maybe it's passing in a > > display URL instead of the true underlying URL, which should always be > > non-empty. > The value is permanent_text_ from the omnibox edit model, which says it is "the > URL of the currently displayed page". It comes from ToolbarModel::GetText() > which sets it to blank if ToolbarModel::ShouldDisplayURL() is false, which > happens for extension pages and for WebUIs that set HideURL(). > > > I'm not sure which is actually more correct. It depends whether you > > want "what the user sees" or "where the user is". > We want the latter, where the user is. This seems to be PermanentURL() in all > cases except those where the URL is set to blank. > > > Certainly I would imagine > > there are likely useful zerosuggestions if the user is currently on the new > tab > > page -- but knowing what they are may not be a job for a server-side request, > > and may instead require querying the sorts of backends that give us the NTP > > thumbnails, recently closed tabs, etc. > Yeah quite possibly. > > > > > What is your plan regarding locally-sourced zerosuggestions? It this > something > > you're looking at? > We haven't looked at this. We'd have to think carefully about how this would > work given other ntp changes we're looking at. > > https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... > File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... > chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: > autocomplete_controller_->Stop(true); > On 2012/08/15 05:30:39, Peter Kasting wrote: > > On 2012/08/14 22:27:11, Jered wrote: > > > On 2012/08/14 19:20:02, Peter Kasting wrote: > > > > Nit: I think this change is safe, but it's not totally clear to me why you > > > need > > > > to make it. > > > > > > OmniboxView::CloseOmniboxPopup() is sometimes called when the omnibox popup > > > isn't even open. > > > > Like when? Perhaps those calls are bugs and should be removed instead of > > changing the code here. > > > > > Previously, stopping autocomplete in this case was a no-op, but with > > > zero-suggest it would stop suggestions fetching immediately after starting > > (and > > > then the popup never opens). > > > > This is definitely a bad effect that we should fix, but I'm not sure whether > the > > fix you've made is correct. Knowing the call chain that triggers this would > be > > useful. > > It seems to happen all over the place in Mac UI code. I am not sure about other > platforms, I could check gtk but don't have easy access to Windows for testing. > But glancing at the code, I think the assumption made is that it's ok to close > the popup if already closed and this method should not do anything if that's the > case. Here are some call chains I observed: > > 1. When opening chrome, Browser::UpdateToolbar -> > BrowserWindowCocoa::UpdateToolbar -> LocationBarViewMac::Update -> > OmniboxViewMac::RevertAll -> OmniboxViewMac::CloseOmniboxPopup (twice). > 2. On new tab, -[AutocompleteTextField windowDidResignKey:] -> > OmniboxViewMac::ClosePopup(), then UpdateToolbar() twice. > 3. On navigating, -[AutocompleteTextFieldEditor mouseDown:] -> > OmniboxViewMac::ClosePopup(), then OmniboxViewMac::OnDidEndEditing() -> > OmniboxViewMac::ClosePopup(), then chrome::Navigate() -> ... (twice), then > Browser::UpdateToolbar -> ... (twice). I filed http://crbug.com/142887 for the weird mousedown behavior actually, that seems like a straight up bug. > > https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... > File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/4008/chrome/browser/ui/o... > chrome/browser/ui/omnibox/omnibox_edit_model.cc:703: // having a special entry > point for zero-suggest. > On 2012/08/15 05:30:39, Peter Kasting wrote: > > Ah. That's what you meant. > > > > Hmm. I wonder if we really want to call Start() here or if we'll still want > to > > preserve a distinction in the API. Oh well, that probably is not perfectly > > decidable yet, so this comment is fine. Or maybe you could add "We may want > to" > > in front of the rest of the comment. > > Done.
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/15 16:32:12, Jered wrote: > On 2012/08/15 05:30:39, Peter Kasting wrote: > > I'm not sure which is actually more correct. It depends whether you > > want "what the user sees" or "where the user is". > We want the latter, where the user is. This seems to be PermanentURL() in all > cases except those where the URL is set to blank. You're never at a blank URL, because that's not actually possible. (Even about:blank is not a blank URL.) The address of the New Tab Page is chrome://newtab/, not the empty string. PermanentURL() is seemingly giving you the display URL, not the true URL. Since you say you want the latter, you should fix the caller to provide that, then change the code here to DCHECK(is_valid()). > > What is your plan regarding locally-sourced zerosuggestions? It this > something > > you're looking at? > We haven't looked at this. We'd have to think carefully about how this would > work given other ntp changes we're looking at. Consider adding TODOs or NOTEs if there are places in this code where you'd want to look at locally-sourced suggestions, and/or filing a bug to cover. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); On 2012/08/15 16:32:12, Jered wrote: > On 2012/08/15 05:30:39, Peter Kasting wrote: > > On 2012/08/14 22:27:11, Jered wrote: > > > Previously, stopping autocomplete in this case was a no-op, but with > > > zero-suggest it would stop suggestions fetching immediately after starting > > (and > > > then the popup never opens). > > > > This is definitely a bad effect that we should fix, but I'm not sure whether > the > > fix you've made is correct. Knowing the call chain that triggers this would > be > > useful. > > It seems to happen all over the place in Mac UI code. The call chains you wrote about don't seem like they would cause a case like you describe above where we'd have zerosuggestions in progress which would get stopped. They seem like cases where the zerosuggest provider ought not to be running anyway. Am I wrong, or was there another case or cases that explicitly broke the zerosuggestion flow?
https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (spec.empty() || url.scheme() != chrome::kHttpScheme) On 2012/08/15 17:35:31, Peter Kasting wrote: > On 2012/08/15 16:32:12, Jered wrote: > > On 2012/08/15 05:30:39, Peter Kasting wrote: > > > I'm not sure which is actually more correct. It depends whether you > > > want "what the user sees" or "where the user is". > > We want the latter, where the user is. This seems to be PermanentURL() in all > > cases except those where the URL is set to blank. > > You're never at a blank URL, because that's not actually possible. (Even > about:blank is not a blank URL.) The address of the New Tab Page is > chrome://newtab/, not the empty string. > > PermanentURL() is seemingly giving you the display URL, not the true URL. Since > you say you want the latter, you should fix the caller to provide that, then > change the code here to DCHECK(is_valid()). Done. > > > > What is your plan regarding locally-sourced zerosuggestions? It this > > something > > > you're looking at? > > We haven't looked at this. We'd have to think carefully about how this would > > work given other ntp changes we're looking at. > > Consider adding TODOs or NOTEs if there are places in this code where you'd want > to look at locally-sourced suggestions, and/or filing a bug to cover. I added a TODO here. https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/2015/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:450: autocomplete_controller_->Stop(true); On 2012/08/15 17:35:31, Peter Kasting wrote: > On 2012/08/15 16:32:12, Jered wrote: > > On 2012/08/15 05:30:39, Peter Kasting wrote: > > > On 2012/08/14 22:27:11, Jered wrote: > > > > Previously, stopping autocomplete in this case was a no-op, but with > > > > zero-suggest it would stop suggestions fetching immediately after starting > > > (and > > > > then the popup never opens). > > > > > > This is definitely a bad effect that we should fix, but I'm not sure whether > > the > > > fix you've made is correct. Knowing the call chain that triggers this would > > be > > > useful. > > > > It seems to happen all over the place in Mac UI code. > > The call chains you wrote about don't seem like they would cause a case like you > describe above where we'd have zerosuggestions in progress which would get > stopped. They seem like cases where the zerosuggest provider ought not to be > running anyway. Am I wrong, or was there another case or cases that explicitly > broke the zerosuggestion flow? I think it was the Mac mousedown bug. We got focus, started, then mousedown happened next and stopped it. I reverted this change because the method is called "StopAutocomplete", it should actually stop it. I will look for a more specific fix.
https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (!url.is_valid() || url.scheme() != chrome::kHttpScheme) Nit: Remove "!url.is_valid() ||", since we should never handle DCHECK failure. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:155: // gets dropped as soon as the user types something. Nit: Since the zerosuggest provider is the only thing providing results in this case, you don't actually need 2000/1999 here and below. You could just have a single kMaxZeroSuggestRelevance = 100 atop this file, use that here, and decrement below (based on how many matches are already in the result set, see next comment). https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:189: (user_text_.empty() || user_text_is_url) ? 1999 : 100; The suggestions we add should not have the same relevance as each other, so that their sort order is consistent. This relevance score should decrease as you add more suggestions. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.h:108: // Current query as UTF-16. Nit: Don't keep both these members just to reduce the number of string conversions you do, because those conversions aren't in the inner loop of anything time-critical. Pick one -- probably the UTF8 version -- and convert elsewhere as needed. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:443: if (popup_->IsOpen()) { Nit: Revert remaining change to this function https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:537: (match.provider && match.provider->name() == "ZeroSuggest")) Nit: You don't need "match.provider &&", all matches must have a provider. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:703: // here rather than having a special entry point for zero-suggest. Nit: Might want: // Note that we avoid PermanentURL() here because it's not guaranteed to give us the actual underlying current URL, e.g. if we're on the NTP and the |permanent_text_| is empty.
https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:45: if (!url.is_valid() || url.scheme() != chrome::kHttpScheme) On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: Remove "!url.is_valid() ||", since we should never handle DCHECK failure. Done. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:155: // gets dropped as soon as the user types something. On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: Since the zerosuggest provider is the only thing providing results in this > case, you don't actually need 2000/1999 here and below. You could just have a > single kMaxZeroSuggestRelevance = 100 atop this file, use that here, and > decrement below (based on how many matches are already in the result set, see > next comment). It seems existing providers (e.g. SearchProvider) keep matches_ around so that they can reuse suggestions when there are minimal changes to omnibox text. With lower scores, I saw these stale suggestions. To work around that, I've changed StartZeroSuggest() to expire them immediately, but this doesn't seem to work as I'd expect. I'll have to dig into this more tomorrow, the code is a bit complex. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:189: (user_text_.empty() || user_text_is_url) ? 1999 : 100; On 2012/08/15 21:52:44, Peter Kasting wrote: > The suggestions we add should not have the same relevance as each other, so that > their sort order is consistent. This relevance score should decrease as you add > more suggestions. Done. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.h (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.h:108: // Current query as UTF-16. On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: Don't keep both these members just to reduce the number of string > conversions you do, because those conversions aren't in the inner loop of > anything time-critical. Pick one -- probably the UTF8 version -- and convert > elsewhere as needed. Done. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:443: if (popup_->IsOpen()) { On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: Revert remaining change to this function Done. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:537: (match.provider && match.provider->name() == "ZeroSuggest")) On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: You don't need "match.provider &&", all matches must have a provider. Done. https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:703: // here rather than having a special entry point for zero-suggest. On 2012/08/15 21:52:44, Peter Kasting wrote: > Nit: Might want: > > // Note that we avoid PermanentURL() here because it's not guaranteed to give us > the actual underlying current URL, e.g. if we're on the NTP and the > |permanent_text_| is empty. Done.
https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... chrome/browser/autocomplete/zero_suggest_provider.cc:155: // gets dropped as soon as the user types something. On 2012/08/15 23:13:00, Jered wrote: > On 2012/08/15 21:52:44, Peter Kasting wrote: > > Nit: Since the zerosuggest provider is the only thing providing results in > this > > case, you don't actually need 2000/1999 here and below. You could just have a > > single kMaxZeroSuggestRelevance = 100 atop this file, use that here, and > > decrement below (based on how many matches are already in the result set, see > > next comment). > > It seems existing providers (e.g. SearchProvider) keep matches_ around so that > they can reuse suggestions when there are minimal changes to omnibox text. With > lower scores, I saw these stale suggestions. To work around that, I've changed > StartZeroSuggest() to expire them immediately, but this doesn't seem to work as > I'd expect. I'll have to dig into this more tomorrow, the code is a bit complex. I'm not really sure what you're saying here. The "minimal_changes" flag means something very specific: the ctrl key state has toggled. If there's a change like "the user typed another character", that is not considered a "minimal" change in the sense that the code understands it. Maybe that's not what you meant. There is a separate caching effect in the SearchProvider specifically that has to do with results provided by the suggest server, and another in the AutocompleteController that is related to smoothing out changes while a user types rapidly. I don't think I understand the problem you're having to begin with, though.
On Aug 15, 2012 5:24 PM, <pkasting@chromium.org> wrote: > > > https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... > File chrome/browser/autocomplete/zero_suggest_provider.cc (right): > > https://chromiumcodereview.appspot.com/10832256/diff/5013/chrome/browser/auto... > chrome/browser/autocomplete/zero_suggest_provider.cc:155: // gets > dropped as soon as the user types something. > On 2012/08/15 23:13:00, Jered wrote: >> >> On 2012/08/15 21:52:44, Peter Kasting wrote: >> > Nit: Since the zerosuggest provider is the only thing providing > > results in >> >> this >> > case, you don't actually need 2000/1999 here and below. You could > > just have a >> >> > single kMaxZeroSuggestRelevance = 100 atop this file, use that here, > > and >> >> > decrement below (based on how many matches are already in the result > > set, see >> >> > next comment). > > >> It seems existing providers (e.g. SearchProvider) keep matches_ around > > so that >> >> they can reuse suggestions when there are minimal changes to omnibox > > text. With >> >> lower scores, I saw these stale suggestions. To work around that, I've > > changed >> >> StartZeroSuggest() to expire them immediately, but this doesn't seem > > to work as >> >> I'd expect. I'll have to dig into this more tomorrow, the code is a > > bit complex. > > I'm not really sure what you're saying here. The "minimal_changes" flag > means something very specific: the ctrl key state has toggled. If > there's a change like "the user typed another character", that is not > considered a "minimal" change in the sense that the code understands it. > Maybe that's not what you meant. > > There is a separate caching effect in the SearchProvider specifically > that has to do with results provided by the suggest server, and another > in the AutocompleteController that is related to smoothing out changes > while a user types rapidly. > > I don't think I understand the problem you're having to begin with, > though. Problem is I'm seeing suggestions from the last search on StartZeroSuggest because at the moment they are cleared out in Start. Most of these have scores above 100. I want to get rid of them so they don't show up with zero suggestions. > > https://chromiumcodereview.appspot.com/10832256/
On 2012/08/16 00:42:20, jered1 wrote: > Problem is I'm seeing suggestions from the last search on StartZeroSuggest > because at the moment they are cleared out in Start. Most of these have > scores above 100. I want to get rid of them so they don't show up with zero > suggestions. Ugh. This is an artifact of trying to wire through the AutocompleteController something that totally doesn't fit its processing pattern. I suggest not plumbing these results through the standard "provider updated" callbacks and instead having a custom zerosuggest-specific callback in the controller that just routes the zerosuggest provider's results directly to listeners, bypassing everything else the controller does.
On 2012/08/16 00:46:57, Peter Kasting wrote: > On 2012/08/16 00:42:20, jered1 wrote: > > Problem is I'm seeing suggestions from the last search on StartZeroSuggest > > because at the moment they are cleared out in Start. Most of these have > > scores above 100. I want to get rid of them so they don't show up with zero > > suggestions. > > Ugh. This is an artifact of trying to wire through the AutocompleteController > something that totally doesn't fit its processing pattern. > > I suggest not plumbing these results through the standard "provider updated" > callbacks and instead having a custom zerosuggest-specific callback in the > controller that just routes the zerosuggest provider's results directly to > listeners, bypassing everything else the controller does. I tried a couple options. Adding a new callback proved tricky because we want to keep zero suggestions around if the user begins to type them (even if they are really low scoring). How is this way? Seemed least invasive to me.
On 2012/08/16 17:25:57, Jered wrote: > On 2012/08/16 00:46:57, Peter Kasting wrote: > > On 2012/08/16 00:42:20, jered1 wrote: > > > Problem is I'm seeing suggestions from the last search on StartZeroSuggest > > > because at the moment they are cleared out in Start. Most of these have > > > scores above 100. I want to get rid of them so they don't show up with zero > > > suggestions. > > > > Ugh. This is an artifact of trying to wire through the AutocompleteController > > something that totally doesn't fit its processing pattern. > > > > I suggest not plumbing these results through the standard "provider updated" > > callbacks and instead having a custom zerosuggest-specific callback in the > > controller that just routes the zerosuggest provider's results directly to > > listeners, bypassing everything else the controller does. > > I tried a couple options. Adding a new callback proved tricky because we want to > keep zero suggestions around if the user begins to type them (even if they are > really low scoring). How is this way? Seemed least invasive to me. (Sorry, I messed up my git repo somehow and ended up rebasing as part of my struggle to fix it, so this patch includes the rebase.)
On 2012/08/16 17:28:52, Jered wrote: > On 2012/08/16 17:25:57, Jered wrote: > > On 2012/08/16 00:46:57, Peter Kasting wrote: > > > On 2012/08/16 00:42:20, jered1 wrote: > > > > Problem is I'm seeing suggestions from the last search on StartZeroSuggest > > > > because at the moment they are cleared out in Start. Most of these have > > > > scores above 100. I want to get rid of them so they don't show up with > zero > > > > suggestions. > > > > > > Ugh. This is an artifact of trying to wire through the > AutocompleteController > > > something that totally doesn't fit its processing pattern. > > > > > > I suggest not plumbing these results through the standard "provider updated" > > > callbacks and instead having a custom zerosuggest-specific callback in the > > > controller that just routes the zerosuggest provider's results directly to > > > listeners, bypassing everything else the controller does. > > > > I tried a couple options. Adding a new callback proved tricky because we want > to > > keep zero suggestions around if the user begins to type them (even if they are > > really low scoring). How is this way? Seemed least invasive to me. > > (Sorry, I messed up my git repo somehow and ended up rebasing as part of > my struggle to fix it, so this patch includes the rebase.) (sreeram@ helped me fix this so the last patch set is a clean diff again.)
pkasting@ friendly ping. Hoping to merge this before I disappear into the desert for a few weeks... :-) On 2012/08/17 15:14:23, Jered wrote: > On 2012/08/16 17:28:52, Jered wrote: > > On 2012/08/16 17:25:57, Jered wrote: > > > On 2012/08/16 00:46:57, Peter Kasting wrote: > > > > On 2012/08/16 00:42:20, jered1 wrote: > > > > > Problem is I'm seeing suggestions from the last search on > StartZeroSuggest > > > > > because at the moment they are cleared out in Start. Most of these have > > > > > scores above 100. I want to get rid of them so they don't show up with > > zero > > > > > suggestions. > > > > > > > > Ugh. This is an artifact of trying to wire through the > > AutocompleteController > > > > something that totally doesn't fit its processing pattern. > > > > > > > > I suggest not plumbing these results through the standard "provider > updated" > > > > callbacks and instead having a custom zerosuggest-specific callback in the > > > > controller that just routes the zerosuggest provider's results directly to > > > > listeners, bypassing everything else the controller does. > > > > > > I tried a couple options. Adding a new callback proved tricky because we > want > > to > > > keep zero suggestions around if the user begins to type them (even if they > are > > > really low scoring). How is this way? Seemed least invasive to me. > > > > (Sorry, I messed up my git repo somehow and ended up rebasing as part of > > my struggle to fix it, so this patch includes the rebase.) > > (sreeram@ helped me fix this so the last patch set is a clean diff again.)
Hi Peter, FYI, Jered is out for a couple of weeks, so I'm going to take this change over. Continuing review at https://chromiumcodereview.appspot.com/10877021. Thanks, Samarth On 2012/08/20 17:16:35, Jered wrote: > pkasting@ friendly ping. Hoping to merge this before I disappear into the desert > for a few weeks... :-) > > On 2012/08/17 15:14:23, Jered wrote: > > On 2012/08/16 17:28:52, Jered wrote: > > > On 2012/08/16 17:25:57, Jered wrote: > > > > On 2012/08/16 00:46:57, Peter Kasting wrote: > > > > > On 2012/08/16 00:42:20, jered1 wrote: > > > > > > Problem is I'm seeing suggestions from the last search on > > StartZeroSuggest > > > > > > because at the moment they are cleared out in Start. Most of these > have > > > > > > scores above 100. I want to get rid of them so they don't show up with > > > zero > > > > > > suggestions. > > > > > > > > > > Ugh. This is an artifact of trying to wire through the > > > AutocompleteController > > > > > something that totally doesn't fit its processing pattern. > > > > > > > > > > I suggest not plumbing these results through the standard "provider > > updated" > > > > > callbacks and instead having a custom zerosuggest-specific callback in > the > > > > > controller that just routes the zerosuggest provider's results directly > to > > > > > listeners, bypassing everything else the controller does. > > > > > > > > I tried a couple options. Adding a new callback proved tricky because we > > want > > > to > > > > keep zero suggestions around if the user begins to type them (even if they > > are > > > > really low scoring). How is this way? Seemed least invasive to me. > > > > > > (Sorry, I messed up my git repo somehow and ended up rebasing as part of > > > my struggle to fix it, so this patch includes the rebase.) > > > > (sreeram@ helped me fix this so the last patch set is a clean diff again.) |
