Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689803002:
Ensure nearby URL count metric is properly initialized (Closed)
|
Created:
3 years, 10 months ago by mattreynolds Modified:
3 years, 10 months ago Reviewers:
Mark P CC:
chromium-reviews, jdonnelly+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove nearby URL count metrics for Physical Web omnibox provider
The nearby_url_count_ field was not initialized correctly in the query
suggest case. This CL ensures that the field is always set to the
number of metadata items returned by the Physical Web data source.
A new histogram is added to record the number of nearby URLs when the
omnibox is focused. Another histogram is added to detect how frequently
this corner case occurs.
BUG=691059
Review-Url: https://codereview.chromium.org/2689803002
Cr-Commit-Position: refs/heads/master@{#452620}
Committed: https://chromium.googlesource.com/chromium/src/+/6b89f92fabae32da4caae8f1b0c387888ade9496
Patch Set 1 #
Total comments: 3
Patch Set 2 : add unit test, AtFocus histogram #
Total comments: 4
Patch Set 3 : split histogram into NearbyUrlCount.AtFocus and NearbyURLCount.AtMatchCreation #Patch Set 4 : add new histogram to detect corner case #
Total comments: 2
Patch Set 5 : npos #
Total comments: 24
Patch Set 6 : changes for mpearson@ #Patch Set 7 : fix errors #
Total comments: 4
Patch Set 8 : remove BeginOmniboxSession, re-add histogram description #
Messages
Total messages: 26 (6 generated)
mattreynolds@chromium.org changed reviewers: + mpearson@chromium.org
Hi Mark, PTAL Looks like I broke one of our metrics when I added query suggest. Is there a way to write components unit tests that check whether UMA was recorded?
On 2017/02/10 20:16:50, mattreynolds wrote: > Hi Mark, PTAL > > Looks like I broke one of our metrics when I added query suggest. Was the bug simply that nearby_url_count_ was uninitialized in the constructor? I actually don't see anything else that you fixed in the changelist that could cause an issue. Is there something I'm overlooking? > Is there a way > to write components unit tests that check whether UMA was recorded? Take a look at base/test/histogram_tester.h --mark https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:85: ConstructZeroSuggestMatches(std::move(metadata_list)); I don't understand this move operation. Then again, I haven't thought much about our smart pointer types in a while. Won't this move mean you clean up the pointer at the end of this call? Why is that what you want?
On 2017/02/11 05:17:39, Mark P wrote: > On 2017/02/10 20:16:50, mattreynolds wrote: > > Hi Mark, PTAL > > > > Looks like I broke one of our metrics when I added query suggest. > > Was the bug simply that nearby_url_count_ was uninitialized in the > constructor? I actually don't see anything else that you fixed in the > changelist that could cause an issue. Is there something I'm overlooking? The metric is intended to record "the number of nearby Physical Web URLs at the time the provider last constructed matches" so it should update in each case where we construct matches, but previously it was only set in the zero-suggest case. I moved the logic to PhysicalWebProvider::Start to make sure it wouldn't be overlooked if we added another Construct.*Matches method. > > Is there a way > > to write components unit tests that check whether UMA was recorded? > > Take a look at > base/test/histogram_tester.h Thanks! I'll ping you when I have a test for this. https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:85: ConstructZeroSuggestMatches(std::move(metadata_list)); On 2017/02/11 05:17:39, Mark P wrote: > I don't understand this move operation. Then again, I haven't thought much > about our smart pointer types in a while. Won't this move mean you clean up the > pointer at the end of this call? Why is that what you want? This preserves the same behavior as before. In the previous version, GetMetadataList returns a prvalue so it didn't need std::move. Now that it has identity, it is an xvalue and needs std::move. Essentially, I'm choosing to be explicit that the metadata list will not be accessed in PhysicalWebProvider::Start after we pass it to one of the Construct.*Matches methods. As-is, the metadata list should be released when Construct.*Matches returns. We could choose to pass a const reference instead and then it would be cleaned up when PhysicalWebProvider::Start returns.
On 2017/02/13 19:00:05, mattreynolds wrote: > On 2017/02/11 05:17:39, Mark P wrote: > > On 2017/02/10 20:16:50, mattreynolds wrote: > > > Hi Mark, PTAL > > > > > > Looks like I broke one of our metrics when I added query suggest. > > > > Was the bug simply that nearby_url_count_ was uninitialized in the > > constructor? I actually don't see anything else that you fixed in the > > changelist that could cause an issue. Is there something I'm overlooking? > > The metric is intended to record "the number of nearby Physical Web URLs at the > time the provider last constructed matches" Um, it was? :-P Histograms.xml says "The number of nearby Physical Web URLs when the user focused the omnibox." It seems to me that's what it's still doing. I'm not actually sure we want this bug fixed yet. I think it'd be useful to know the number of physical web suggestions available at the time of omnibox focus. The only difference between the current behavior and the "bug-fixed" behavior is when some physical web beacons appear to disappear while the user is editing the omnibox, right? If so, I think the current metric is better, as it's a better reflection on the number of URLs that the user might've been exposed to. The URLs that appear as-you-type may not appear at all (not matching) or appear but be easier to overlook or be outranked. If you want a count of URLs at the last time the constructor made matches, that's fine with me. But I think we shouldn't get rid of the current metric on the way. Am I understanding the situation right? --mark > so it should update in each case > where we construct matches, but previously it was only set in the zero-suggest > case. > I moved the logic to PhysicalWebProvider::Start to make sure it wouldn't > be overlooked if we added another Construct.*Matches method. > > > > Is there a way > > > to write components unit tests that check whether UMA was recorded? > > > > Take a look at > > base/test/histogram_tester.h > > Thanks! I'll ping you when I have a test for this. > > https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... > File components/omnibox/browser/physical_web_provider.cc (right): > > https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... > components/omnibox/browser/physical_web_provider.cc:85: > ConstructZeroSuggestMatches(std::move(metadata_list)); > On 2017/02/11 05:17:39, Mark P wrote: > > I don't understand this move operation. Then again, I haven't thought much > > about our smart pointer types in a while. Won't this move mean you clean up > the > > pointer at the end of this call? Why is that what you want? > > This preserves the same behavior as before. In the previous version, > GetMetadataList returns a prvalue so it didn't need std::move. Now that it has > identity, it is an xvalue and needs std::move. > > Essentially, I'm choosing to be explicit that the metadata list will not be > accessed in PhysicalWebProvider::Start after we pass it to one of the > Construct.*Matches methods. As-is, the metadata list should be released when > Construct.*Matches returns. We could choose to pass a const reference instead > and then it would be cleaned up when PhysicalWebProvider::Start returns.
> > The metric is intended to record "the number of nearby Physical Web URLs at > the > > time the provider last constructed matches" > > Um, it was? :-P > > Histograms.xml says "The number of nearby Physical Web URLs when the > user focused the omnibox." It seems to me that's what it's still doing. That description was written before we added after typing matches, and is inconsistent with the description in the source (see PWP::AddProviderInfo). Thanks for pointing it out, I'll update it. > I'm not actually sure we want this bug fixed yet. I think it'd be useful to > know the number of physical web suggestions available at the time of > omnibox focus. > > The only difference between the current behavior and the "bug-fixed" > behavior is when some physical web beacons appear to disappear while > the user is editing the omnibox, right? We're currently recording what appears to be uninitialized values (20k hits for the >=50 bucket is very suspect). I don't think there's anything to be gained by keeping the current behavior. > If so, I think the current metric is better, as it's a better reflection on the > number of URLs that the user might've been exposed to. The URLs that > appear as-you-type may not appear at all (not matching) or appear but > be easier to overlook or be outranked. If you want a count of URLs at > the last time the constructor made matches, that's fine with me. But I > think we shouldn't get rid of the current metric on the way. Makes sense. I'll record Omnibox.SuggestionUsed.NearbyURLCount.OnFocus if the last time we created matches was for the zero suggest case. Ideally we'd also know whether Physical Web suggestions were created but were outranked by other matches. IIUC that's currently not recorded, but it's something we'd like to add.
Hi Mark, I added the new histogram and a unit test, please take another look.
I've only had time to take a partial look. Here are preliminary comments: On 2017/02/13 20:07:38, mattreynolds wrote: > We're currently recording what appears to be uninitialized values (20k hits for > the >=50 bucket is very suspect). I agree that sounds unbelievable. Yet, I'm still having trouble understanding how that can happen. I see we don't set nearby_url_count_ on the ConstructQuerySuggestMatches() code path. It's only initialized on the ConstructZeroSuggestMatches() code path (as well as in incognito windows). Yet, I thought you offered zero suggest suggestions on every entry point to the omnibox, including the NTP and focussing the omnibox when on an arbitrary web page. In other words, I'd think that for a user to end up using an omnibox suggestion, the user would've had to have done an initial focus event that caused the physical web zero suggest suggestions to appear. This would initialize nearby_url_count_. What am I missing? I definitely want to make sure we don't have uninitialized variables running around, so I generally support this changelist, but I'm not yet convinced that what it's fixing is the root cause of the suspect count. add'l comments below --mark https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/1/components/omnibox/browser/... components/omnibox/browser/physical_web_provider.cc:85: ConstructZeroSuggestMatches(std::move(metadata_list)); On 2017/02/13 19:00:04, mattreynolds wrote: > On 2017/02/11 05:17:39, Mark P wrote: > > I don't understand this move operation. Then again, I haven't thought much > > about our smart pointer types in a while. Won't this move mean you clean up > the > > pointer at the end of this call? Why is that what you want? > > This preserves the same behavior as before. In the previous version, > GetMetadataList returns a prvalue so it didn't need std::move. Now that it has > identity, it is an xvalue and needs std::move. > > Essentially, I'm choosing to be explicit that the metadata list will not be > accessed in PhysicalWebProvider::Start after we pass it to one of the > Construct.*Matches methods. Ah, okay. Thanks for the explanation. > As-is, the metadata list should be released when > Construct.*Matches returns. We could choose to pass a const reference instead > and then it would be cleaned up when PhysicalWebProvider::Start returns. https://codereview.chromium.org/2689803002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:52: void BeginOmniboxSession(); It's not clear how / why this is different than AutocompleteProvider::ResetSession(), which you could override. https://codereview.chromium.org/2689803002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42842: + at 50. nit: either consider renaming this histogram per https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... or at least mention that in early versions of M-58 (and earlier?) uses of omnibox suggestions in the situation ... ended up causing emits to the 50+ bucket regardless of the actual value.
This changelist is still in your court, right? Let me know if you're waiting for me to take a more detailed look now. --mark On Tue, Feb 14, 2017 at 10:45 PM, <mpearson@chromium.org> wrote: > > I've only had time to take a partial look. Here are preliminary comments: > > On 2017/02/13 20:07:38, mattreynolds wrote: > > We're currently recording what appears to be uninitialized values (20k > hits > for > > the >=50 bucket is very suspect). > > I agree that sounds unbelievable. Yet, I'm still having trouble > understanding > how that can happen. I see we don't set nearby_url_count_ on the > ConstructQuerySuggestMatches() > code path. It's only initialized on the ConstructZeroSuggestMatches() code > path > (as well as in incognito windows). Yet, I thought you offered zero suggest > suggestions on every entry point to the omnibox, including the NTP and > focussing > the omnibox when on an arbitrary web page. > > In other words, I'd think that for a user to end up using an omnibox > suggestion, > the user would've had to have done an initial focus event that caused the > physical web zero suggest suggestions to appear. This would initialize > nearby_url_count_. > > What am I missing? > > I definitely want to make sure we don't have uninitialized variables > running > around, so I generally support this changelist, but I'm not yet convinced > that > what it's fixing is the root cause of the suspect count. > > add'l comments below > > --mark > > > > https://codereview.chromium.org/2689803002/diff/1/ > components/omnibox/browser/physical_web_provider.cc > File components/omnibox/browser/physical_web_provider.cc (right): > > https://codereview.chromium.org/2689803002/diff/1/ > components/omnibox/browser/physical_web_provider.cc#newcode85 > components/omnibox/browser/physical_web_provider.cc:85: > ConstructZeroSuggestMatches(std::move(metadata_list)); > On 2017/02/13 19:00:04, mattreynolds wrote: > > On 2017/02/11 05:17:39, Mark P wrote: > > > I don't understand this move operation. Then again, I haven't > thought much > > > about our smart pointer types in a while. Won't this move mean you > clean up > > the > > > pointer at the end of this call? Why is that what you want? > > > > This preserves the same behavior as before. In the previous version, > > GetMetadataList returns a prvalue so it didn't need std::move. Now > that it has > > identity, it is an xvalue and needs std::move. > > > > Essentially, I'm choosing to be explicit that the metadata list will > not be > > accessed in PhysicalWebProvider::Start after we pass it to one of the > > Construct.*Matches methods. > Ah, okay. Thanks for the explanation. > > > As-is, the metadata list should be released when > > Construct.*Matches returns. We could choose to pass a const reference > instead > > and then it would be cleaned up when PhysicalWebProvider::Start > returns. > > https://codereview.chromium.org/2689803002/diff/20001/ > components/omnibox/browser/physical_web_provider.h > File components/omnibox/browser/physical_web_provider.h (right): > > https://codereview.chromium.org/2689803002/diff/20001/ > components/omnibox/browser/physical_web_provider.h#newcode52 > components/omnibox/browser/physical_web_provider.h:52: void > BeginOmniboxSession(); > It's not clear how / why this is different than > AutocompleteProvider::ResetSession(), which you could override. > > https://codereview.chromium.org/2689803002/diff/20001/ > tools/metrics/histograms/histograms.xml > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/2689803002/diff/20001/ > tools/metrics/histograms/histograms.xml#newcode42842 > tools/metrics/histograms/histograms.xml:42842: + at 50. > nit: either consider renaming this histogram per > https://chromium.googlesource.com/chromium/src.git/+/HEAD/ > tools/metrics/histograms/README.md#Revising-Histograms > or at least mention that in early versions of M-58 (and earlier?) uses > of omnibox suggestions in the situation ... ended up causing emits to > the 50+ bucket regardless of the actual value. > > https://codereview.chromium.org/2689803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I don't know how those values were recorded. There must be some way to invoke the omnibox without setting from_omnibox_focus. I think this is possible on desktop by pasting into the omnibox, but I haven't found any way to do it on mobile. I assume we don't record metrics during test runs. But if we do, it's possible that tests sometimes call AddProviderInfo() before Start(), or use an AutocompleteInput that triggers query suggest instead of zero suggest. Any ideas? https://codereview.chromium.org/2689803002/diff/20001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/20001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:52: void BeginOmniboxSession(); On 2017/02/15 06:45:53, Mark P wrote: > It's not clear how / why this is different than > AutocompleteProvider::ResetSession(), which you could override. ResetSession doesn't get called on focus, so it doesn't work for us. https://codereview.chromium.org/2689803002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42842: + at 50. On 2017/02/15 06:45:54, Mark P wrote: > nit: either consider renaming this histogram per > https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histo... > or at least mention that in early versions of M-58 (and earlier?) uses of > omnibox suggestions in the situation ... ended up causing emits to the 50+ > bucket regardless of the actual value. > Renamed to Omnibox.SuggestionUsed.NearbyURLCount.AtMatchCreation
On 2017/02/17 03:08:12, mattreynolds wrote: > I don't know how those values were recorded. There must be some way to invoke > the omnibox without setting from_omnibox_focus. I think this is possible on > desktop by pasting into the omnibox, but I haven't found any way to do it on > mobile. I recently noticed that on desktop, when you create a new tab and the omnibox is focussed by default, it doesn't trigger a Start(on_focus) event. Maybe something similar can happen on android? Note I think this is *only* an android issue. For Android canary/dev users who are part of the experiment, about 10% emit to the 50+ bucket. For iOS canary/dev, no users emit to the iOS bucket (and I think we have enough users that the lack of users in that bucket is indicative). > I assume we don't record metrics during test runs. Correct. I'd encourage you spend some time trying to figure out this lack of focus(>1 hour). This might indicate a deeper bug somewhere. > But if we do, it's possible > that tests sometimes call AddProviderInfo() before Start(), or use an > AutocompleteInput that triggers query suggest instead of zero suggest. > > Any ideas? Try investigating voice search? Also, other comments: * I don't see the histogram rename in this latest patchset. Did you forget to upload a new patchset? * Please initialize the count variables to something like npos, not 0. 0 indicates a real value that we understand. - If we can't figure out the weird counts, we should also revise the emitting code not to emit when the value is npos. And we should revise the histogram descriptions explaining that sometimes we don't emit if this weird situation happens. --mark
On Fri, Feb 17, 2017 at 5:07 PM, <mpearson@chromium.org> wrote: > - If we can't figure out the weird counts, we should also revise the > emitting > code not to emit when the value is npos. And we should revise the histogram > descriptions explaining that sometimes we don't emit if this weird > situation > happens. > Actually, correct this: if we can't figure out the weird counts, we should emit them to a histogram (either this or another) so we can keep an eye on the magnitude of the issue. We'd like to know if it changes. --mark > > --mark > > > https://codereview.chromium.org/2689803002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> Did you forget to upload a new patchset? Oops, yes. > Please initialize the count variables to something like npos, not 0. Now I'm initializing to kInvalidUrlCount. I tried using SIZE_MAX, but it doesn't play nicely with the histogram (it gets recorded into bucket 0 instead of the overflow bucket). > if we can't figure out the weird counts, we should emit them to a histogram (either this or another) so we can keep an eye on the magnitude of the issue. Good idea. Now we record to Omnibox.PhysicalWebProvider.SuggestionUsedWithoutOmniboxFocus (records true if the corner case is hit, false otherwise). I don't think there's any benefit in storing the invalid URL count values.
Description was changed from ========== Ensure nearby URL count metric is properly initialized The nearby_url_count_ field was not initialized correctly in the query suggest case. This CL ensures that the field is always set to the number of metadata items returned by the Physical Web data source. BUG=691059 ========== to ========== Improve nearby URL count metrics for Physical Web omnibox provider The nearby_url_count_ field was not initialized correctly in the query suggest case. This CL ensures that the field is always set to the number of metadata items returned by the Physical Web data source. A new histogram is added to record the number of nearby URLs when the omnibox is focused. Another histogram is added to detect how frequently this corner case occurs. BUG=691059 ==========
On 2017/02/21 23:45:07, mattreynolds wrote: > > Did you forget to upload a new patchset? > > Oops, yes. > > > Please initialize the count variables to something like npos, not 0. > > Now I'm initializing to kInvalidUrlCount. I tried using SIZE_MAX, but it doesn't > play nicely with the histogram (it gets recorded into bucket 0 instead of the > overflow bucket). > > > if we can't figure out the weird counts, we should emit them to a histogram > (either this or another) so we can keep an eye on the magnitude of the issue. > > Good idea. Now we record to > Omnibox.PhysicalWebProvider.SuggestionUsedWithoutOmniboxFocus (records true if > the corner case is hit, false otherwise). I don't think there's any benefit in > storing the invalid URL count values. Adding that histogram sounds good to me. Did you try the VOICE_SEARCH experiment I suggested in an earlier patchset or are you uninterested in digging deeper here? Let me know. --mark https://codereview.chromium.org/2689803002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:91: size_t nearby_url_count_at_focus_; Can you use npos and test for it explicitly rather than having a second field to detect validity?
I investigated Voice Search but couldn't get it to trigger the corner case. Voice Search invokes the provider several times before showing the suggestion list. But, it does invoke the provider once with from_omnibox_focus == true which is sufficient to initialize the onFocus URL count. For a test query "page", it called PhysicalWebProvider::Start seven times before showing suggestions: input == "page", from_omnibox_focus == false input == "Paige", from_omnibox_focus == false input == "Peach", from_omnibox_focus == false input == "beige", from_omnibox_focus == false input == "Beach", from_omnibox_focus == false input == "page", from_omnibox_focus == true input == "page", from_omnibox_focus == false Selecting a suggestion records the nearby URL count at the time of the 6th call (with from_omnibox_focus == true). https://codereview.chromium.org/2689803002/diff/60001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/60001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:91: size_t nearby_url_count_at_focus_; On 2017/02/22 06:25:39, Mark P wrote: > Can you use npos and test for it explicitly rather than having a second field to > detect validity? Sure, done.
generally looks fine to me; just a series of minor comments. Sorry that this review has taken so long. --mark https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:59: const bool input_from_focus = input.from_omnibox_focus(); optional nit: I don't think this alias buys you much. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:62: if (input_from_focus) { nit: no need for { } here https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:66: // Don't provide suggestions in incognito mode. nit: combine 66-72 with 74-81, as the result is the same. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:70: nearby_url_count_at_focus_ = 0; Should this had_physical_web_suggestions_at_focus_or_later_ = false be in this block too? https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:85: if (input_from_focus) { nit: {} unnecessary https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:167: // focus will be invalid because the omnibox was never focused. In an earlier omit the sentence "In an earlier version" and revise the following sentence (perhaps "Now we guard" -> "We guard"). No one reading the code needs to know the history. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:173: bool suggestion_used_without_focus = nit: const https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:211: // session begins. Per-session metrics are recorded in AddProviderInfo. Add something like // Both these variables are set properly later within the Start() flow. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:86: // matches. Please say something about npos here and below. https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43075: -<histogram name="Omnibox.SuggestionUsed.NearbyURLCount" units="URLs"> Please don't delete hold histograms; instead mark them as obsolete. https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42930: + invoked by focusing the omnibox during the current omnibox session. Recorded Between the first and second sentence, please add something like: "This is unexpected; it's probably a bug somewhere." https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43109: + at 50. Please add a sentence pointing people to Omnibox.PhysicalWebProvider.SuggestionUsedWithoutOmniboxFocus (and why).
Thanks Mark! https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:59: const bool input_from_focus = input.from_omnibox_focus(); On 2017/02/23 00:38:59, Mark P wrote: > optional nit: I don't think this alias buys you much. Removed https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:62: if (input_from_focus) { On 2017/02/23 00:38:59, Mark P wrote: > nit: no need for { } here Removed https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:66: // Don't provide suggestions in incognito mode. On 2017/02/23 00:38:59, Mark P wrote: > nit: combine 66-72 with 74-81, as the result is the same. Done. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:70: nearby_url_count_at_focus_ = 0; On 2017/02/23 00:38:59, Mark P wrote: > Should this > had_physical_web_suggestions_at_focus_or_later_ = false > be in this block too? Yep, added https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:85: if (input_from_focus) { On 2017/02/23 00:38:59, Mark P wrote: > nit: {} unnecessary Removed https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:167: // focus will be invalid because the omnibox was never focused. In an earlier On 2017/02/23 00:38:59, Mark P wrote: > omit the sentence "In an earlier version" and revise the following sentence > (perhaps "Now we guard" -> "We guard"). No one reading the code needs to know > the history. Done. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:173: bool suggestion_used_without_focus = On 2017/02/23 00:38:59, Mark P wrote: > nit: const Done. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.cc:211: // session begins. Per-session metrics are recorded in AddProviderInfo. On 2017/02/23 00:38:59, Mark P wrote: > Add something like > // Both these variables are set properly later within the Start() flow. Done. https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... File components/omnibox/browser/physical_web_provider.h (right): https://codereview.chromium.org/2689803002/diff/80001/components/omnibox/brow... components/omnibox/browser/physical_web_provider.h:86: // matches. On 2017/02/23 00:38:59, Mark P wrote: > Please say something about npos here and below. Done. https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43075: -<histogram name="Omnibox.SuggestionUsed.NearbyURLCount" units="URLs"> On 2017/02/23 00:38:59, Mark P wrote: > Please don't delete hold histograms; instead mark them as obsolete. Added it back with an <obsolete> block https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:42930: + invoked by focusing the omnibox during the current omnibox session. Recorded On 2017/02/23 00:38:59, Mark P wrote: > Between the first and second sentence, please add something like: "This is > unexpected; it's probably a bug somewhere." Done. https://codereview.chromium.org/2689803002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:43109: + at 50. On 2017/02/23 00:38:59, Mark P wrote: > Please add a sentence pointing people to > Omnibox.PhysicalWebProvider.SuggestionUsedWithoutOmniboxFocus (and why). Done
lgtm modulo two comments, including one big one If you agree with me, feel free to make the appropriate revisions and submit. If you disagree, please wait for another back-and-forth with me about the changelist. cheers, mark https://codereview.chromium.org/2689803002/diff/120001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:205: // focuses the omnibox. Now that I read this code closer, I don't think this function BeginOmniboxSession is necessary. I think you can drop it entirely. Start() looks like if (focus) set these variables (via BeginOmniboxSession) if (special case) { set these variables return } if (focus) set these variables } else { .. } There is no code path on which these variables don't get overridden later within the same function call, implying that setting them here is not needed. https://codereview.chromium.org/2689803002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43114: + </obsolete> Please leave the original description. (maybe revise it for clarity about what we understand it was actually measuring)
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2689803002/#ps140001 (title: "remove BeginOmniboxSession, re-add histogram description")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks again! https://codereview.chromium.org/2689803002/diff/120001/components/omnibox/bro... File components/omnibox/browser/physical_web_provider.cc (right): https://codereview.chromium.org/2689803002/diff/120001/components/omnibox/bro... components/omnibox/browser/physical_web_provider.cc:205: // focuses the omnibox. On 2017/02/23 05:09:49, Mark P wrote: > Now that I read this code closer, I don't think this function > BeginOmniboxSession is necessary. I think you can drop it entirely. > > Start() looks like > if (focus) > set these variables (via BeginOmniboxSession) > if (special case) { > set these variables > return > } > if (focus) > set these variables > } else { > .. > } > > > There is no code path on which these variables don't get overridden later within > the same function call, implying that setting them here is not needed. Makes sense, removed. https://codereview.chromium.org/2689803002/diff/120001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2689803002/diff/120001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:43114: + </obsolete> On 2017/02/23 05:09:49, Mark P wrote: > Please leave the original description. > (maybe revise it for clarity about what we understand it was actually measuring) Done.
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487878852281190,
"parent_rev": "641c52c72d465295087d9a937dab9dea7333aecb", "commit_rev":
"6b89f92fabae32da4caae8f1b0c387888ade9496"}
Message was sent while issue was closed.
Description was changed from ========== Improve nearby URL count metrics for Physical Web omnibox provider The nearby_url_count_ field was not initialized correctly in the query suggest case. This CL ensures that the field is always set to the number of metadata items returned by the Physical Web data source. A new histogram is added to record the number of nearby URLs when the omnibox is focused. Another histogram is added to detect how frequently this corner case occurs. BUG=691059 ========== to ========== Improve nearby URL count metrics for Physical Web omnibox provider The nearby_url_count_ field was not initialized correctly in the query suggest case. This CL ensures that the field is always set to the number of metadata items returned by the Physical Web data source. A new histogram is added to record the number of nearby URLs when the omnibox is focused. Another histogram is added to detect how frequently this corner case occurs. BUG=691059 Review-Url: https://codereview.chromium.org/2689803002 Cr-Commit-Position: refs/heads/master@{#452620} Committed: https://chromium.googlesource.com/chromium/src/+/6b89f92fabae32da4caae8f1b0c3... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6b89f92fabae32da4caae8f1b0c3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
