Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13141002:
Use Instant suggested match type for Instant temporary text. (Closed)
|
Created:
7 years, 9 months ago by sreeram Modified:
7 years, 7 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, gideonwald, dominich, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, beaudoin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse Instant suggested match type for Instant temporary text.
BUG=224522, 173414
R=mpearson@chromium.org, pkasting@chromium.org
TEST=See bug.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196989
Patch Set 1 #
Total comments: 1
Patch Set 2 : TODO #
Total comments: 7
Patch Set 3 : . #Patch Set 4 : const string16& #
Total comments: 14
Patch Set 5 : |relevance|, comments #Patch Set 6 : TODO #
Total comments: 10
Patch Set 7 : SuggestExactInput #Patch Set 8 : static #
Total comments: 3
Patch Set 9 : Remove GetProviders() #
Total comments: 2
Patch Set 10 : No DCHECK #Patch Set 11 : Expanded comment #Patch Set 12 : Rebase #Patch Set 13 : Fix test #
Messages
Total messages: 32 (0 generated)
Please review.
https://codereview.chromium.org/13141002/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:555: match.transition = content::PAGE_TRANSITION_GENERATED; It's not at all clear to me why this fixes the problem you're having. The comment here needs to be filled out to describe precisely what this is doing and why it should be here, as well as why having Instant set the transition type correctly itself doesn't make sense, and how the proper transition type is going to be set after this. Also, don't refer to bugs in comments. The comment should tell the reader everything they need to know.
On 2013/03/28 20:35:00, Peter Kasting wrote: > It's not at all clear to me why this fixes the problem you're having. Actually, my first patch was inadequate (it didn't solve the problem in all cases). Please see patchset #2. It's not fully done, and needs your feedback. +mpearson for his opinion too. https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1226: // TODO XXX Here, I want to return either a UWYT match or a SWYT match (using the current view_->GetText()), depending on |is_instant_temporary_text_a_search_query_|. Currently, the portion of search_provider.cc that adds the SWYT match is tightly intertwined with the rest of the code. HistoryURLProvider::SuggestExactInput() is more easy to re-use (given it's already re-used for the Ctrl-Enter case). What do you recommend I do here? 1. Pull out the necessary parts to enable reuse? 2. Something else (perhaps an entirely different approach altogether)?
https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1226: // TODO XXX On 2013/04/08 16:07:28, sreeram wrote: > Here, I want to return either a UWYT match or a SWYT match (using the current > view_->GetText()), depending on |is_instant_temporary_text_a_search_query_|. So to be clear, the reason that running through the Classify() case won't work here is because you have text that looks like one thing but should be treated like the other (e.g. a search for "facebook.com")? If so, I would say factoring the SearchProvider enough to allow for creating a SWYT match directly is fine. https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:521: bool is_instant_temporary_text_a_search_query_; We now have several bools about what the text in the omnibox actually is. Consider changing at least some of these to an enum, e.g. enum OmniboxTextType { PERMANENT_TEXT, USER_TEXT, NON_INSTANT_TEMPORARY_TEXT, INSTANT_TEMPORARY_URL, INSTANT_TEMPORARY_SEARCH }
https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1226: // TODO XXX On 2013/04/08 20:13:50, Peter Kasting wrote: > So to be clear, the reason that running through the Classify() case won't work > here is because you have text that looks like one thing but should be treated > like the other (e.g. a search for "facebook.com")? Exactly. > If so, I would say factoring the SearchProvider enough to allow for creating a > SWYT match directly is fine. Thanks. I'll do that and ping this thread when the patch is ready. https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:521: bool is_instant_temporary_text_a_search_query_; On 2013/04/08 20:13:50, Peter Kasting wrote: > We now have several bools about what the text in the omnibox actually is. > Consider changing at least some of these to an enum, e.g. > > enum OmniboxTextType { > PERMANENT_TEXT, > USER_TEXT, > NON_INSTANT_TEMPORARY_TEXT, > INSTANT_TEMPORARY_URL, > INSTANT_TEMPORARY_SEARCH > } Can I punt on this? @beaudoin's refactor should take care of this. Even if the refactor calls for a large redesign, we can do parts of it (such as collapsing related state variables into one) incrementally, quickly.
https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1226: // TODO XXX On 2013/04/08 20:20:48, sreeram wrote: > On 2013/04/08 20:13:50, Peter Kasting wrote: > > So to be clear, the reason that running through the Classify() case won't work > > here is because you have text that looks like one thing but should be treated > > like the other (e.g. a search for "facebook.com")? > > Exactly. K, make sure to make that clear in the comments here. https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): https://codereview.chromium.org/13141002/diff/8001/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:521: bool is_instant_temporary_text_a_search_query_; On 2013/04/08 20:20:48, sreeram wrote: > On 2013/04/08 20:13:50, Peter Kasting wrote: > > We now have several bools about what the text in the omnibox actually is. > > Consider changing at least some of these to an enum > > Can I punt on this? Sure.
@pkasting: Please review patchset #3.
https://codereview.chromium.org/13141002/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:80: bool GetSearchWhatYouTypedMatch(const string16& query, I find this somewhat misleading. The match you're returning is not completely populated correctly for use. For instance, relevance score is not initialized. You should either set it or revise this comment. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:256: if (search_provider) I know this shouldn't happen in normal use. Nevertheless, I'm curious: why did you bother reminding this guard? https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: text, text, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, match); Are there cases when the match you want to provide isn't identical to the input string? For instance, normal usage strips the ? from the beginning of omnibox inputs. Do you need to do that here too? https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: text, text, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, match); Isn't using TemplateURLRef::NO_SUGGESTIONS_AVAILABLE a lie? Can't there be suggestions available here? https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1246: AutocompleteInput input(text, string16::npos, string16(), GURL(), false, what's wrong with passing the actual current URL here? https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1246: AutocompleteInput input(text, string16::npos, string16(), GURL(), false, I'm pretty sure it's not used, but I think you might as well set prevent_inline_autocomplete to true. It seems more appropriate. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1249: autocomplete_controller_->history_url_provider(), input, false); Is it correct to always trim the http? Maybe it should depend on what the input looks like?
Addressed the following comments in patchset #5. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:80: bool GetSearchWhatYouTypedMatch(const string16& query, On 2013/04/09 23:59:11, Mark P wrote: > I find this somewhat misleading. The match you're returning is not completely > populated correctly for use. For instance, relevance score is not initialized. > You should either set it or revise this comment. Done. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:256: if (search_provider) On 2013/04/09 23:59:11, Mark P wrote: > I know this shouldn't happen in normal use. Nevertheless, I'm curious: why did > you bother reminding this guard? It shouldn't happen at all, whether in normal use or in unit tests, so that's why I removed it. When I added the code in GetInfoForCurrentText() to call search_provider()->GetSearchWhatYouTypedMatch(), I knew I didn't need the guard, so I went looking to see where else we had such a guard that isn't necessary. In contrast, the guard _is_ needed in FinalizeInstantQuery(), as the comment there says, because of unit tests. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: text, text, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, match); On 2013/04/09 23:59:11, Mark P wrote: > Are there cases when the match you want to provide isn't identical to the input > string? For instance, normal usage strips the ? from the beginning of omnibox > inputs. Do you need to do that here too? Not needed. Even if you type a query starting with "?", and you arrow up/down, the Instant suggested text (i.e., |text| here) won't have a "?". https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1246: AutocompleteInput input(text, string16::npos, string16(), GURL(), false, On 2013/04/09 23:59:11, Mark P wrote: > what's wrong with passing the actual current URL here? Nothing wrong. I've added a comment. Please see if that is satisfactory. If not, I'll add the current URL. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1246: AutocompleteInput input(text, string16::npos, string16(), GURL(), false, On 2013/04/09 23:59:11, Mark P wrote: > I'm pretty sure it's not used, but I think you might as well set > prevent_inline_autocomplete to true. It seems more appropriate. As above, I've added a comment as to why I'm mostly passing in "default" values. Passing "true" makes it look as though it's required for a non-obvious reason, so let me know if the comment isn't sufficient. https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1249: autocomplete_controller_->history_url_provider(), input, false); On 2013/04/09 23:59:11, Mark P wrote: > Is it correct to always trim the http? Maybe it should depend on what the input > looks like? The false indicates to NOT trim the http. The trimming only affects the "contents", i.e., what would be displayed if this match were to be displayed in a dropdown, for example. I've added a comment explaining that it actually won't be used for such purposes. Let me know if that doesn't work for you.
https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/21001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1244: text, text, TemplateURLRef::NO_SUGGESTIONS_AVAILABLE, false, match); On 2013/04/09 23:59:11, Mark P wrote: > Isn't using TemplateURLRef::NO_SUGGESTIONS_AVAILABLE a lie? Can't there be > suggestions available here? You're right. Good catch. When we are using the local fallback Instant overlay (i.e., not the Google-provided overlay), the SearchProvider will retrieve suggestions (http://crbug.com/225245) and therefore we should set this correctly. Working on it...
On 2013/04/11 20:48:57, sreeram wrote: > When we are using the local fallback Instant overlay (i.e., not the > Google-provided overlay), the SearchProvider will retrieve suggestions > (http://crbug.com/225245) and therefore we should set this correctly. Working on > it... Hmph. This turns out to be a big undertaking. We need the local fallback overlay to send us the index of the entry that was chosen, where it currently only sends us the text. I've put a TODO here, and filed a bug at high priority to get this fixed (since the rollout plan for Instant Extended calls for the local stuff to be launched first). Is that okay for now? http://crbug.com/230661
On 2013/04/11 23:19:31, sreeram wrote: > On 2013/04/11 20:48:57, sreeram wrote: > > When we are using the local fallback Instant overlay (i.e., not the > > Google-provided overlay), the SearchProvider will retrieve suggestions > > (http://crbug.com/225245) and therefore we should set this correctly. Working > on > > it... > > Hmph. This turns out to be a big undertaking. We need the local fallback overlay > to send us the index of the entry that was chosen, where it currently only sends > us the text. I've put a TODO here, and filed a bug at high priority to get this > fixed (since the rollout plan for Instant Extended calls for the local stuff to > be launched first). Is that okay for now? http://crbug.com/230661 Can you please file a separate bug, marked as blocking on the one you just filed, that's assigned to you and specifically mentioned what you must change here once the setValue bug is fixed? thanks, mark
On 2013/04/12 00:00:47, Mark P wrote: > Can you please file a separate bug, marked as blocking on the one you just > filed, that's assigned to you and specifically mentioned what you must change > here once the setValue bug is fixed? Done: http://crbug.com/230683
lgtm but please wait for Peter to circle back before submitting.
On 2013/04/12 00:26:39, Mark P wrote: > lgtm > > but please wait for Peter to circle back before submitting. Thanks. Peter, PTAL when you have some time.
The question about being static below is the really critical one. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:230: match->keyword = is_keyword ? providers_.keyword_provider() : Nit: Wrap this like the old code (break after '?' and indent 4), since this indenting doesn't really match anything in the style guide. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1382: match.type = type; Can we eliminate this, or the fill_into_edit mucking below, by moving code into your new function? For example, you could perhaps pass in the desired type as an argument. The function could maybe access |input_| if doing so makes sense (see concerns in the header), and thus take care of all the rest of the fill_into_edit stuff below. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:80: bool GetSearchWhatYouTypedMatch(const string16& query, I wonder if this should be static. Is it correct for it to access members like the current DSP and keyword provider, or might that need to be recalculated at the time of the call in case the call happens not in the same callstack as those are otherwise accessed? Similar concerns apply to if the function were to access things like |input_|. Nit: I think it'd be slightly nicer (and more consistent with the HUP) to return an AutocompleteMatch by value here. You can return the empty match if there's no DSP, which will allow callers to check for failure by asking if the destination_url is_valid(). Along those consistency lines, it might make sense to call this SuggestExactInput(), which also is clearer in the keyword case (because we don't usually use the phrase "what you typed match" with keywords). OTOH it might be more confusing due to the word "Suggest". I don't have a strong opinion. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1253: // Only the destination_url of the match will be used (to navigate to the Nit: Factor something like this comment (but that accounts for the difference in the block above) to above the conditional instead of mostly repeating it in both arms.
PTAL. Addressed the following comments in patchset #7. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:230: match->keyword = is_keyword ? providers_.keyword_provider() : On 2013/04/12 00:29:01, Peter Kasting wrote: > Nit: Wrap this like the old code (break after '?' and indent 4), since this > indenting doesn't really match anything in the style guide. Done. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1382: match.type = type; On 2013/04/12 00:29:01, Peter Kasting wrote: > Can we eliminate this, or the fill_into_edit mucking below, by moving code into > your new function? For example, you could perhaps pass in the desired type as > an argument. The function could maybe access |input_| if doing so makes sense > (see concerns in the header), and thus take care of all the rest of the > fill_into_edit stuff below. I don't think it makes sense for "SuggestExactInput" to be doing things that clearly are because of non-exact input, so I've left the type and fill_into_edit mucking as is. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:80: bool GetSearchWhatYouTypedMatch(const string16& query, On 2013/04/12 00:29:01, Peter Kasting wrote: > I wonder if this should be static. Is it correct for it to access members like > the current DSP and keyword provider, or might that need to be recalculated at > the time of the call in case the call happens not in the same callstack as those > are otherwise accessed? Similar concerns apply to if the function were to > access things like |input_|. The only new caller of this method is the one in omnibox_edit_model.cc. There, this is only called if we have temporary text. It's impossible to have temporary text when the user isn't actively editing. In particular, if the user switches tabs or unfocuses the omnibox, the temporary text will become user text. So the user can't change to another tab and switch the DSP from underneath us. (Should I worry about group policy updates? Or the TemplateURLService completing its load?) The name of the method itself doesn't imply this guarantee, of course. But then again, if you randomly call any of the other SearchProvider methods after changing the default search provider, I suspect they will also break (e.g.: FinalizeInstantQuery). So, is it okay to leave this as non-static? > Nit: I think it'd be slightly nicer (and more consistent with the HUP) to return > an AutocompleteMatch by value here. You can return the empty match if there's > no DSP, which will allow callers to check for failure by asking if the > destination_url is_valid(). Done. > Along those consistency lines, it might make sense to call this > SuggestExactInput(), which also is clearer in the keyword case (because we don't > usually use the phrase "what you typed match" with keywords). OTOH it might be > more confusing due to the word "Suggest". I don't have a strong opinion. Done. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1253: // Only the destination_url of the match will be used (to navigate to the On 2013/04/12 00:29:01, Peter Kasting wrote: > Nit: Factor something like this comment (but that accounts for the difference in > the block above) to above the conditional instead of mostly repeating it in both > arms. Done.
https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1382: match.type = type; On 2013/04/12 22:36:18, sreeram wrote: > On 2013/04/12 00:29:01, Peter Kasting wrote: > > Can we eliminate this, or the fill_into_edit mucking below, by moving code > into > > your new function? For example, you could perhaps pass in the desired type as > > an argument. The function could maybe access |input_| if doing so makes sense > > (see concerns in the header), and thus take care of all the rest of the > > fill_into_edit stuff below. > > I don't think it makes sense for "SuggestExactInput" to be doing things that > clearly are because of non-exact input, so I've left the type and fill_into_edit > mucking as is. Hmm. Maybe I shouldn't have been so hasty to suggest calling this SuggestExactInput(). Perhaps we should call it CreateSearchSuggestion and generalize it slightly. Even with exact input, some of the mucking makes sense -- prepending a '?' for FORCED_QUERY or the keyword name in keyword mode (the latter of which the function already does). I'm mostly just trying to figure out what would result in the least duplication of code/effort. I don't have a violent opinion. https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/13141002/diff/43001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:80: bool GetSearchWhatYouTypedMatch(const string16& query, On 2013/04/12 22:36:18, sreeram wrote: > On 2013/04/12 00:29:01, Peter Kasting wrote: > > I wonder if this should be static. Is it correct for it to access members > like > > the current DSP and keyword provider, or might that need to be recalculated at > > the time of the call in case the call happens not in the same callstack as > those > > are otherwise accessed? Similar concerns apply to if the function were to > > access things like |input_|. > > The only new caller of this method is the one in omnibox_edit_model.cc. There, > this is only called if we have temporary text. It's impossible to have temporary > text when the user isn't actively editing. In particular, if the user switches > tabs or unfocuses the omnibox, the temporary text will become user text. So the > user can't change to another tab and switch the DSP from underneath us. (Should > I worry about group policy updates? Or the TemplateURLService completing its > load?) You do have to worry about pointers changing any time there's asynchronicity, but looking more closely this code doesn't do anything like that. > So, is it okay to leave this as non-static? I'm kind of torn. On the one hand, it probably won't hurt anything. On the other hand, it's a little bit weird to be depending on existing state. This is the first function the SearchProvider has added that's intended to be called by someone other than the AutocompleteController. The HUP's SuggestExactInput() is static. I think I would be more comfortable with this being static. It just seems easier for the caller to reason about and clearer that this is something of a "public utility function, namespaced to the SearchProvider class".
Peter, PTAL. Changed the method to be static.
https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:174: match.keyword = is_keyword ? The only reason this function needs |search_providers| is to choose the appropriate keyword. If instead you pass in the keyword, you can avoid taking a Providers object. To calculate this keyword, the OmniboxEditModel can just get the DSP's keyword directly. This allows you to avoid exposing GetProviders(), which in turn allows you to basically revert all the Providers-related changes in this file (and the header). Sample code to get the DSP's keyword on the OmniboxEditModel side: const TemplateURL* default_provider = TemplateURLServiceFactory::GetForProfile(profile_)-> GetDefaultSearchProvider(); if (default_provider && default_provider->SupportsReplacement()) { ... = CreateSearchSuggestion(..., default_provider->keyword(), ...); ... } else { // Can't create a new search match. Probably in this case you should just // use an AutocompleteMatch with an empty |destination_url| so it can't be // navigated; this should realistically never happen anyway. }
https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:174: match.keyword = is_keyword ? On 2013/04/27 01:21:21, Peter Kasting wrote: > The only reason this function needs |search_providers| is to choose the > appropriate keyword. If instead you pass in the keyword, you can avoid taking a > Providers object. > > To calculate this keyword, the OmniboxEditModel can just get the DSP's keyword > directly. This allows you to avoid exposing GetProviders(), which in turn > allows you to basically revert all the Providers-related changes in this file > (and the header). > > Sample code to get the DSP's keyword on the OmniboxEditModel side: > > const TemplateURL* default_provider = > TemplateURLServiceFactory::GetForProfile(profile_)-> > GetDefaultSearchProvider(); > if (default_provider && default_provider->SupportsReplacement()) { > ... = CreateSearchSuggestion(..., default_provider->keyword(), ...); > ... > } else { > // Can't create a new search match. Probably in this case you should just > // use an AutocompleteMatch with an empty |destination_url| so it can't be > // navigated; this should realistically never happen anyway. > } But I do want to be able to support keyword mode. @dougw is working on supporting keyword mode for Instant Extended, and I want to make it easy for him when he gets to this part. Wouldn't he then have to redo some of the logic in GetProviders() to figure out the keyword provider?
https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/13141002/diff/62001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:174: match.keyword = is_keyword ? On 2013/04/27 01:25:54, sreeram wrote: > But I do want to be able to support keyword mode. @dougw is working on > supporting keyword mode for Instant Extended, and I want to make it easy for him > when he gets to this part. Wouldn't he then have to redo some of the logic in > GetProviders() to figure out the keyword provider? I would say, let's worry about that when we get there. It's not 100% clear to me that that will end up needing to run through this particular case in the omnibox code for keyword searches. If it does, the right answer will probably be simplified version of GetProviders() that only returns a string16 that's the keyword for the correct provider. The full Providers struct, the idea of ensuring the default and keyword providers aren't the same, etc. is all needed only by the SearchProvider internals and doesn't ever need to be exposed publicly.
Okay, done. PTAL.
LGTM with one caveat https://codereview.chromium.org/13141002/diff/66002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/66002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1280: DCHECK(default_provider->SupportsReplacement()); I'm nervous about these two DCHECKs. I think they can fail if something (e.g. policy) clears your DSP in the midst of you interacting with instant. I think instead of DCHECKing these you should do something like what my last comment suggested, e.g. pass an empty keyword string to CreateSearchSuggestion() and let it return you a no-destination match. This will look weird but it's better than crashing somewhere. (Add a comment about why we have this weird edge case.)
https://codereview.chromium.org/13141002/diff/66002/chrome/browser/ui/omnibox... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/13141002/diff/66002/chrome/browser/ui/omnibox... chrome/browser/ui/omnibox/omnibox_edit_model.cc:1280: DCHECK(default_provider->SupportsReplacement()); On 2013/04/27 02:15:26, Peter Kasting wrote: > I'm nervous about these two DCHECKs. I think they can fail if something (e.g. > policy) clears your DSP in the midst of you interacting with instant. > > I think instead of DCHECKing these you should do something like what my last > comment suggested, e.g. pass an empty keyword string to CreateSearchSuggestion() > and let it return you a no-destination match. This will look weird but it's > better than crashing somewhere. > > (Add a comment about why we have this weird edge case.) Done. I had added the DCHECK because Instant now listens for DSP changes and resets state, so we ought not to hit the case you're worried about, but anyway.
On 2013/04/27 02:21:56, sreeram wrote: > Done. I had added the DCHECK because Instant now listens for DSP changes and > resets state, so we ought not to hit the case you're worried about, but anyway. Oh. If it should truly be impossible to hit this, then by all means stick with the DCHECKs. I'm a big fan of using DCHECKs in place of actual handlers when they truly represent assertions that should never be violated.
On 2013/04/27 02:37:45, Peter Kasting wrote: > On 2013/04/27 02:21:56, sreeram wrote: > > Done. I had added the DCHECK because Instant now listens for DSP changes and > > resets state, so we ought not to hit the case you're worried about, but > anyway. > > Oh. If it should truly be impossible to hit this, then by all means stick with > the DCHECKs. I'm a big fan of using DCHECKs in place of actual handlers when > they truly represent assertions that should never be violated. It's okay. I'll leave it as is (without the DCHECK), mainly so that if something changes in Instant later (that stops listening to DSP changes or whatever), we don't want this code crashing.
On 2013/04/27 02:44:21, sreeram wrote: > On 2013/04/27 02:37:45, Peter Kasting wrote: > > On 2013/04/27 02:21:56, sreeram wrote: > > > Done. I had added the DCHECK because Instant now listens for DSP changes and > > > resets state, so we ought not to hit the case you're worried about, but > > anyway. > > > > Oh. If it should truly be impossible to hit this, then by all means stick > with > > the DCHECKs. I'm a big fan of using DCHECKs in place of actual handlers when > > they truly represent assertions that should never be violated. > > It's okay. I'll leave it as is (without the DCHECK), mainly so that if something > changes in Instant later (that stops listening to DSP changes or whatever), we > don't want this code crashing. :/ I'm not a huge fan of that, but if you really want to do it, at least add a comment noting that this should never happen now, and why you're leaving it.
On 2013/04/27 02:51:28, Peter Kasting wrote: > :/ I'm not a huge fan of that, but if you really want to do it, at least add a > comment noting that this should never happen now, and why you're leaving it. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sreeram@chromium.org/13141002/61007
Failed to apply patch for chrome/browser/autocomplete/search_provider.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/autocomplete/search_provider.cc
Hunk #2 FAILED at 1419.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/autocomplete/search_provider.cc.rej
Patch: chrome/browser/autocomplete/search_provider.cc
Index: chrome/browser/autocomplete/search_provider.cc
diff --git a/chrome/browser/autocomplete/search_provider.cc
b/chrome/browser/autocomplete/search_provider.cc
index
be43ae17d8b4fdff159ffe2ad5e616802da43d68..d8af4a5ef9b543c377971294388a0d1d75a79613
100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -124,6 +124,99 @@ const int SearchProvider::kKeywordProviderURLFetcherID = 2;
// static
int SearchProvider::kMinimumTimeBetweenSuggestQueriesMs = 100;
+// static
+AutocompleteMatch SearchProvider::CreateSearchSuggestion(
+ Profile* profile,
+ AutocompleteProvider* autocomplete_provider,
+ const AutocompleteInput& input,
+ const string16& query_string,
+ const string16& input_text,
+ int relevance,
+ AutocompleteMatch::Type type,
+ int accepted_suggestion,
+ bool is_keyword,
+ const string16& keyword) {
+ AutocompleteMatch match(autocomplete_provider, relevance, false, type);
+
+ // Bail out now if we don't actually have a valid provider.
+ match.keyword = keyword;
+ const TemplateURL* provider_url = match.GetTemplateURL(profile, false);
+ if (provider_url == NULL)
+ return match;
+
+ match.contents.assign(query_string);
+ // We do intra-string highlighting for suggestions - the suggested segment
+ // will be highlighted, e.g. for input_text = "you" the suggestion may be
+ // "youtube", so we'll bold the "tube" section: you*tube*.
+ if (input_text != query_string) {
+ size_t input_position = match.contents.find(input_text);
+ if (input_position == string16::npos) {
+ // The input text is not a substring of the query string, e.g. input
+ // text is "slasdot" and the query string is "slashdot", so we bold the
+ // whole thing.
+ match.contents_class.push_back(
+ ACMatchClassification(0, ACMatchClassification::MATCH));
+ } else {
+ // TODO(beng): ACMatchClassification::MATCH now seems to just mean
+ // "bold" this. Consider modifying the terminology.
+ // We don't iterate over the string here annotating all matches because
+ // it looks odd to have every occurrence of a substring that may be as
+ // short as a single character highlighted in a query suggestion result,
+ // e.g. for input text "s" and query string "southwest airlines", it
+ // looks odd if both the first and last s are highlighted.
+ if (input_position != 0) {
+ match.contents_class.push_back(
+ ACMatchClassification(0, ACMatchClassification::NONE));
+ }
+ match.contents_class.push_back(
+ ACMatchClassification(input_position, ACMatchClassification::DIM));
+ size_t next_fragment_position = input_position + input_text.length();
+ if (next_fragment_position < query_string.length()) {
+ match.contents_class.push_back(
+ ACMatchClassification(next_fragment_position,
+ ACMatchClassification::NONE));
+ }
+ }
+ } else {
+ // Otherwise, we're dealing with the "default search" result which has no
+ // completion.
+ match.contents_class.push_back(
+ ACMatchClassification(0, ACMatchClassification::NONE));
+ }
+
+ // When the user forced a query, we need to make sure all the fill_into_edit
+ // values preserve that property. Otherwise, if the user starts editing a
+ // suggestion, non-Search results will suddenly appear.
+ if (input.type() == AutocompleteInput::FORCED_QUERY)
+ match.fill_into_edit.assign(ASCIIToUTF16("?"));
+ if (is_keyword)
+ match.fill_into_edit.append(match.keyword + char16(' '));
+ if (!input.prevent_inline_autocomplete() &&
+ StartsWith(query_string, input_text, false)) {
+ match.inline_autocomplete_offset =
+ match.fill_into_edit.length() + input_text.length();
+ }
+ match.fill_into_edit.append(query_string);
+
+ const TemplateURLRef& search_url = provider_url->url_ref();
+ DCHECK(search_url.SupportsReplacement());
+ match.search_terms_args.reset(
+ new TemplateURLRef::SearchTermsArgs(query_string));
+ match.search_terms_args->original_query = input_text;
+ match.search_terms_args->accepted_suggestion = accepted_suggestion;
+ // This is the destination URL sans assisted query stats. This must be set
+ // so the AutocompleteController can properly de-dupe; the controller will
+ // eventually overwrite it before it reaches the user.
+ match.destination_url =
+ GURL(search_url.ReplaceSearchTerms(*match.search_terms_args.get()));
+
+ // Search results don't look like URLs.
+ match.transition = is_keyword ?
+ content::PAGE_TRANSITION_KEYWORD : content::PAGE_TRANSITION_GENERATED;
+
+ return match;
+}
+
SearchProvider::SearchProvider(AutocompleteProviderListener* listener,
Profile* profile)
: AutocompleteProvider(listener, profile,
@@ -1326,85 +1419,14 @@ void SearchProvider::AddMatchToMap(const string16&
query_string,
int accepted_suggestion,
bool is_keyword,
MatchMap* map) {
- AutocompleteMatch match(this, relevance, false, type);
- std::vector<size_t> content_param_offsets;
- // Bail out now if we don't actually have a valid provider.
- match.keyword = is_keyword ?
+ const string16& keyword = is_keyword ?
providers_.keyword_provider() : providers_.default_provider();
- const TemplateURL* provider_url = match.GetTemplateURL(profile_, false);
- if (provider_url == NULL)
+ AutocompleteMatch match = CreateSearchSuggestion(profile_, this, input_,
+ query_string, input_text, relevance, type, accepted_suggestion,
+ is_keyword, keyword);
+ if (!match.destination_url.is_valid())
return;
- match.contents.assign(query_string);
- // We do intra-string highlighting for suggestions - the suggested segment
- // will be highlighted, e.g. for input_text = "you" the suggestion may be
- // "youtube", so we'll bold the "tube" section: you*tube*.
- if (input_text != query_string) {
- size_t input_position = match.contents.find(input_text);
- if (input_position == string16::npos) {
- // The input text is not a substring of the query string, e.g. input
- // text is "slasdot" and the query string is "slashdot", so we bold the
- // whole thing.
- match.contents_class.push_back(
- ACMatchClassification(0, ACMatchClassification::MATCH));
- } else {
- // TODO(beng): ACMatchClassification::MATCH now seems to just mean
- // "bold" this. Consider modifying the terminology.
- // We don't iterate over the string here annotating all matches because
- // it looks odd to have every occurrence of a substring that may be as
- // short as a single character highlighted in a query suggestion result,
- // e.g. for input text "s" and query string "southwest airlines", it
- // looks odd if both the first and last s are highlighted.
- if (input_position != 0) {
- match.contents_class.push_back(
- ACMatchClassification(0, ACMatchClassification::NONE));
- }
- match.contents_class.push_back(
- ACMatchClassification(input_position, ACMatchClassification::DIM));
- size_t next_fragment_position = input_position + input_text.length();
- if (next_fragment_position < query_string.length()) {
- match.contents_class.push_back(
- ACMatchClassification(next_fragment_position,
- ACMatchClassification::NONE));
- }
- }
- } else {
- // Otherwise, we're dealing with the "default search" result which has no
- // completion.
- match.contents_class.push_back(
- ACMatchClassification(0, ACMatchClassification::NONE));
- }
-
- // When the user forced a query, we need to make sure all the fill_into_edit
- // values preserve that property. Otherwise, if the user starts editing a
- // suggestion, non-Search results will suddenly appear.
- if (input_.type() == AutocompleteInput::FORCED_QUERY)
- match.fill_into_edit.assign(ASCIIToUTF16("?"));
- if (is_keyword)
- match.fill_into_edit.append(match.keyword + char16(' '));
- if (!input_.prevent_inline_autocomplete() &&
- StartsWith(query_string, input_text, false)) {
- match.inline_autocomplete_offset =
- match.fill_into_edit.length() + input_text.length();
- }
- match.fill_into_edit.append(query_string);
-
- const TemplateURLRef& search_url = provider_url->url_ref();
- DCHECK(search_url.SupportsReplacement());
- match.search_terms_args.reset(
- new TemplateURLRef::SearchTermsArgs(query_string));
- match.search_terms_args->original_query = input_text;
- match.search_terms_args->accepted_suggestion = accepted_suggestion;
- // This is the destination URL sans assisted query stats. This must be set
- // so the AutocompleteController can properly de-dupe; the controller will
- // eventually overwrite it before it reaches the user.
- match.destination_url =
- GURL(search_url.ReplaceSearchTerms(*match.search_terms_args.get()));
-
- // Search results don't look like URLs.
- match.transition = is_keyword ?
- content::PAGE_TRANSITION_KEYWORD : content::PAGE_TRANSITION_GENERATED;
-
// Try to add |match| to |map|. If a match for |query_string| is already in
// |map|, replace it if |match| is more relevant.
// NOTE: Keep this ToLower() call in sync with url_database.cc.
Message was sent while issue was closed.
Committed patchset #13 manually as r196989 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
