Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11316057:
Omnibox: Log Elapsed Time Since Last Time The Default Match Changed (Closed)
|
Created:
8 years, 1 month ago by Mark P Modified:
8 years, 1 month ago CC:
chromium-reviews, MAD, James Su, Ilya Sherman, jar (doing other things), Bart N. Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionOmnibox: Log Elapsed Time Since Last Time The Default Match Changed
For users opted-in in UMA, in the logs that record interactions with the omnibox. keep track of the length of time the default match has been displayed.
BUG=
TEST=by hand, using LOG(INFO)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168913
Patch Set 1 #
Total comments: 36
Patch Set 2 : Feedback comments. #
Total comments: 14
Patch Set 3 : Latest iteration. #Patch Set 4 : rebase #
Messages
Total messages: 15 (0 generated)
The first patch in my quest to get measurement of unexpected behavior / jank in the omnibox. Please take a look. thanks, mark
https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:217: UpdateResult(false, true); // force saying the default match has changed Nit: Comment adds nothing over the function declaration. Say why we want to force notification here. (Something like what I said in my email to you yesterday.) https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:271: // matches we have to query the providers again.) I'm not sure this parenthetical comment really belongs here. I think maybe we want this on the UpdateResult() declaration, or maybe in the definition. Basically we want to note that any existing result may contain (a) stale matches from the previous result set and (b) these results may have actually outranked legitimate matches from the current result set, so to get just the current matches, it's easiest to simply throw everything out and then reconstruct a result set from the providers' current data. https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:273: // if it did not). Again, this just says what we're doing; we need to instead say why. (I'm not actually 100% sure we need to force-notify in this case. I know we used to but that may have been unnecessary.) https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:288: // update the results if we're not in Start(). Nit: Line wrapping change unnecessary https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:308: bool regenerate_result, bool force_notify_default_match_changed) { Nit: One arg per line (your style is legal, but doesn't match the autocomplete code style) https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:312: string16 last_default_fill_into_edit(ASCIIToUTF16("")); Nit: remove "(ASCIIToUTF16(""))" as it does nothing (3 places) https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:365: (default_is_valid && last_default_was_valid && Nit: Adding "last_default_was_valid &&" is unnecessary because it's guaranteed to be true based on the previous portion of the conditional. https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:369: if (notify_default_match) { Nit: {} unnecessary https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... File chrome/browser/autocomplete/autocomplete_controller.h (right): https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.h:129: base::TimeTicks last_time_default_match_changed() const { Nit: return class objects by const & where possible? https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.h:155: // default match has changed even if it hasn't. Nit: May want to motivate this last sentence with why we'd do this https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.h:206: // appeared as the default match.) This also be me more recent than the Nit: extra "me" https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.h:209: // when we consider a match has changed. Nit: has -> to have https://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omn... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): https://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omn... chrome/browser/ui/omnibox/omnibox_edit_model.cc:563: DCHECK(log.elapsed_time_since_user_first_modified_omnibox >= Nit: DCHECK_GE
http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_controller.h (right): http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:205: // corresponds to the first instance https://mail.google.com/ first Nit: remove second "first". http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:206: // appeared as the default match.) This also be me more recent than the Nit: move dot outside parenthese. http://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:555: base::TimeTicks::Now() - There is some inconsistency here, because you call Now() twice. A better approach would be to set Now() to a local variable and use it here and above. http://codereview.chromium.org/11316057/diff/1/chrome/common/metrics/proto/om... File chrome/common/metrics/proto/omnibox_event.proto (right): http://codereview.chromium.org/11316057/diff/1/chrome/common/metrics/proto/om... chrome/common/metrics/proto/omnibox_event.proto:57: // provider. Regardless, it should always be less than the field less -> less or equal?
Thanks for the feedback and encouragement to better comment the reasons the code works the way it does. --mark http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_controller.cc (right): http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:217: UpdateResult(false, true); // force saying the default match has changed On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: Comment adds nothing over the function declaration. Say why we want to > force notification here. (Something like what I said in my email to you > yesterday.) Expanded greatly, new with useful information (largely pasted from your e-mail). http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:271: // matches we have to query the providers again.) On 2012/11/16 23:57:18, Peter Kasting wrote: > I'm not sure this parenthetical comment really belongs here. > > I think maybe we want this on the UpdateResult() declaration, or maybe in the > definition. Basically we want to note that any existing result may contain (a) > stale matches from the previous result set and (b) these results may have > actually outranked legitimate matches from the current result set, so to get > just the current matches, it's easiest to simply throw everything out and then > reconstruct a result set from the providers' current data. Moved to the UpdateResults declaration, largely rewritten per your suggestion. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:273: // if it did not). On 2012/11/16 23:57:18, Peter Kasting wrote: > Again, this just says what we're doing; we need to instead say why. > > (I'm not actually 100% sure we need to force-notify in this case. I know we > used to but that may have been unnecessary.) Added somewhat of an explanation, largely culled from your e-mail. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:288: // update the results if we're not in Start(). On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: Line wrapping change unnecessary Fine, put back. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:308: bool regenerate_result, bool force_notify_default_match_changed) { On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: One arg per line (your style is legal, but doesn't match the autocomplete > code style) Okay, fixed. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:312: string16 last_default_fill_into_edit(ASCIIToUTF16("")); On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: remove "(ASCIIToUTF16(""))" as it does nothing (3 places) Fixed all three. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:365: (default_is_valid && last_default_was_valid && On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: Adding "last_default_was_valid &&" is unnecessary because it's guaranteed > to be true based on the previous portion of the conditional. I know. However, I think it makes the logic cleaner especially because I said the two string16 last_default_... strings are only valid if |last_default_was_valid|. I like making it obvious that |last_default_was_valid| is true when I'm going to use those last_default_... strings. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.cc:369: if (notify_default_match) { On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: {} unnecessary Removed. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... File chrome/browser/autocomplete/autocomplete_controller.h (right): http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:129: base::TimeTicks last_time_default_match_changed() const { On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: return class objects by const & where possible? Good idea. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:155: // default match has changed even if it hasn't. On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: May want to motivate this last sentence with why we'd do this Done. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:205: // corresponds to the first instance https://mail.google.com/ first On 2012/11/19 18:02:21, Bart N. wrote: > Nit: remove second "first". Done. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:206: // appeared as the default match.) This also be me more recent than the On 2012/11/19 18:02:21, Bart N. wrote: > Nit: move dot outside parenthese. Let's as-is. The parenthetical remark is a full sentence. http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:206: // appeared as the default match.) This also be me more recent than the On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: extra "me" Removed "me" http://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/au... chrome/browser/autocomplete/autocomplete_controller.h:209: // when we consider a match has changed. On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: has -> to have Done. http://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omni... File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right): http://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:555: base::TimeTicks::Now() - On 2012/11/19 18:02:21, Bart N. wrote: > There is some inconsistency here, because you call Now() twice. A better > approach would be to set Now() to a local variable and use it here and above. I was wondering if someone would call me out on this. Switched to using a temporary local variable. http://codereview.chromium.org/11316057/diff/1/chrome/browser/ui/omnibox/omni... chrome/browser/ui/omnibox/omnibox_edit_model.cc:563: DCHECK(log.elapsed_time_since_user_first_modified_omnibox >= On 2012/11/16 23:57:18, Peter Kasting wrote: > Nit: DCHECK_GE Can't use a DCHECK_GE because DCHECK using << to display its parameters to an output stream, and these TimeTicks variables have no toString() function. http://codereview.chromium.org/11316057/diff/1/chrome/common/metrics/proto/om... File chrome/common/metrics/proto/omnibox_event.proto (right): http://codereview.chromium.org/11316057/diff/1/chrome/common/metrics/proto/om... chrome/common/metrics/proto/omnibox_event.proto:57: // provider. Regardless, it should always be less than the field On 2012/11/19 18:02:21, Bart N. wrote: > less -> less or equal? Good point. Revised.
http://codereview.chromium.org/11316057/diff/5002/chrome/browser/metrics/metr... File chrome/browser/metrics/metrics_log.cc (right): http://codereview.chromium.org/11316057/diff/5002/chrome/browser/metrics/metr... chrome/browser/metrics/metrics_log.cc:982: log.elapsed_time_since_user_first_modified_omnibox.InMilliseconds()); Looks like you're using the same value here and on line 979 -- I'm assuming you meant to use a different one here?
https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:365: (default_is_valid && last_default_was_valid && On 2012/11/19 22:21:45, Mark P wrote: > On 2012/11/16 23:57:18, Peter Kasting wrote: > > Nit: Adding "last_default_was_valid &&" is unnecessary because it's guaranteed > > to be true based on the previous portion of the conditional. > > I know. However, I think it makes the logic cleaner especially because I said > the two string16 last_default_... strings are only valid if > |last_default_was_valid|. I like making it obvious that > |last_default_was_valid| is true when I'm going to use those last_default_... > strings. In that case what about removing the "default_is_valid" condition instead? https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:217: // This is triggers to the edit model to update things such as the Nit: This is triggers to the edit -> This triggers the edit https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:280: // generate them, thus ensuring that no results from the previous Nit: generate -> regenerate https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:286: // too often because it hasn't had any major side effects. Wait a minute, looking again at the code before, it doesn't look like we actually used to force-notify. Is my brain just muddled because it's late? Or did I misread things before? Because if not then it doesn't seem like we need to pass true for the second arg here. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.h (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.h:213: // corresponds to the first instance https://mail.google.com/ Nit: How about "then this is the time that URL first appeared as the default match." https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.h:214: // appeared as the default match.) This also be more recent than the Nit: This also -> This may also
Getting there... --mark https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/1/chrome/browser/autocomplete/a... chrome/browser/autocomplete/autocomplete_controller.cc:365: (default_is_valid && last_default_was_valid && On 2012/11/20 06:49:19, Peter Kasting wrote: > On 2012/11/19 22:21:45, Mark P wrote: > > On 2012/11/16 23:57:18, Peter Kasting wrote: > > > Nit: Adding "last_default_was_valid &&" is unnecessary because it's > guaranteed > > > to be true based on the previous portion of the conditional. > > > > I know. However, I think it makes the logic cleaner especially because I said > > the two string16 last_default_... strings are only valid if > > |last_default_was_valid|. I like making it obvious that > > |last_default_was_valid| is true when I'm going to use those last_default_... > > strings. > > In that case what about removing the "default_is_valid" condition instead? I'm comfortable with that. Done. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:217: // This is triggers to the edit model to update things such as the On 2012/11/20 06:49:19, Peter Kasting wrote: > Nit: This is triggers to the edit -> This triggers the edit Done. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:280: // generate them, thus ensuring that no results from the previous On 2012/11/20 06:49:19, Peter Kasting wrote: > Nit: generate -> regenerate Done. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:286: // too often because it hasn't had any major side effects. On 2012/11/20 06:49:19, Peter Kasting wrote: > Wait a minute, looking again at the code before, it doesn't look like we > actually used to force-notify. Is my brain just muddled because it's late? Or > did I misread things before? I believe the old code, by clearing the results before calling UpdateResults(), ended up with a notification. (The last default match will be not valid, and presumably we'll have a default match in the current set from the synchronous reply to the search provider, so we'll always think the default match has changed and notify.) > > Because if not then it doesn't seem like we need to pass true for the second arg > here. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.h (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.h:213: // corresponds to the first instance https://mail.google.com/ On 2012/11/20 06:49:19, Peter Kasting wrote: > Nit: How about "then this is the time that URL first appeared as the default > match." Done. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.h:214: // appeared as the default match.) This also be more recent than the On 2012/11/20 06:49:19, Peter Kasting wrote: > Nit: This also -> This may also Done. https://codereview.chromium.org/11316057/diff/5002/chrome/browser/metrics/met... File chrome/browser/metrics/metrics_log.cc (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/metrics/met... chrome/browser/metrics/metrics_log.cc:982: log.elapsed_time_since_user_first_modified_omnibox.InMilliseconds()); On 2012/11/20 01:44:30, Ilya Sherman wrote: > Looks like you're using the same value here and on line 979 -- I'm assuming you > meant to use a different one here? Good catch! Fixed.
metrics/ changes LGTM
LGTM other than below https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:286: // too often because it hasn't had any major side effects. On 2012/11/20 17:34:41, Mark P wrote: > On 2012/11/20 06:49:19, Peter Kasting wrote: > > Wait a minute, looking again at the code before, it doesn't look like we > > actually used to force-notify. Is my brain just muddled because it's late? > Or > > did I misread things before? > > I believe the old code, by clearing the results before calling UpdateResults(), > ended up with a notification. (The last default match will be not valid, and > presumably we'll have a default match in the current set from the synchronous > reply to the search provider, so we'll always think the default match has > changed and notify.) Ah, you're right. Still, I wonder if we can't just go ahead and try to not notify here. Unlike with the "notify during the synchronous pass" case, I can't think of anything that this would break, and I get really uncomfortable when we have code that no one understands and we think is likely wrong but we don't touch. If you're nervous about this we could always do it as a separate change so it's easy to revert if something goes wrong.
https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... File chrome/browser/autocomplete/autocomplete_controller.cc (right): https://codereview.chromium.org/11316057/diff/5002/chrome/browser/autocomplet... chrome/browser/autocomplete/autocomplete_controller.cc:286: // too often because it hasn't had any major side effects. On 2012/11/20 20:59:50, Peter Kasting wrote: > On 2012/11/20 17:34:41, Mark P wrote: > > On 2012/11/20 06:49:19, Peter Kasting wrote: > > > Wait a minute, looking again at the code before, it doesn't look like we > > > actually used to force-notify. Is my brain just muddled because it's late? > > Or > > > did I misread things before? > > > > I believe the old code, by clearing the results before calling > UpdateResults(), > > ended up with a notification. (The last default match will be not valid, and > > presumably we'll have a default match in the current set from the synchronous > > reply to the search provider, so we'll always think the default match has > > changed and notify.) > > Ah, you're right. > > Still, I wonder if we can't just go ahead and try to not notify here. Unlike > with the "notify during the synchronous pass" case, I can't think of anything > that this would break, and I get really uncomfortable when we have code that no > one understands and we think is likely wrong but we don't touch. > > If you're nervous about this we could always do it as a separate change so it's > easy to revert if something goes wrong. I will do this in a separate changelist. How do you propose I test that? Use the build for ten minutes and see if anything goes wrong? If not, check it in and watch for reports?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/11316057/2006
Failed to apply patch for chrome/browser/ui/omnibox/omnibox_edit_model.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/omnibox/omnibox_edit_model.cc
Hunk #1 succeeded at 537 (offset -4 lines).
Hunk #2 FAILED at 551.
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/omnibox/omnibox_edit_model.cc.rej
Patch: chrome/browser/ui/omnibox/omnibox_edit_model.cc
Index: chrome/browser/ui/omnibox/omnibox_edit_model.cc
diff --git a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
index
188611d51c8d59bf58b73013b81d2d9bb9549725..76d5c3a68582afa17bad173a610046ad472fcf07
100644
--- a/chrome/browser/ui/omnibox/omnibox_edit_model.cc
+++ b/chrome/browser/ui/omnibox/omnibox_edit_model.cc
@@ -541,6 +541,7 @@ void OmniboxEditModel::OpenMatch(const AutocompleteMatch&
match,
// We only care about cases where there is a selection (i.e. the popup is
// open).
if (popup_->IsOpen()) {
+ const base::TimeTicks& now(base::TimeTicks::Now());
// TODO(sreeram): Handle is_temporary_text_set_by_instant_ correctly.
AutocompleteLog log(
autocomplete_controller_->input().text(),
@@ -550,14 +551,20 @@ void OmniboxEditModel::OpenMatch(const AutocompleteMatch&
match,
-1, // don't yet know tab ID; set later if appropriate
ClassifyPage(controller_->GetTabContents()->
web_contents()->GetURL()),
- base::TimeTicks::Now() - time_user_first_modified_omnibox_,
+ now - time_user_first_modified_omnibox_,
string16::npos, // inline autocomplete length; possibly set later
+ now - autocomplete_controller_->last_time_default_match_changed(),
result());
DCHECK(user_input_in_progress_ ||
match.provider->type() == AutocompleteProvider::TYPE_ZERO_SUGGEST)
<< "We didn't get here through the expected series of calls. "
<< "time_user_first_modified_omnibox_ is not set correctly and other "
<< "things may be wrong. Match provider: " <<
match.provider->GetName();
+ DCHECK(log.elapsed_time_since_user_first_modified_omnibox >=
+ log.elapsed_time_since_last_change_to_default_match)
+ << "We should've got the notification that the user modified the "
+ << "omnibox text at same time or before the most recent time the "
+ << "default match changed.";
if (index != OmniboxPopupModel::kNoMatch)
log.selected_index = index;
else if (!has_temporary_text_)
On 2012/11/20 21:05:34, Mark P wrote: > How do you propose I test that? Use the build for ten minutes and see if > anything goes wrong? If not, check it in and watch for reports? Yeah, I don't really have a better suggestion.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mpearson@chromium.org/11316057/7011
Change committed as 168913 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
