Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333273009:
Don't call AutocompleteInput::Parse() on a background thread, part 2. (Closed)
|
Created:
6 years, 6 months ago by Peter Kasting Modified:
6 years, 6 months ago CC:
chromium-reviews, James Su, Changwan Ryu Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't call AutocompleteInput::Parse() on a background thread, part 2.
This changes HistoryURLProviderParams from holding an ACMatches object to
holding a HistoryMatches object; the HistoryMatches will no longer be fixed up
in DoAutocomplete(), but rather after it returns control to the UI thread.
The majority of this change is mechanical, but some nontrivial changes have to
be made to DoAutocomplete() to account for how the "promote" action it could
previously take must also happen on the UI thread. Therefore the |params|
object also has to gain a few members to allow the post-DoAutocomplete() code to
do promotion correctly.
BUG=376199
TEST=none
R=mpearson@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278558
Patch Set 1 #
Total comments: 11
Patch Set 2 : Review comments #
Total comments: 19
Patch Set 3 : Review comments #
Total comments: 9
Patch Set 4 : Patch for landing #
Messages
Total messages: 11 (0 generated)
Minor feedback on the .h file. I'll get to the .cc file when I have a chance (regardless of whether you reply to these comments first or not). --mark https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this Remember this. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:91: enum PromoteType { nit: comment https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:140: size_t exact_suggestion; nit: (in a later changelist, perhaps) I've always thought this was a horrible name for this variable. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:143: // element of |matches|, or neither as the first AutocompleteMatch. By convention, what is this set to if the first element of matches is the what you typed match? https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:235: // the value of params->promote_type. Clearing matches seems like an odd initial/side effect. Here are better options: * Make the caller clear matches beforehand; call this function AddPromotedMatchIfNecessary. * Call this function something like ResetMatchesToPromotedMatch * Make this function deal fine with pre-existing matches; i.e., make it rearrange matches it already has to promote the relevant match if necessary.
New snap up. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:91: enum PromoteType { On 2014/06/18 18:23:45, Mark P wrote: > nit: comment I couldn't think of anything to put here that wasn't redundant with the comments on |promote_type| below. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:140: size_t exact_suggestion; On 2014/06/18 18:23:45, Mark P wrote: > nit: (in a later changelist, perhaps) > I've always thought this was a horrible name for this variable. Me too. I figured out a way to change this to a bool with a better name. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:143: // element of |matches|, or neither as the first AutocompleteMatch. On 2014/06/18 18:23:45, Mark P wrote: > By convention, what is this set to if the first element of matches is the what > you typed match? Added comments about this. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:235: // the value of params->promote_type. On 2014/06/18 18:23:44, Mark P wrote: > Clearing matches seems like an odd initial/side effect. It is. I did it because both callers wanted it so the code was shorter this way, but that probably shouldn't trump having the function mean something sane, semantically. > Here are better options: > * Make the caller clear matches beforehand; call this function > AddPromotedMatchIfNecessary. I took this route (but didn't rename the function, the existing name seemed better).
https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this Quick question: why is this not for commit? Wouldn't this type of checks help detecting thread issues as early as possible and prevent regression?
https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this On 2014/06/19 00:41:40, Changwan Ryu wrote: > Quick question: why is this not for commit? Wouldn't this type of checks help > detecting thread issues as early as possible and prevent regression? It wouldn't be horrible -- though we should do it as a DCHECK instead of a CHECK in that case -- but the very next patch I'm going to land starts passing a Profile* to Parse() and all functions that call Parse(), which automatically means the functions are only usable on the UI thread (unless otherwise noted), meaning that this becomes somewhat redundant. If we ensure no one is calling this on a background thread now, I'm not really worried about it regressing in the future, as it would be very unusual for someone to actually want to do so.
https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this I see. Thanks for explanation!
This seems logically sound, at statement I mean as high praise given the complexity of this provider. --mark https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:91: enum PromoteType { On 2014/06/18 20:38:30, Peter Kasting wrote: > On 2014/06/18 18:23:45, Mark P wrote: > > nit: comment > > I couldn't think of anything to put here that wasn't redundant with the comments > on |promote_type| below. Then perhaps say "See comments by declaration of |promote_type| below."? https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this Remember me. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:536: params->what_you_typed_match = what_you_typed_match; I think you should explicitly comment something like // All other variables (but one) in |params| were either not changed in DoAutocomplete() or will be overridden regardless in DoAutocomplete(). The one exception is promote_type, which we use to pass information about what the first pass did to the second. Consider also adding one or more comments to the data structure in the header. Maybe a comment by the declaration of promote_type would be appropriate to mention its message-passing nature. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:741: // Checking what_you_typed_match.is_history_what_you_typed_match tells us How about inserting a sentence here to give context for this explanation. The "Try to promote" is too broad. It doesn't give enough context, and then you give into the details. Perhaps something like "We need to know whether we should consider the exact suggestion as being in the user's history and therefore promote it accordingly." (note also the added emphasis here on "considered to be in history", because the latter part of the explanation is elaborating on precisely this nuance. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:745: // into the FRONT_HISTORY_MATCH case for "foo.com" but the second pass can nit: case -> case below to make it obvious this long comment is referring to a particular nearby line of code. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:753: (!backend || consider comment: // on first pass https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:755: HistoryURLProviderParams::FRONT_HISTORY_MATCH)) && consider comment: // first pass did not inline autocomplete a URL https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:817: // the matches. side comment: I really wish we didn't have the .promoted field on matches, which doesn't actually mean the same thing as promoted in this sense. (.promoted only comes from whether it was promoted by PromoteOrCreateShorterSuggestion().) https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:827: static_cast<int>(params->matches.size() - 1 - first_match)) : I think "- first_match" is unnecessary. If matches is empty, first_match will be zero. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.h:140: // in |matches|. nit: You might want to mention that there are some edge cases with whether we decide to consider an unvisited URL on a known intranet hostname to be in history, and people should refer to the implementation for details. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.h:238: // |params| to the front of |matches_|, depending on the value of nit for clarity: |params| -> |params->matches| (to make it obvious that the matches in params is not matches_ (in this class / the one the comment is referring to). I flipped back and forth between reading the code and reading this comment a few times before I got it.
New snap up. https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/1/chrome/browser/autocomplete/... chrome/browser/autocomplete/history_url_provider.h:91: enum PromoteType { On 2014/06/19 03:53:13, Mark P wrote: > On 2014/06/18 20:38:30, Peter Kasting wrote: > > On 2014/06/18 18:23:45, Mark P wrote: > > > nit: comment > > > > I couldn't think of anything to put here that wasn't redundant with the > comments > > on |promote_type| below. > > Then perhaps say "See comments by declaration of |promote_type| below."? Done. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.cc (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:536: params->what_you_typed_match = what_you_typed_match; On 2014/06/19 03:53:14, Mark P wrote: > I think you should explicitly comment something like > // All other variables (but one) in |params| were either not changed in > DoAutocomplete() or will be overridden regardless in DoAutocomplete(). The one > exception is promote_type, which we use to pass information about what the first > pass did to the second. I removed this as discussed in person. I also added a comment here about not resetting |params|, and why. > Consider also adding one or more comments to the data structure in the header. > Maybe a comment by the declaration of promote_type would be appropriate to > mention its message-passing nature. Yep, added. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:741: // Checking what_you_typed_match.is_history_what_you_typed_match tells us On 2014/06/19 03:53:14, Mark P wrote: > How about inserting a sentence here to give context for this explanation. The > "Try to promote" is too broad. It doesn't give enough context, and then you > give into the details. Perhaps something like > "We need to know whether we should consider the exact suggestion as being in the > user's history and therefore promote it accordingly." > > (note also the added emphasis here on "considered to be in history", because the > latter part of the explanation is elaborating on precisely this nuance. I reorganized the comments here a bit. I took the unusual step of placing comments in the midst of the conditional, because I think it's clearer in this case to do so. This also addresses your next few comments. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:817: // the matches. On 2014/06/19 03:53:14, Mark P wrote: > side comment: I really wish we didn't have the .promoted field on matches, which > doesn't actually mean the same thing as promoted in this sense. (.promoted only > comes from whether it was promoted by PromoteOrCreateShorterSuggestion().) I found a way to eliminate this member by adding a return value to PromoteOrCreateShorterSuggestion(). https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:827: static_cast<int>(params->matches.size() - 1 - first_match)) : On 2014/06/19 03:53:14, Mark P wrote: > I think "- first_match" is unnecessary. If matches is empty, first_match will > be zero. You're right, though it took me some examination to be sure. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.h (right): https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.h:140: // in |matches|. On 2014/06/19 03:53:14, Mark P wrote: > nit: You might want to mention that there are some edge cases with whether we > decide to consider an unvisited URL on a known intranet hostname to be in > history, and people should refer to the implementation for details. Done. https://codereview.chromium.org/333273009/diff/20001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.h:238: // |params| to the front of |matches_|, depending on the value of On 2014/06/19 03:53:15, Mark P wrote: > nit for clarity: |params| -> |params->matches| > (to make it obvious that the matches in params is not matches_ (in this class / > the one the comment is referring to). > I flipped back and forth between reading the code and reading this comment a few > times before I got it. > Done.
https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/autocomplete_input.cc (right): https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/autocomplete_input.cc:133: // NOT FOR COMMIT: Remove this once trybots make it clear no callers call this Remember me. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.cc (right): https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:747: // suggest the exact input as a better match. (Note that during the first nit: At the beginning of this line, consider inserting "consider the exact suggestion to be in history and therefore will not" I think it better explains the flow of the logic here. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:751: FixupExactSuggestion(db, classifier, params); nit: I feel like there out to be a trivial comment above FixupExactSuggestion. This current conditional reads funny with intra-conditional comments on every clause but this one. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:753: // If the exact suggestion is in history, we should treat it as the best match optional nit: is in history -> is considered to be in history https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:1004: ensure_can_inline; Please leave things in the other order: i.e., ensure_can_inline && CreateOrPromote(). CreateOrPromoteMatch() can have side effects, and I'm not positive we want to run it if ensure_can_inline is false. (I imagine &= does not evaluate the right hand side if the left hand side is 0.)
Uploaded a new patch. I've taken out the check in autocomplete_input.cc in this one so that it can be landed assuming the trybot runs on patch set 3 look good. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... File chrome/browser/autocomplete/history_url_provider.cc (right): https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:747: // suggest the exact input as a better match. (Note that during the first On 2014/06/19 21:35:14, Mark P wrote: > nit: At the beginning of this line, consider inserting > "consider the exact suggestion to be in history and therefore will not" > > I think it better explains the flow of the logic here. > Done. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:751: FixupExactSuggestion(db, classifier, params); On 2014/06/19 21:35:14, Mark P wrote: > nit: I feel like there out to be a trivial comment above FixupExactSuggestion. > This current conditional reads funny with intra-conditional comments on every > clause but this one. I solved this by introducing a temp. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:753: // If the exact suggestion is in history, we should treat it as the best match On 2014/06/19 21:35:14, Mark P wrote: > optional nit: is in history -> is considered to be in history Modified the comment in a somewhat related way. https://codereview.chromium.org/333273009/diff/40001/chrome/browser/autocompl... chrome/browser/autocomplete/history_url_provider.cc:1004: ensure_can_inline; On 2014/06/19 21:35:14, Mark P wrote: > Please leave things in the other order: i.e., ensure_can_inline && > CreateOrPromote(). CreateOrPromoteMatch() can have side effects, and I'm not > positive we want to run it if ensure_can_inline is false. (I imagine &= does > not evaluate the right hand side if the left hand side is 0.) Discussed in person why this is correct.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
