Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829068:
Add query formulation time to aqs (assisted query stats) parameter (Closed)
|
Created:
8 years, 4 months ago by hfung Modified:
8 years, 4 months ago CC:
chromium-reviews, Bart N Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd query formulation time to aqs (assisted query stats) parameter
BUG=136848
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149326
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 10
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #Messages
Total messages: 9 (0 generated)
This adds the query formulation time to the aqs parameter. However, I was confused how it triggers. It seems to take a while for aqs to get sent after logging in on google.com, and still gets sent even after I log out of google.com (one Chrome restart plus one search seems to be needed for aqs to get logged/unlogged).
https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: AutocompleteMatch match_copy(match); Nit: You don't need to copy the whole match, just to create a new GURL. You can declare said GURL above this conditional so that the actual OnAutocompleteAccept() call can happen unconditionally, eliminating the else arm. https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:610: base::TimeDelta query_formulation_time = base::TimeTicks::Now() Nit: Operators go on the ends of lines; break after '=' https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:613: base::StringPrintf(".%" PRIuS, You've passed the format string for a size_t and then supplied an int64 as the value.
Thanks for taking a look so quickly! https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: AutocompleteMatch match_copy(match); On 2012/07/30 20:24:02, Peter Kasting wrote: > Nit: You don't need to copy the whole match, just to create a new GURL. > > You can declare said GURL above this conditional so that the actual > OnAutocompleteAccept() call can happen unconditionally, eliminating the else > arm. Done. https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:610: base::TimeDelta query_formulation_time = base::TimeTicks::Now() On 2012/07/30 20:24:02, Peter Kasting wrote: > Nit: Operators go on the ends of lines; break after '=' Done. https://chromiumcodereview.appspot.com/10829068/diff/1/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:613: base::StringPrintf(".%" PRIuS, On 2012/07/30 20:24:02, Peter Kasting wrote: > You've passed the format string for a size_t and then supplied an int64 as the > value. Done.
https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: if (template_url && match.search_terms_args.get()) { Nit: Comments about what you're doing? https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:613: *match.search_terms_args.get()); Nit: No need for get() https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:614: search_terms_args.assisted_query_stats += You unconditionally add to this. That seems questionable. Won't many matches have no assisted_query_stats? https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:615: base::StringPrintf(".%" WidePRIu64, Why is this using the wide format arg when the string is narrow? Why is this still using an unsigned specifier when the provided argument is signed? https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:621: match.transition, Nit: Why add a linebreak?
I waited for the tryserver results to all pass this time. https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: if (template_url && match.search_terms_args.get()) { On 2012/07/30 22:52:50, Peter Kasting wrote: > Nit: Comments about what you're doing? Done. https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:613: *match.search_terms_args.get()); On 2012/07/30 22:52:50, Peter Kasting wrote: > Nit: No need for get() Done. https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:614: search_terms_args.assisted_query_stats += On 2012/07/30 22:52:50, Peter Kasting wrote: > You unconditionally add to this. That seems questionable. Won't many matches > have no assisted_query_stats? I'm not sure since it seems to always be set in AutocompleteController::UpdateAssistedQueryStats() though I don't know if that covers all AutocompleteMatch instances. But adding a check doesn't hurt so I added it. https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:615: base::StringPrintf(".%" WidePRIu64, On 2012/07/30 22:52:50, Peter Kasting wrote: > Why is this using the wide format arg when the string is narrow? > > Why is this still using an unsigned specifier when the provided argument is > signed? Done. Sorry, I didn't read format_macros.h carefully and for some reason I thought it was a size_t and so it was unsigned. https://chromiumcodereview.appspot.com/10829068/diff/1002/chrome/browser/ui/o... chrome/browser/ui/omnibox/omnibox_edit_model.cc:621: match.transition, On 2012/07/30 22:52:50, Peter Kasting wrote: > Nit: Why add a linebreak? Done.
LGTM https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: if (template_url && match.search_terms_args.get()) { I think you should add "&& !match.search_terms_args->assisted_query_stats.empty()" to this conditional instead of doing it later; we don't actually need to do anything in here if that check fails. https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:610: // Append the query formulation time (time from when the user first typed Nit: The comment should probably go up above this conditional (i.e. at the top of the code you're adding).
Thanks Peter for all the quick reviews! I'll probably commit this later today. https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:609: if (template_url && match.search_terms_args.get()) { On 2012/07/31 02:35:01, Peter Kasting wrote: > I think you should add "&& > !match.search_terms_args->assisted_query_stats.empty()" to this conditional > instead of doing it later; we don't actually need to do anything in here if that > check fails. Done. https://chromiumcodereview.appspot.com/10829068/diff/5/chrome/browser/ui/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:610: // Append the query formulation time (time from when the user first typed On 2012/07/31 02:35:01, Peter Kasting wrote: > Nit: The comment should probably go up above this conditional (i.e. at the top > of the code you're adding). Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hfung@google.com/10829068/6
Change committed as 149326 |
