Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23164011:
Omnibox: Reduce Bolding Flicker in SearchProvider (Closed)
|
Created:
7 years, 4 months ago by Mark P Modified:
6 years, 11 months ago CC:
chromium-reviews, James Su Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Reduce Bolding Flicker in SearchProvider
Do this by computing bolding at the time results are fetched from the
suggest server and keeping that same bolding regardless of what
keys the user presses later.
At the moment, this change only affects query suggestions, not
navsuggestions.
BUG=259486
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243998
Patch Set 1 #
Total comments: 8
Patch Set 2 : even fancier solution #Patch Set 3 : even fancier solution #
Total comments: 6
Patch Set 4 : cleaned up zero suggest #
Total comments: 14
Patch Set 5 : rebase now that many cleanup changelist have been submitted #Patch Set 6 : polish edit case #
Total comments: 26
Patch Set 7 : Mike's comments #
Total comments: 10
Patch Set 8 : Anuj and Mike's comments #Patch Set 9 : fix test to compile #
Total comments: 6
Patch Set 10 : rename function #Patch Set 11 : fix test to set match_contents #
Messages
Total messages: 39 (0 generated)
Hi Mike, the lord of all refactoring in SearchProvider,
I took a stab at fixing the associated bug. This is the cleanest approach I
could come up with. What do you think?
Please only look at search_provider.{cc,h}, not zero-suggest provider, and don't
worry about bad comments, overly-long lines, bad spacing, etc. I didn't bother
to clean up this patchset because it wouldn't be worth the time to do so if you
don't like the approach.
--mark
Some comments to explain what I did in this patchset. --mark https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:144: contents_.assign(suggestion_); This whole blob was moved from CreateSearchSuggestion, with match.contents -> contents_ match.contents_class -> contents_class_ https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:220: // First look for the user's input inside the fill_into_edit as it would be This whole section is the necessary parts from NavigationToMatch() to compute contents_ and contents_class_. https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:952: ACMatchClassifications contents_class; Forgot to delete those two lines, sorry. https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.h:180: // string16 fill_into_edit_; I abandoned this part, sorry that the lines are still here. https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.h:321: static AutocompleteMatch CreateSearchSuggestion( FYI, this is an exact move except I changed some of the parameters to this function. I didn't yet bother revising the comment.
I think I'm okay with the overall approach, but I guess this means we'll no longer synchronously update result bolding as the user edits their input and we'll have to wait on new suggestions, right? I think this is a little lame, but probably not as lame as the current flickering. Can you get the CL into a bare minimum working state so we can test the patch locally against ToT with the steps in the bug's report? https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:220: // First look for the user's input inside the fill_into_edit as it would be On 2013/08/15 18:34:23, Mark P (likely away 8.26-9.15) wrote: > This whole section is the necessary parts from NavigationToMatch() to compute > contents_ and contents_class_. Can we reduce more duplication between the two?
> I think I'm okay with the overall approach, but I guess this means we'll no > longer synchronously update result bolding as the user edits their input and > we'll have to wait on new suggestions, right? Yes. > I think this is a little lame, but probably not as lame as the current > flickering. I agree. I actually think it's more lame than the current flickering (which I think basically only happens on backspace). But it sounds like I'm outvoted. > Can you get the CL into a bare minimum working state so we can test > the patch locally against ToT with the steps in the bug's report? It is in that working state, and it fixes the issue in the bug. --mark https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:220: // First look for the user's input inside the fill_into_edit as it would be On 2013/08/15 18:51:03, msw wrote: > On 2013/08/15 18:34:23, Mark P (likely away 8.26-9.15) wrote: > > This whole section is the necessary parts from NavigationToMatch() to compute > > contents_ and contents_class_. > > Can we reduce more duplication between the two? I can't figure out how. Here's the problem: inline_autocompletion has to be computed live. It's computed using inline_autocomplete_offset. Many of the lines that remain in NavigationToMatch are things that are used to compute inline_autocomplete_offset; all these have to happen live on the current input, none can be moved into NavigationResult for pre-computing. The only idea I have is to make a helper function that returns format_types and a tentative inline_autocomplete_offset (that later may get modified by NavigationToMatch's FormatURL call). That'll eliminate most of the common code, but it sounds a little awkward. What do you think?
On Thu, Aug 15, 2013 at 12:28 PM, <mpearson@chromium.org> wrote: > I think I'm okay with the overall approach, but I guess this means we'll >> no >> longer synchronously update result bolding as the user edits their input >> and >> we'll have to wait on new suggestions, right? >> > > Yes. I didn't look at the change, I only read you guys' comments, but I'm not OK with this, assuming it affects all matches, and not just the ones "held over". The change should only affect "held over" matches, and even then, we should update the bolding in them if the new input string can be found (and thus we're not going to suddenly bold the whole match). Basically, we should narrowly target the fix. I think this is a little lame, but probably not as lame as the current >> flickering. >> > > I agree. I actually think it's more lame than the current flickering > (which > I think basically only happens on backspace). But it sounds like I'm > outvoted. No, you're not. PK
On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: > > I think this is a little lame, but probably not as lame as the current > > flickering. > > I agree. I actually think it's more lame than the current flickering (which > I think basically only happens on backspace). But it sounds like I'm > outvoted. Eh, flickering happens on typing something that breaks leading substring matching too (typing 'd' after "foo" when you have suggestion "foot" makes "fooT" turn into "FOOT" before the result goes away). I tried the patch locally, and it does seem nicer (less jittery) for those types of cases, but I don't know that it's definitively better. I have a spitball idea that might be the best of both worlds. We could take the set of synchronously re-used Results and only update the classifications in some limited (less jarring) scenarios. For example, the updated input "foo"->"food" could yield "fooT" (show the matching leading substrings even though the whole input isn't a leading substring of the suggestion, instead of jarring the whole suggestion by making it completely bold on a single letter change). We could still update suggestion "fooDS" to "foodS" since the bolding change would be more subtle and reasonable. Then hitting backspace could yield "fooT" (still) and "fooDS" once again. WDYT? Shouldn't be *too* tough. https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/s... chrome/browser/autocomplete/search_provider.cc:220: // First look for the user's input inside the fill_into_edit as it would be On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: > On 2013/08/15 18:51:03, msw wrote: > > On 2013/08/15 18:34:23, Mark P (likely away 8.26-9.15) wrote: > > > This whole section is the necessary parts from NavigationToMatch() to > compute > > > contents_ and contents_class_. > > > > Can we reduce more duplication between the two? > > I can't figure out how. Here's the problem: inline_autocompletion has to be > computed live. It's computed using inline_autocomplete_offset. Many of the > lines that remain in NavigationToMatch are things that are used to compute > inline_autocomplete_offset; all these have to happen live on the current input, > none can be moved into NavigationResult for pre-computing. > > The only idea I have is to make a helper function that returns format_types and > a tentative inline_autocomplete_offset (that later may get modified by > NavigationToMatch's FormatURL call). That'll eliminate most of the common code, > but it sounds a little awkward. What do you think? I was thinking of a helper function, but only if works well.
On Thu, Aug 15, 2013 at 12:48 PM, <msw@chromium.org> wrote: > I have a spitball idea that might be the best of both worlds. We could > take the > set of synchronously re-used Results and only update the classifications > in some > limited (less jarring) scenarios. For example, the updated input > "foo"->"food" > could yield "fooT" (show the matching leading substrings even though the > whole > input isn't a leading substring of the suggestion, instead of jarring the > whole > suggestion by making it completely bold on a single letter change). We > could > still update suggestion "fooDS" to "foodS" since the bolding change would > be > more subtle and reasonable. Then hitting backspace could yield "fooT" > (still) > and "fooDS" once again. WDYT? Shouldn't be *too* tough. I think this is basically what I was suggesting also. PK
On Thu, Aug 15, 2013 at 12:48 PM, <msw@chromium.org> wrote: > On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: > >> > I think this is a little lame, but probably not as lame as the current >> > flickering. >> > > I agree. I actually think it's more lame than the current flickering >> (which >> I think basically only happens on backspace). But it sounds like I'm >> outvoted. >> > > Eh, flickering happens on typing something that breaks leading substring > matching too (typing 'd' after "foo" when you have suggestion "foot" makes > "fooT" turn into "FOOT" before the result goes away). I tried the patch > locally, > and it does seem nicer (less jittery) for those types of cases, but I > don't know > that it's definitively better. > > I have a spitball idea that might be the best of both worlds. We could > take the > set of synchronously re-used Results and only update the classifications > in some > limited (less jarring) scenarios. For example, the updated input > "foo"->"food" > could yield "fooT" (show the matching leading substrings even though the > whole > input isn't a leading substring of the suggestion, instead of jarring the > whole > suggestion by making it completely bold on a single letter change). We > could > still update suggestion "fooDS" to "foodS" since the bolding change would > be > more subtle and reasonable. Then hitting backspace could yield "fooT" > (still) > and "fooDS" once again. WDYT? Shouldn't be *too* tough. Your proposal sounds nice. I think the right way to implement it would be to have something that runs at about the same time as RemoveStaleResults that recalculates contents_class. If the suggestion is consistent with the new input, then bold the suggestion (a.k.a. contents) properly; if it's not consistent (i.e., we'd default to the bold everything case), then leave the bolding as it is. --mark > > https://codereview.chromium.**org/23164011/diff/1/chrome/** > browser/autocomplete/search_**provider.cc<https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/search_provider.cc> > File chrome/browser/autocomplete/**search_provider.cc (right): > > https://codereview.chromium.**org/23164011/diff/1/chrome/** > browser/autocomplete/search_**provider.cc#newcode220<https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/search_provider.cc#newcode220> > chrome/browser/autocomplete/**search_provider.cc:220: // First look for > the user's input inside the fill_into_edit as it would be > On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: > >> On 2013/08/15 18:51:03, msw wrote: >> > On 2013/08/15 18:34:23, Mark P (likely away 8.26-9.15) wrote: >> > > This whole section is the necessary parts from NavigationToMatch() >> > to > >> compute >> > > contents_ and contents_class_. >> > >> > Can we reduce more duplication between the two? >> > > I can't figure out how. Here's the problem: inline_autocompletion has >> > to be > >> computed live. It's computed using inline_autocomplete_offset. Many >> > of the > >> lines that remain in NavigationToMatch are things that are used to >> > compute > >> inline_autocomplete_offset; all these have to happen live on the >> > current input, > >> none can be moved into NavigationResult for pre-computing. >> > > The only idea I have is to make a helper function that returns >> > format_types and > >> a tentative inline_autocomplete_offset (that later may get modified by >> NavigationToMatch's FormatURL call). That'll eliminate most of the >> > common code, > >> but it sounds a little awkward. What do you think? >> > > I was thinking of a helper function, but only if works well. > > https://codereview.chromium.**org/23164011/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry I forgot to ask the implicit ending: what do you think? On Thu, Aug 15, 2013 at 5:04 PM, Mark Pearson <mpearson@chromium.org> wrote: > > On Thu, Aug 15, 2013 at 12:48 PM, <msw@chromium.org> wrote: > >> On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: >> >>> > I think this is a little lame, but probably not as lame as the current >>> > flickering. >>> >> >> I agree. I actually think it's more lame than the current flickering >>> (which >>> I think basically only happens on backspace). But it sounds like I'm >>> outvoted. >>> >> >> Eh, flickering happens on typing something that breaks leading substring >> matching too (typing 'd' after "foo" when you have suggestion "foot" makes >> "fooT" turn into "FOOT" before the result goes away). I tried the patch >> locally, >> and it does seem nicer (less jittery) for those types of cases, but I >> don't know >> that it's definitively better. >> >> I have a spitball idea that might be the best of both worlds. We could >> take the >> set of synchronously re-used Results and only update the classifications >> in some >> limited (less jarring) scenarios. For example, the updated input >> "foo"->"food" >> could yield "fooT" (show the matching leading substrings even though the >> whole >> input isn't a leading substring of the suggestion, instead of jarring the >> whole >> suggestion by making it completely bold on a single letter change). We >> could >> still update suggestion "fooDS" to "foodS" since the bolding change would >> be >> more subtle and reasonable. Then hitting backspace could yield "fooT" >> (still) >> and "fooDS" once again. WDYT? Shouldn't be *too* tough. > > > Your proposal sounds nice. I think the right way to implement it would be > to have something that runs at about the same time as RemoveStaleResults > that recalculates contents_class. If the suggestion is consistent with the > new input, then bold the suggestion (a.k.a. contents) properly; if it's not > consistent (i.e., we'd default to the bold everything case), then leave the > bolding as it is. > > --mark > > >> >> https://codereview.chromium.**org/23164011/diff/1/chrome/** >> browser/autocomplete/search_**provider.cc<https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/search_provider.cc> >> File chrome/browser/autocomplete/**search_provider.cc (right): >> >> https://codereview.chromium.**org/23164011/diff/1/chrome/** >> browser/autocomplete/search_**provider.cc#newcode220<https://codereview.chromium.org/23164011/diff/1/chrome/browser/autocomplete/search_provider.cc#newcode220> >> chrome/browser/autocomplete/**search_provider.cc:220: // First look for >> the user's input inside the fill_into_edit as it would be >> On 2013/08/15 19:28:15, Mark P (likely away 8.26-9.15) wrote: >> >>> On 2013/08/15 18:51:03, msw wrote: >>> > On 2013/08/15 18:34:23, Mark P (likely away 8.26-9.15) wrote: >>> > > This whole section is the necessary parts from NavigationToMatch() >>> >> to >> >>> compute >>> > > contents_ and contents_class_. >>> > >>> > Can we reduce more duplication between the two? >>> >> >> I can't figure out how. Here's the problem: inline_autocompletion has >>> >> to be >> >>> computed live. It's computed using inline_autocomplete_offset. Many >>> >> of the >> >>> lines that remain in NavigationToMatch are things that are used to >>> >> compute >> >>> inline_autocomplete_offset; all these have to happen live on the >>> >> current input, >> >>> none can be moved into NavigationResult for pre-computing. >>> >> >> The only idea I have is to make a helper function that returns >>> >> format_types and >> >>> a tentative inline_autocomplete_offset (that later may get modified by >>> NavigationToMatch's FormatURL call). That'll eliminate most of the >>> >> common code, >> >>> but it sounds a little awkward. What do you think? >>> >> >> I was thinking of a helper function, but only if works well. >> >> https://codereview.chromium.**org/23164011/<https://codereview.chromium.org/2... >> > >
On 2013/08/16 00:05:18, Mark P (likely away 8.26-9.15) wrote: > > Your proposal sounds nice. I think the right way to implement it would be > > to have something that runs at about the same time as RemoveStaleResults > > that recalculates contents_class. If the suggestion is consistent with the > > new input, then bold the suggestion (a.k.a. contents) properly; if it's not > > consistent (i.e., we'd default to the bold everything case), then leave the > > bolding as it is. > > > > --mark That's probably fine, though recomputing elsewhere isn't necessarily bad. One think I didn't think about is changes near the start/middle of the text...
On Thu, Aug 15, 2013 at 5:33 PM, <msw@chromium.org> wrote: > One think I didn't think about is changes near the start/middle of the > text... > As with other changes, we should update the bolding for them if the new input string is still found within the result. If it's not, we just don't change the bolding. PK
I have a new version that works almost as we discussed. It recalculate bolding
on every keystroke but tries to avoid problematically large bolding changes.
You can read the header files for details on how it works. This new version is
a valid patch; you can download it and try it if you want. I think it looks
good.
This time I actually comments and formatted the code in search_provider.{cc,h}
and autocomplete_provider.{cc,h} correctly. All of that is ready for a real
review.
Please don't yet look at zero suggest provider. It's currently a mess. (It's
just hobbled together so this patchset compiles.)
--mark
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_provider.cc (right):
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_provider.cc:112: bool
AutocompleteProvider::HasHTTPScheme(const string16& input) {
This is a cut and paste.
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:111: // Look for the user's input
inside the formatted_url as it would be without
This logic was moved here from NavigationToMatch().
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:185: void
SearchProvider::SuggestResult::CalculateContents(
This is still basically a cut-and-paste from CreateSearchSuggestion, with the
change to handle allow_bolding_all and also a replacement of match.contents to
contents_ and match.contents_class to contents_class_.
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:286: const net::FormatUrlTypes
format_types =
This logic here and below was moved here from NavigationToMatch().
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:369: AutocompleteMatch
SearchProvider::CreateSearchSuggestion(
When this review is almost complete, I will move this to the right place in this
file according to header order. I've left it as is for the moment for ease of
review.
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.h (right):
https://codereview.chromium.org/23164011/diff/19001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.h:333: static AutocompleteMatch
CreateSearchSuggestion(
This is both a move from its previous location and changing a few command line
parameters. This time I updated the comment above appropriately.
Consider this a friendly reminder about this review. (I'm not in a hurry to submit this change; I just think we all forgot about it this week.) --mark
(1) I was waiting on msw's review to give an OWNERS review, did you want me to review directly as well? (2) You said zero_suggest_provider wasn't ready, were you going to change things there?
On Thu, Aug 22, 2013 at 11:17 AM, <pkasting@chromium.org> wrote: > (1) I was waiting on msw's review to give an OWNERS review, did you want > me to > review directly as well? > You can wait for him and then review it to whatever extent you desire. > (2) You said zero_suggest_provider wasn't ready, were you going to change > things > there? > Good point. Will do. --mark > > https://codereview.chromium.**org/23164011/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I cleaned up the zero suggest code. The unit tests still don't compile, but I don't want to spend time to fix them until I know this is an approach you guys like. --mark
I like some of the refactoring here that seems tangential to the actual change but reduces complexity and parameters. On the other hand, it's a relatively minor behavioral improvement and the code is still sorely in need of more refactoring. But if you and Peter this this CL is fine without my suggestions, I'll back off a little and hope some future work cleans up some of the mess here. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:106: void GetTrimHttpAndOffsets(const string16& formatted_url, Yeah, this wound up being slightly awkward, mostly because it has 3 out params, one of which is not used in one of the two function callsites. If you'd prefer to duplicate this code inline at the callsites, I wouldn't be too opposed, but I think that's indicative of some other refactoring potentially yielding a better final result. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:276: bool trim_http; nit: explicitly init to false. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:277: size_t match_start, inline_autocomplete_offset; nit: declare each on separate lines; explicitly init to 0. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1555: bool trim_http; nit: explicitly init to false. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1556: size_t match_start, inline_autocomplete_offset; nit: declare each on separate lines; explicitly init to 0. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:177: // The main text displayed in the address bar dropdown. nit: ..."and its style info." or similar that fits on one line. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:192: // Most parameters are self-explanatory. |input_text| is used to I think this whole comment is unnecessary here; ditto for NavigationResult. Put the bit about avoiding flicker in SearchProvider::RecalculateBolding. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:210: void CalculateContents(bool allow_bolding_all, It'd be nice to put a single pure virtual decl in the base Result struct, even though the two structs would OVERRIDE to provide distinct impls and SuggestResult would ignore the |languages| arg. I suggest a consolidated decl to make it easier to handle SuggestResult and NavigationResult instances similarly/together in the future. Even better would be more cohesion of some of the match generation code, specifically adding a |Result::fill_into_edit_| member that's set within this same function alongside |contents_| and |contents_class_|. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:316: // and |type| based on the search query suggestion represented in |result| nit: add trailing period. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:333: static AutocompleteMatch CreateSearchSuggestion( This would be simpler as a non-static function more similar to NavigationToMatch; it could drop |autocomplete_provider| (using |this| internally), drop |input| and |input_text| for |input_|[.text()], and use result.from_keyword_provider() instead of taking |template_url|. I would also highly recommend removing the last three parameters (int accepted_suggestion, int omnibox_start_margin, and bool append_extra_query_params) and simply setting that data within match.search_terms_args and match.destination_url from SearchProvider::AddMatchToMap. I know it's a bit tangential, but SearchProvider is such a mess it's hardly maintainable. I can hardly imagine approving this CL's additional complexity for a fairly low-impact behavioral change without offsetting the new complexity a bit (either now or with some prerequisite work). https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:338: const string16& input_text, Remove this and just use |input|'s AutocompleteInput::text(). https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/zero_suggest_provider.cc:277: const string16 current_query_str16 = ASCIIToUTF16(current_query_); Drop "_str16", use "_string" or "_string16" if you must. https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/zero_suggest_provider.cc:324: // Pass in result.suggestion() as the input_text since we don't want any nit: "to avoid bolding" https://codereview.chromium.org/23164011/diff/26001/chrome/browser/autocomple... chrome/browser/autocomplete/zero_suggest_provider.cc:357: // We ignore navigation.contents() and navigation.contents_class() nit: "Zero suggest results should always omit protocols and never appear bold."
Hey Mark, did this slip off your radar?
On 2013/11/13 08:06:53, Peter Kasting wrote: > Hey Mark, did this slip off your radar? Yes, though somewhat intentionally. This changelist is for a minor (in my opinion) bug and yet was getting to be a headache. More directly, some of Mike's suggestions were roughly the opposite of things he previously said (e.g., on another change I previously tried to move some things to the base class from the children and he opposed it, now his comments encourage it) and I wasn't ready to resurface that previous long-winded discussion. Maybe I'll feel like talking about it in a week or two. --mark
On 2013/11/13 17:14:37, Mark P wrote: > On 2013/11/13 08:06:53, Peter Kasting wrote: > > Hey Mark, did this slip off your radar? > > Yes, though somewhat intentionally. This changelist is for a minor (in my > opinion) bug OK. I still think this is pretty important to do.
Peter & Mike, I revived this changelist after rebasing on top of some cleanup changes I submitted in the last month. I also removed the navsuggest stuff from this change, so it's cleaner and easier to evaluate whether you like this approach. Can the two of you please take another look? If it's okay, I'll attempt to add navsuggest bolding stuff too. I'll also ask Anuj to look (to double check what he though he was doing with match_content.empty()). thanks, mark https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (left): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:105: // Builds the match contents and classification for the contents, and updates This function moved to CalculateContentsClass() with slight modifications: - reflect different variable names, pointers versus not - remove setting of match->contents (this gets pushed earlier to ParseSuggestResults()) - add code to handle allow_bolding_all == false case
I tried to look at this, and for some reason my brain is just fried and won't process anything. It seems like other than moving a function you're not actually changing very much, so maybe Mike's review will be sufficient?
On Fri, Jan 3, 2014 at 4:48 PM, <pkasting@chromium.org> wrote: > I tried to look at this, and for some reason my brain is just fried and > won't > process anything. It seems like other than moving a function you're not > actually changing very much The only thing this does is precompute |contents_class| for query suggestions, rather than recomputing it on the fly on every input. Then, under some conditions (certain inputs), we recompute it again. --mark > , so maybe Mike's review will be sufficient? > > https://codereview.chromium.org/23164011/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:239: if (!allow_bolding_all && (input_text != match_contents_) && nit: remove the unnecessary input_text != match_contents_ string comparison. If the strings were equal, then input_position would not be base::string16::npos (it would be zero), and this code wouldn't trigger anyway, right? https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:251: size_t input_position = match_contents_.find(input_text); Remove this, use the above duplicated identifier... https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:420: nit: remove blank line (or swap with line above.) https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:581: const base::string16& input_text, SuggestResults* suggest_results) { nit: one param per line. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:894: // Revise the bolding of suggest results that remain so they look good against nit: consider "Update the content and classifications of remaining results." or similar. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1229: // Error correction for bad data from server. |match_contents| was already initialized with |suggestion| in the general case, so this could only be helpful in the |suggestion_details| case. This code should be moved directly after that |match_contents| modification. More importantly, why would the server supply a blank "title" or "t" field in the suggestion details? That should be fixed on the server side, even if we really do need this stupid workaround here. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:225: const ACMatchClassifications& match_contents_class() const { nit: consider match_classifications_ or match_class_ https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:239: void CalculateContentsClass(const bool allow_bolding_all, nit: consider "Calculate[Match]Classifications". https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:254: // The contents to be displayed in the autocomplete match, and its style nit: remove "in the autocomplete match" https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:386: // For every result in |suggest_results|, call CalculateContentsClass(), nit: consider "Recalculates the match contents and classifications of |suggest_results|, given the current input." or similar. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:389: static void RecalculateBolding(const base::string16& input_text, nit: consider "Recalculate[Match]Classifications" or similar. Bolding seems like an implementation detail of the presentation of certain match classifications.
https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:239: if (!allow_bolding_all && (input_text != match_contents_) && On 2014/01/06 21:09:00, msw wrote: > nit: remove the unnecessary input_text != match_contents_ string comparison. If > the strings were equal, then input_position would not be base::string16::npos > (it would be zero), and this code wouldn't trigger anyway, right? Right you are. Done. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:251: size_t input_position = match_contents_.find(input_text); On 2014/01/06 21:09:00, msw wrote: > Remove this, use the above duplicated identifier... Stupid resolve error re-introduced this. I had it right before! (I'm not a dumbass. :-) ) https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:420: On 2014/01/06 21:09:00, msw wrote: > nit: remove blank line (or swap with line above.) Done. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:581: const base::string16& input_text, SuggestResults* suggest_results) { On 2014/01/06 21:09:00, msw wrote: > nit: one param per line. Done. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:894: // Revise the bolding of suggest results that remain so they look good against On 2014/01/06 21:09:00, msw wrote: > nit: consider "Update the content and classifications of remaining results." or > similar. Done. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1229: // Error correction for bad data from server. On 2014/01/06 21:09:00, msw wrote: > |match_contents| was already initialized with |suggestion| in the general case, > so this could only be helpful in the |suggestion_details| case. This code should > be moved directly after that |match_contents| modification. More importantly, > why would the server supply a blank "title" or "t" field in the suggestion > details? That should be fixed on the server side, even if we really do need this > stupid workaround here. I plan to talk to anuj about this because he's the one who added code to set match_contents to suggestion if the suggestion was blank. I think this code is only to handle bad data from the server; he can tell me for sure. Note that I like having the code here regardless under the principle you've long supported that SearchProvider should do something sane regardless of what it gets back from the server. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:225: const ACMatchClassifications& match_contents_class() const { On 2014/01/06 21:09:00, msw wrote: > nit: consider match_classifications_ or match_class_ I'd prefer to keep the name that corresponds to the name of the variable name in the corresponding AutocompleteMatch. Note that there are two variables of types of ACMatchClassification associated with an AutocompleteMatch, so referring to something as, say, match_classification is unclear. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:239: void CalculateContentsClass(const bool allow_bolding_all, On 2014/01/06 21:09:00, msw wrote: > nit: consider "Calculate[Match]Classifications". see comment above. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:254: // The contents to be displayed in the autocomplete match, and its style On 2014/01/06 21:09:00, msw wrote: > nit: remove "in the autocomplete match" Done. https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:386: // For every result in |suggest_results|, call CalculateContentsClass(), On 2014/01/06 21:09:00, msw wrote: > nit: consider "Recalculates the match contents and classifications of > |suggest_results|, given the current input." or similar. Shortened it much like you suggested. In the process, dropped the "match contents" section because I realized this doesn't recalculate match contents. (That was an earlier version of this change.) https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:389: static void RecalculateBolding(const base::string16& input_text, On 2014/01/06 21:09:00, msw wrote: > nit: consider "Recalculate[Match]Classifications" or similar. Bolding seems like > an implementation detail of the presentation of certain match classifications. Good point. Changed to RecalculateMatchContentsClass().
Hi Anuj, I added you as a reviewer to this changelist because I don't really understand what you were doing with the line put in SetAndClassifyMatchContents() that does: match->contents = match_contents.empty() ? query_string : match_contents; Why would match_contents be empty? As near as I can tell, it'll be empty if the server says it should be empty (i.e., bad data from the server) and at no other times. Is this what you meant to correct? As you can see in this change, I removed that line from SetAndClassifyMatchContents() (indeed, I removed the whole function), and instead placed a match_contents is empty check in ParseSuggestResult(). Please take a look and decide if this seems reasonable to you. thanks, mark
https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1229: // Error correction for bad data from server. On 2014/01/06 22:20:16, Mark P wrote: > On 2014/01/06 21:09:00, msw wrote: > > |match_contents| was already initialized with |suggestion| in the general > case, > > so this could only be helpful in the |suggestion_details| case. This code > should > > be moved directly after that |match_contents| modification. More importantly, > > why would the server supply a blank "title" or "t" field in the suggestion > > details? That should be fixed on the server side, even if we really do need > this > > stupid workaround here. > > I plan to talk to anuj about this because he's the one who added code to set > match_contents to suggestion if the suggestion was blank. I think this code is > only to handle bad data from the server; he can tell me for sure. > > Note that I like having the code here regardless under the principle you've long > supported that SearchProvider should do something sane regardless of what it > gets back from the server. Keeping the workaround to fixup the broken |suggestion_detail| server behavior may be necessary (and in that case it's fine by me), but since |match_contents| is *initialized* to |suggestion| and only ever reassigned in the above |suggestion_detail| block, I strongly suggest moving this assignment into that block, immediately after the "suggestion_detail->GetString("title", &match_contents)... " line. (ie. only fixup |match_contents| in the case where it'll be broken; otherwise when |suggestion_details| is NULL, you're doing "match_contents = suggestion; if (match_contents.empty()) match_contents = suggestion;"...) https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.h:225: const ACMatchClassifications& match_contents_class() const { On 2014/01/06 22:20:16, Mark P wrote: > On 2014/01/06 21:09:00, msw wrote: > > nit: consider match_classifications_ or match_class_ > > I'd prefer to keep the name that corresponds to the name of the variable name in > the corresponding AutocompleteMatch. > > Note that there are two variables of types of ACMatchClassification associated > with an AutocompleteMatch, so referring to something as, say, > match_classification is unclear. okay https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:249: base::ASCIIToUTF16(current_query_); Use UTF8ToUTF16; or is |current_query_| really ASCII? Consider |current_query|.
On Mon, Jan 6, 2014 at 2:24 PM, <mpearson@chromium.org> wrote: > > Hi Anuj, > > I added you as a reviewer to this changelist because I don't really > understand > what you were doing with the line put in > SetAndClassifyMatchContents() > that does: > match->contents = match_contents.empty() ? query_string : match_contents; > > Why would match_contents be empty? As near as I can tell, it'll be empty > if the > server says it should be empty (i.e., bad data from the server) and at no > other > times. Is this what you meant to correct? > The server provides match_contents only when it is different from suggestion. That check is an artifact of my first implementation where I didn't initialize match_contents to be same as suggestion at the time of parsing. > As you can see in this change, I removed that line from > SetAndClassifyMatchContents() (indeed, I removed the whole function), and > instead placed a match_contents is empty check in ParseSuggestResult(). > That change looks good. --------------------- While you are meddling with this, I wish someone would just get rid of the ACMatchClassifications. It over-specifies the possible formats without being generic enough. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:258: // "bold" this. Consider modifying the terminology. I think we can remove this TODO. Although it seems to mean just BOLD, the Android implementation clearly doesn't follow this. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1211: base::string16 match_contents = suggestion; Probably omit this assignment given the check you are adding below. Also, remove the comment about bad data. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:219: const base::string16& input_text); I will prefer input_text as the first parameter. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:387: static void RecalculateMatchContentsClass(const base::string16& input_text, Minor suggestion : Rename to UpdateMatchContentsClass?
It sounds like we're almost done... --mark https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/94001/chrome/browser/autocomple... chrome/browser/autocomplete/search_provider.cc:1229: // Error correction for bad data from server. On 2014/01/06 22:50:21, msw wrote: > On 2014/01/06 22:20:16, Mark P wrote: > > On 2014/01/06 21:09:00, msw wrote: > > > |match_contents| was already initialized with |suggestion| in the general > > case, > > > so this could only be helpful in the |suggestion_details| case. This code > > should > > > be moved directly after that |match_contents| modification. More > importantly, > > > why would the server supply a blank "title" or "t" field in the suggestion > > > details? That should be fixed on the server side, even if we really do need > > this > > > stupid workaround here. > > > > I plan to talk to anuj about this because he's the one who added code to set > > match_contents to suggestion if the suggestion was blank. I think this code > is > > only to handle bad data from the server; he can tell me for sure. > > > > Note that I like having the code here regardless under the principle you've > long > > supported that SearchProvider should do something sane regardless of what it > > gets back from the server. > > Keeping the workaround to fixup the broken |suggestion_detail| server behavior > may be necessary (and in that case it's fine by me), but since |match_contents| > is *initialized* to |suggestion| and only ever reassigned in the above > |suggestion_detail| block, I strongly suggest moving this assignment into that > block, immediately after the "suggestion_detail->GetString("title", > &match_contents)... " line. (ie. only fixup |match_contents| in the case where > it'll be broken; otherwise when |suggestion_details| is NULL, you're doing > "match_contents = suggestion; if (match_contents.empty()) match_contents = > suggestion;"...) Moved the correcting assignment in the block, as you suggested. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:258: // "bold" this. Consider modifying the terminology. On 2014/01/06 23:23:38, Anuj wrote: > I think we can remove this TODO. Although it seems to mean just BOLD, the > Android implementation clearly doesn't follow this. I agree we can remove it. Removed. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:1211: base::string16 match_contents = suggestion; On 2014/01/06 23:23:38, Anuj wrote: > Probably omit this assignment given the check you are adding below. I left the assignment because I moved the check into the if (suggestion_details) block. > Also, remove the comment about bad data. Left the comment about bad data because, now that the correction is in the block, it's appropriate. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.h (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:219: const base::string16& input_text); On 2014/01/06 23:23:38, Anuj wrote: > I will prefer input_text as the first parameter. The important thing in SuggestResult is the suggestion. Everything else are details about the suggestion or the context in which it comes from. I think the parameter order should follow the principle that the suggestion is central. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.h:387: static void RecalculateMatchContentsClass(const base::string16& input_text, On 2014/01/06 23:23:38, Anuj wrote: > Minor suggestion : Rename to UpdateMatchContentsClass? Yeah, that's better. Done. https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... File chrome/browser/autocomplete/zero_suggest_provider.cc (right): https://codereview.chromium.org/23164011/diff/174001/chrome/browser/autocompl... chrome/browser/autocomplete/zero_suggest_provider.cc:249: base::ASCIIToUTF16(current_query_); On 2014/01/06 22:50:21, msw wrote: > Use UTF8ToUTF16; or is |current_query_| really ASCII? Consider |current_query|. current_query_ is always a URL spec, which means it's ASCII.
LGTM
FYI, I decided to do the followup change to reduce flicker related to navsuggestions in a separate changelist. I'll probably submit this one on mid-day Wednesday, giving Anuj and Peter time for additional comments (if any). --mark
Couple more thoughts https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:235: void SearchProvider::SuggestResult::CalculateContentsClass( I prefer the old name ClassifyMatchContents. Peter, what do you think? https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:236: const bool allow_bolding_all, I do not like this param - allow_bolding_all. Is it possible that you store the input_text as a member property for which this suggestion was created, and replace allow_bolding_all with input_text_ == input_text?
https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:235: void SearchProvider::SuggestResult::CalculateContentsClass( On 2014/01/07 23:48:14, Anuj wrote: > I prefer the old name ClassifyMatchContents. I like that name better too. I'll change it when I'm sitting at my desktop today. (BTW, I don't know why you're calling it "the old name". As far as I can recall, none of the iterations of this change used that name.) > Peter, what do you think? https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:236: const bool allow_bolding_all, On 2014/01/07 23:48:14, Anuj wrote: > I do not like this param - allow_bolding_all. > > Is it possible that you store the input_text as a member property for which this > suggestion was created, and replace allow_bolding_all with input_text_ == > input_text? I don't like that approach. It makes more sense to me to have the parameters describe what functionality is allowed and what is not. The parameter could just as easily be called allow_large_bolding_changes (indeed, perhaps I'll end up changing it and the code to that in the future), in which case it's obvious that trying to pass in input_text instead is the wrong thing to do. It's too roundabout.
lgtm https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... File chrome/browser/autocomplete/search_provider.cc (right): https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:235: void SearchProvider::SuggestResult::CalculateContentsClass( > (BTW, I don't know why you're calling it "the old name". As far as I can > recall, none of the iterations of this change used that name.) I am calling it "the old name", because the method it replaces was named "SetAndClassifyMatchContents" https://codereview.chromium.org/23164011/diff/324001/chrome/browser/autocompl... chrome/browser/autocomplete/search_provider.cc:236: const bool allow_bolding_all, > I don't like that approach. It makes more sense to me to have the parameters > describe what functionality is allowed and what is not. The parameter could > just as easily be called allow_large_bolding_changes (indeed, perhaps I'll end > up changing it and the code to that in the future), in which case it's obvious > that trying to pass in input_text instead is the wrong thing to do. It's too > roundabout. We just removed a TODO about reference to "Bold". So "bold" in this param name is also not valid. While parameters should describe what is allowed and what is not, but I think in this case this parameter needs awareness of the cases when the whole of match_contents may be matched. Sometimes the caller really needs to know the inner workings of a function. But in general, it leads to hard to read code at the point where a method is called. My 2 cents. I won't delay review more on this issue.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/23164011/394001
Retried try job too often on linux_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/23164011/674001
Message was sent while issue was closed.
Change committed as 243998 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
