Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
264123002:
Google Now Message Center Stats (Closed)
|
Created:
6 years, 7 months ago by robliao Modified:
6 years, 7 months ago CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org, rgustafson Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGoogle Now Message Center Stats
Provide support for counting Google Now cards in the message center.
BUG=369754
TBR=xiyuan@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269035
Patch Set 1 #
Total comments: 18
Patch Set 2 : CR Feedback #
Total comments: 6
Patch Set 3 : Text Update #Patch Set 4 : Histogram Update #
Total comments: 11
Patch Set 5 : String and Constant Update #Patch Set 6 : Label update #Patch Set 7 : Wording Update #Patch Set 8 : Refactor #
Total comments: 6
Patch Set 9 : Rename #Messages
Total messages: 52 (0 generated)
isherman, dewittj: Please provide owner approval for these files. Thanks! isherman: tools/metrics/actions/actions.xml tools/metrics/histograms/histograms.xml dewittj: chrome/browser/notifications/google_now_notification_stats_collector.h chrome/browser/notifications/google_now_notification_stats_collector.cc chrome/browser/notifications/message_center_notification_manager.h chrome/browser/notifications/message_center_notification_manager.cc ui/message_center/fake_message_center.h ui/message_center/fake_message_center.cc ui/message_center/message_center.h ui/message_center/message_center_impl.h ui/message_center/message_center_impl.cc ui/message_center/message_center_observer.h ui/message_center/views/message_popup_collection.cc
Would you mind counting the # of non-Now notifications as well? Also this lacks a Mac implementation of PoppedUpNotification.
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( I don't think you need a sparse histogram for this. One of the COUNTS histograms should do nicely. https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:67: iter++) { ++iter https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... tools/metrics/actions/actions.xml:2896: <description>Please enter the description of this user action.</description> ^^^ https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:7332: + units="count"> Optional nit: No need to specify a super generic unit like "count", though you can if you really want to.
On 2014/05/05 21:37:59, Ilya Sherman wrote: > https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... > File chrome/browser/notifications/google_now_notification_stats_collector.cc > (right): > > https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... > chrome/browser/notifications/google_now_notification_stats_collector.cc:42: > UMA_HISTOGRAM_SPARSE_SLOWLY( > I don't think you need a sparse histogram for this. One of the COUNTS > histograms should do nicely. > > https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... > chrome/browser/notifications/google_now_notification_stats_collector.cc:67: > iter++) { > ++iter > > https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... > File tools/metrics/actions/actions.xml (right): > > https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... > tools/metrics/actions/actions.xml:2896: <description>Please enter the > description of this user action.</description> > ^^^ > > https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... > tools/metrics/histograms/histograms.xml:7332: + units="count"> > Optional nit: No need to specify a super generic unit like "count", though you > can if you really want to. dewittj: How would you like the non-now cards to be recorded? The UMA point is 'Now' specific right now. We could use the same name like GoogleNow.MessageCenter.Displayed.NonNowNotificationsVisible or something to that effect.
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( We want to avoid bucketing if at all possible. How should we set up HISTOGRAM_CUSTOM_COUNTS to do this? On 2014/05/05 21:37:59, Ilya Sherman wrote: > I don't think you need a sparse histogram for this. One of the COUNTS > histograms should do nicely. https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:67: iter++) { On 2014/05/05 21:37:59, Ilya Sherman wrote: > ++iter Done. https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/264123002/diff/1/tools/metrics/actions/action... tools/metrics/actions/actions.xml:2896: <description>Please enter the description of this user action.</description> :-). Was blending in here. On 2014/05/05 21:37:59, Ilya Sherman wrote: > ^^^ https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:7332: + units="count"> Others were and it's nice to be specific. On 2014/05/05 21:37:59, Ilya Sherman wrote: > Optional nit: No need to specify a super generic unit like "count", though you > can if you really want to. https://codereview.chromium.org/264123002/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/264123002/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1499: 'browser/policy/cloud/user_policy_signin_service_base.cc', Sync Artifacts.
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2014/05/05 22:39:28, robliao wrote: > We want to avoid bucketing if at all possible. How should we set up > HISTOGRAM_CUSTOM_COUNTS to do this? Why do you want to avoid bucketing? What sorts of values are you trying to measure? If you're pretty confident that a user would never have more than, say, 10 notifications, and you only want the corresponding 10 buckets, that's fine; but in that case you should impose a maximum limit and emit to an overflow bucket if that limit is exceeded. https://codereview.chromium.org/264123002/diff/20001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/264123002/diff/20001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:2898: This action is triggered when the notification pops up. nit: I'd rephrase this to something more like "A Google Now notification was shown to the user." https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. Which message center, btw? Is the message center specific to Google Now, or is it shared with other notifications? If it's shared, is this counting just the Google Now cards among the notifications? Please clarify this in the summary text.
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( We're interested in measuring the following specific numbers 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ The 10+ may partially expand if there are a lot of 10+ numbers. On 2014/05/05 23:27:36, Ilya Sherman wrote: > On 2014/05/05 22:39:28, robliao wrote: > > We want to avoid bucketing if at all possible. How should we set up > > HISTOGRAM_CUSTOM_COUNTS to do this? > > Why do you want to avoid bucketing? What sorts of values are you trying to > measure? If you're pretty confident that a user would never have more than, > say, 10 notifications, and you only want the corresponding 10 buckets, that's > fine; but in that case you should impose a maximum limit and emit to an overflow > bucket if that limit is exceeded. https://codereview.chromium.org/264123002/diff/20001/tools/metrics/actions/ac... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/264123002/diff/20001/tools/metrics/actions/ac... tools/metrics/actions/actions.xml:2898: This action is triggered when the notification pops up. Iterated. Shown is ambiguous in notifications parlance. On 2014/05/05 23:27:36, Ilya Sherman wrote: > nit: I'd rephrase this to something more like "A Google Now notification was > shown to the user." https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. There is no Google Now specific message center. There is only one message center in Chrome. It is also known as the Notifications Center, but in code, it's called the Message Center. On 2014/05/05 23:27:36, Ilya Sherman wrote: > Which message center, btw? Is the message center specific to Google Now, or is > it shared with other notifications? If it's shared, is this counting just the > Google Now cards among the notifications? Please clarify this in the summary > text.
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( That sounds fine. Please implement that cap. If you think there's a reasonable chance that 10+ would someday need to expand, feel free to choose a higher cap. If the cap starts getting quite large -- say, past 25 or so -- you'll probably want to start thinking about exponentially sized buckets anyway. (Do you really care to distinguish between 23 and 24 notifications?) On 2014/05/05 23:33:55, robliao wrote: > We're interested in measuring the following specific numbers > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > The 10+ may partially expand if there are a lot of 10+ numbers. > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > On 2014/05/05 22:39:28, robliao wrote: > > > We want to avoid bucketing if at all possible. How should we set up > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > Why do you want to avoid bucketing? What sorts of values are you trying to > > measure? If you're pretty confident that a user would never have more than, > > say, 10 notifications, and you only want the corresponding 10 buckets, that's > > fine; but in that case you should impose a maximum limit and emit to an > overflow > > bucket if that limit is exceeded. > https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/20001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. Thanks. Please update the summary text to clarify this. On 2014/05/05 23:33:55, robliao wrote: > There is no Google Now specific message center. There is only one message center > in Chrome. It is also known as the Notifications Center, but in code, it's > called the Message Center. > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > Which message center, btw? Is the message center specific to Google Now, or > is > > it shared with other notifications? If it's shared, is this counting just the > > Google Now cards among the notifications? Please clarify this in the summary > > text. >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( When crunching stats for review, yes. To the original question, what's the right way to set this up? HISTOGRAM_CUSTOM_COUNTS( "GoogleNow.MessageCenter.Displayed.NotificationsVisible", CountVisibleGoogleNowNotifications(), 0, 10, 12)? It's not clear based off the API what we're getting when we specify the bucket count. Do we get these buckets? 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ On 2014/05/05 23:39:07, Ilya Sherman wrote: > That sounds fine. Please implement that cap. If you think there's a reasonable > chance that 10+ would someday need to expand, feel free to choose a higher cap. > If the cap starts getting quite large -- say, past 25 or so -- you'll probably > want to start thinking about exponentially sized buckets anyway. (Do you really > care to distinguish between 23 and 24 notifications?) > > On 2014/05/05 23:33:55, robliao wrote: > > We're interested in measuring the following specific numbers > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > On 2014/05/05 22:39:28, robliao wrote: > > > > We want to avoid bucketing if at all possible. How should we set up > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > Why do you want to avoid bucketing? What sorts of values are you trying to > > > measure? If you're pretty confident that a user would never have more than, > > > say, 10 notifications, and you only want the corresponding 10 buckets, > that's > > > fine; but in that case you should impose a maximum limit and emit to an > > overflow > > > bucket if that limit is exceeded. > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2014/05/05 23:45:56, robliao wrote: > When crunching stats for review, yes. > > To the original question, what's the right way to set this up? > HISTOGRAM_CUSTOM_COUNTS( > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > CountVisibleGoogleNowNotifications(), > 0, > 10, > 12)? > > It's not clear based off the API what we're getting when we specify the bucket > count. > Do we get these buckets? > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ If you want linear buckets, you might as well keep the sparse histogram (you could also use a linear histogram, if you prefer; there's not much practical difference in this case). Just add code to impose the cap, i.e. to clamp the logged value. > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > That sounds fine. Please implement that cap. If you think there's a > reasonable > > chance that 10+ would someday need to expand, feel free to choose a higher > cap. > > If the cap starts getting quite large -- say, past 25 or so -- you'll probably > > want to start thinking about exponentially sized buckets anyway. (Do you > really > > care to distinguish between 23 and 24 notifications?) > > > > On 2014/05/05 23:33:55, robliao wrote: > > > We're interested in measuring the following specific numbers > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > We want to avoid bucketing if at all possible. How should we set up > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > Why do you want to avoid bucketing? What sorts of values are you trying > to > > > > measure? If you're pretty confident that a user would never have more > than, > > > > say, 10 notifications, and you only want the corresponding 10 buckets, > > that's > > > > fine; but in that case you should impose a maximum limit and emit to an > > > overflow > > > > bucket if that limit is exceeded. > > > > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( We discovered that our sparse histograms on the UMA site displayed as 23-24. Is that [23, 24)? Since we're going sparse, is there harm is not clamping the value? On 2014/05/05 23:53:32, Ilya Sherman wrote: > On 2014/05/05 23:45:56, robliao wrote: > > When crunching stats for review, yes. > > > > To the original question, what's the right way to set this up? > > HISTOGRAM_CUSTOM_COUNTS( > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > CountVisibleGoogleNowNotifications(), > > 0, > > 10, > > 12)? > > > > It's not clear based off the API what we're getting when we specify the bucket > > count. > > Do we get these buckets? > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > If you want linear buckets, you might as well keep the sparse histogram (you > could also use a linear histogram, if you prefer; there's not much practical > difference in this case). Just add code to impose the cap, i.e. to clamp the > logged value. > > > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > > That sounds fine. Please implement that cap. If you think there's a > > reasonable > > > chance that 10+ would someday need to expand, feel free to choose a higher > > cap. > > > If the cap starts getting quite large -- say, past 25 or so -- you'll > probably > > > want to start thinking about exponentially sized buckets anyway. (Do you > > really > > > care to distinguish between 23 and 24 notifications?) > > > > > > On 2014/05/05 23:33:55, robliao wrote: > > > > We're interested in measuring the following specific numbers > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > > We want to avoid bucketing if at all possible. How should we set up > > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > > > Why do you want to avoid bucketing? What sorts of values are you trying > > to > > > > > measure? If you're pretty confident that a user would never have more > > than, > > > > > say, 10 notifications, and you only want the corresponding 10 buckets, > > > that's > > > > > fine; but in that case you should impose a maximum limit and emit to an > > > > overflow > > > > > bucket if that limit is exceeded. > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2014/05/06 00:06:15, robliao wrote: > We discovered that our sparse histograms on the UMA site displayed as 23-24. Is > that [23, 24)? Yep, 'tis. > Since we're going sparse, is there harm is not clamping the value? Yes: The backend does not do well with enormous histograms. If your logging code has a bug and somehow ends up logging well above what you *think* is the maximum, this can slow down the whole pipeline. > On 2014/05/05 23:53:32, Ilya Sherman wrote: > > On 2014/05/05 23:45:56, robliao wrote: > > > When crunching stats for review, yes. > > > > > > To the original question, what's the right way to set this up? > > > HISTOGRAM_CUSTOM_COUNTS( > > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > > CountVisibleGoogleNowNotifications(), > > > 0, > > > 10, > > > 12)? > > > > > > It's not clear based off the API what we're getting when we specify the > bucket > > > count. > > > Do we get these buckets? > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > If you want linear buckets, you might as well keep the sparse histogram (you > > could also use a linear histogram, if you prefer; there's not much practical > > difference in this case). Just add code to impose the cap, i.e. to clamp the > > logged value. > > > > > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > > > That sounds fine. Please implement that cap. If you think there's a > > > reasonable > > > > chance that 10+ would someday need to expand, feel free to choose a higher > > > cap. > > > > If the cap starts getting quite large -- say, past 25 or so -- you'll > > probably > > > > want to start thinking about exponentially sized buckets anyway. (Do you > > > really > > > > care to distinguish between 23 and 24 notifications?) > > > > > > > > On 2014/05/05 23:33:55, robliao wrote: > > > > > We're interested in measuring the following specific numbers > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > > > We want to avoid bucketing if at all possible. How should we set up > > > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > > > > > Why do you want to avoid bucketing? What sorts of values are you > trying > > > to > > > > > > measure? If you're pretty confident that a user would never have more > > > than, > > > > > > say, 10 notifications, and you only want the corresponding 10 buckets, > > > > that's > > > > > > fine; but in that case you should impose a maximum limit and emit to > an > > > > > overflow > > > > > > bucket if that limit is exceeded. > > > > > > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( Okay, so how does this sound then? UMA_HISTOGRAM_SPARSE_SLOWLY( "GoogleNow.MessageCenter.Displayed.NotificationsVisible", min(CountVisibleGoogleNowNotifications(), 10); On 2014/05/06 00:10:52, Ilya Sherman wrote: > On 2014/05/06 00:06:15, robliao wrote: > > We discovered that our sparse histograms on the UMA site displayed as 23-24. > Is > > that [23, 24)? > > Yep, 'tis. > > > Since we're going sparse, is there harm is not clamping the value? > > Yes: The backend does not do well with enormous histograms. If your logging > code has a bug and somehow ends up logging well above what you *think* is the > maximum, this can slow down the whole pipeline. > > > On 2014/05/05 23:53:32, Ilya Sherman wrote: > > > On 2014/05/05 23:45:56, robliao wrote: > > > > When crunching stats for review, yes. > > > > > > > > To the original question, what's the right way to set this up? > > > > HISTOGRAM_CUSTOM_COUNTS( > > > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > > > CountVisibleGoogleNowNotifications(), > > > > 0, > > > > 10, > > > > 12)? > > > > > > > > It's not clear based off the API what we're getting when we specify the > > bucket > > > > count. > > > > Do we get these buckets? > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > If you want linear buckets, you might as well keep the sparse histogram (you > > > could also use a linear histogram, if you prefer; there's not much practical > > > difference in this case). Just add code to impose the cap, i.e. to clamp > the > > > logged value. > > > > > > > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > > > > That sounds fine. Please implement that cap. If you think there's a > > > > reasonable > > > > > chance that 10+ would someday need to expand, feel free to choose a > higher > > > > cap. > > > > > If the cap starts getting quite large -- say, past 25 or so -- you'll > > > probably > > > > > want to start thinking about exponentially sized buckets anyway. (Do > you > > > > really > > > > > care to distinguish between 23 and 24 notifications?) > > > > > > > > > > On 2014/05/05 23:33:55, robliao wrote: > > > > > > We're interested in measuring the following specific numbers > > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > > > > We want to avoid bucketing if at all possible. How should we set > up > > > > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > > > > > > > Why do you want to avoid bucketing? What sorts of values are you > > trying > > > > to > > > > > > > measure? If you're pretty confident that a user would never have > more > > > > than, > > > > > > > say, 10 notifications, and you only want the corresponding 10 > buckets, > > > > > that's > > > > > > > fine; but in that case you should impose a maximum limit and emit to > > an > > > > > > overflow > > > > > > > bucket if that limit is exceeded. > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( On 2014/05/06 00:17:33, robliao wrote: > Okay, so how does this sound then? > UMA_HISTOGRAM_SPARSE_SLOWLY( > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > min(CountVisibleGoogleNowNotifications(), 10); Sounds fine. > On 2014/05/06 00:10:52, Ilya Sherman wrote: > > On 2014/05/06 00:06:15, robliao wrote: > > > We discovered that our sparse histograms on the UMA site displayed as 23-24. > > Is > > > that [23, 24)? > > > > Yep, 'tis. > > > > > Since we're going sparse, is there harm is not clamping the value? > > > > Yes: The backend does not do well with enormous histograms. If your logging > > code has a bug and somehow ends up logging well above what you *think* is the > > maximum, this can slow down the whole pipeline. > > > > > On 2014/05/05 23:53:32, Ilya Sherman wrote: > > > > On 2014/05/05 23:45:56, robliao wrote: > > > > > When crunching stats for review, yes. > > > > > > > > > > To the original question, what's the right way to set this up? > > > > > HISTOGRAM_CUSTOM_COUNTS( > > > > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > > > > CountVisibleGoogleNowNotifications(), > > > > > 0, > > > > > 10, > > > > > 12)? > > > > > > > > > > It's not clear based off the API what we're getting when we specify the > > > bucket > > > > > count. > > > > > Do we get these buckets? > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > If you want linear buckets, you might as well keep the sparse histogram > (you > > > > could also use a linear histogram, if you prefer; there's not much > practical > > > > difference in this case). Just add code to impose the cap, i.e. to clamp > > the > > > > logged value. > > > > > > > > > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > > > > > That sounds fine. Please implement that cap. If you think there's a > > > > > reasonable > > > > > > chance that 10+ would someday need to expand, feel free to choose a > > higher > > > > > cap. > > > > > > If the cap starts getting quite large -- say, past 25 or so -- you'll > > > > probably > > > > > > want to start thinking about exponentially sized buckets anyway. (Do > > you > > > > > really > > > > > > care to distinguish between 23 and 24 notifications?) > > > > > > > > > > > > On 2014/05/05 23:33:55, robliao wrote: > > > > > > > We're interested in measuring the following specific numbers > > > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > > > > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > > > > > We want to avoid bucketing if at all possible. How should we set > > up > > > > > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > > > > > > > > > Why do you want to avoid bucketing? What sorts of values are you > > > trying > > > > > to > > > > > > > > measure? If you're pretty confident that a user would never have > > more > > > > > than, > > > > > > > > say, 10 notifications, and you only want the corresponding 10 > > buckets, > > > > > > that's > > > > > > > > fine; but in that case you should impose a maximum limit and emit > to > > > an > > > > > > > overflow > > > > > > > > bucket if that limit is exceeded. > > > > > > > > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/1/chrome/browser/notifications... chrome/browser/notifications/google_now_notification_stats_collector.cc:42: UMA_HISTOGRAM_SPARSE_SLOWLY( It's a done deal. On 2014/05/06 00:18:15, Ilya Sherman wrote: > On 2014/05/06 00:17:33, robliao wrote: > > Okay, so how does this sound then? > > UMA_HISTOGRAM_SPARSE_SLOWLY( > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > min(CountVisibleGoogleNowNotifications(), 10); > > Sounds fine. > > > On 2014/05/06 00:10:52, Ilya Sherman wrote: > > > On 2014/05/06 00:06:15, robliao wrote: > > > > We discovered that our sparse histograms on the UMA site displayed as > 23-24. > > > Is > > > > that [23, 24)? > > > > > > Yep, 'tis. > > > > > > > Since we're going sparse, is there harm is not clamping the value? > > > > > > Yes: The backend does not do well with enormous histograms. If your logging > > > code has a bug and somehow ends up logging well above what you *think* is > the > > > maximum, this can slow down the whole pipeline. > > > > > > > On 2014/05/05 23:53:32, Ilya Sherman wrote: > > > > > On 2014/05/05 23:45:56, robliao wrote: > > > > > > When crunching stats for review, yes. > > > > > > > > > > > > To the original question, what's the right way to set this up? > > > > > > HISTOGRAM_CUSTOM_COUNTS( > > > > > > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > > > > > CountVisibleGoogleNowNotifications(), > > > > > > 0, > > > > > > 10, > > > > > > 12)? > > > > > > > > > > > > It's not clear based off the API what we're getting when we specify > the > > > > bucket > > > > > > count. > > > > > > Do we get these buckets? > > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > > > If you want linear buckets, you might as well keep the sparse histogram > > (you > > > > > could also use a linear histogram, if you prefer; there's not much > > practical > > > > > difference in this case). Just add code to impose the cap, i.e. to > clamp > > > the > > > > > logged value. > > > > > > > > > > > On 2014/05/05 23:39:07, Ilya Sherman wrote: > > > > > > > That sounds fine. Please implement that cap. If you think there's > a > > > > > > reasonable > > > > > > > chance that 10+ would someday need to expand, feel free to choose a > > > higher > > > > > > cap. > > > > > > > If the cap starts getting quite large -- say, past 25 or so -- > you'll > > > > > probably > > > > > > > want to start thinking about exponentially sized buckets anyway. > (Do > > > you > > > > > > really > > > > > > > care to distinguish between 23 and 24 notifications?) > > > > > > > > > > > > > > On 2014/05/05 23:33:55, robliao wrote: > > > > > > > > We're interested in measuring the following specific numbers > > > > > > > > 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10+ > > > > > > > > > > > > > > > > The 10+ may partially expand if there are a lot of 10+ numbers. > > > > > > > > > > > > > > > > On 2014/05/05 23:27:36, Ilya Sherman wrote: > > > > > > > > > On 2014/05/05 22:39:28, robliao wrote: > > > > > > > > > > We want to avoid bucketing if at all possible. How should we > set > > > up > > > > > > > > > > HISTOGRAM_CUSTOM_COUNTS to do this? > > > > > > > > > > > > > > > > > > Why do you want to avoid bucketing? What sorts of values are > you > > > > trying > > > > > > to > > > > > > > > > measure? If you're pretty confident that a user would never > have > > > more > > > > > > than, > > > > > > > > > say, 10 notifications, and you only want the corresponding 10 > > > buckets, > > > > > > > that's > > > > > > > > > fine; but in that case you should impose a maximum limit and > emit > > to > > > > an > > > > > > > > overflow > > > > > > > > > bucket if that limit is exceeded. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/80001/chrome/browser/notificat... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/80001/chrome/browser/notificat... chrome/browser/notifications/google_now_notification_stats_collector.cc:18: const int kNotificationsMaxCount = 10; As I mentioned before, if you're worried that 10 might be too low, feel free to bump this to something like 25. Otherwise, if you decide to resize the histogram later, you'll need to rename it as well -- histograms with data in overflow buckets aren't designed to be resizable. https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. Please update this text, as I requested in previous comments.
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. The correct terminology is already being used. We can't add much more without making this unnecessarily verbose. On 2014/05/06 00:33:39, Ilya Sherman wrote: > Please update this text, as I requested in previous comments.
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. Might I suggest: """The number of Google Now cards visible when the message center (a.k.a. the notification center) is shown.""" On 2014/05/06 00:39:40, robliao wrote: > The correct terminology is already being used. We can't add much more without > making this unnecessarily verbose. > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > Please update this text, as I requested in previous comments.
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. It's the difference between this... https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter And this... https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter The search results of NotificationCenter is very misleading, so that's why I don't really want to go there. On 2014/05/06 00:42:41, Ilya Sherman wrote: > Might I suggest: > > """The number of Google Now cards visible when the message center (a.k.a. the > notification center) is shown.""" > > On 2014/05/06 00:39:40, robliao wrote: > > The correct terminology is already being used. We can't add much more without > > making this unnecessarily verbose. > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > Please update this text, as I requested in previous comments.
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. On 2014/05/06 00:51:41, robliao wrote: > It's the difference between this... > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > And this... > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > The search results of NotificationCenter is very misleading, so that's why I > don't really want to go there. Code search is only one way that people might try to understand the named term. More likely than code search is just general awareness of Chrome's features. Anyone who's writing a codesearch query would probably just search for the histogram name, "GoogleNow.MessageCenter.Displayed.NotificationsVisible", and therefore the confusion that you mentioned doesn't seem very relevant. OTOH, somebody who's viewing the description on the dashboard would likely appreciate the connection to a Chrome feature that they've heard of, as opposed to having to guess. How do I know? Because I had to guess what "the message center" was, and I'm familiar with the notifications feature that Chrome has. Here's another way to think of it: Histogram descriptions should be understandable to somebody who's familiar with the code. Including both terms makes it more likely that such a person would understand what you're referring to. > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > Might I suggest: > > > > """The number of Google Now cards visible when the message center (a.k.a. the > > notification center) is shown.""" > > > > On 2014/05/06 00:39:40, robliao wrote: > > > The correct terminology is already being used. We can't add much more > without > > > making this unnecessarily verbose. > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > Please update this text, as I requested in previous comments. >
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. I think we'll simply have to agree to disagree here. I've changed the term. The search results above are simply an illustration of what terminology that's already used and is not the only justification for using the message center as the term here. When constructing the summary, we should be using precise unambiguous terms that describe the components we're working with. In the Chrome codebase, there is no component named the Notification Center. It's called the Message Center. I will agree this is an unfortunate diversion of the names, but that's what we have here. As a result, everything in the codebase resolves around this terminology. The Message Center has Message Center tests, a message center tray bridge, a message center bubble, a message center button bar, and so forth. Since this histogram name is used in the message center, it is appropriate that the terminology remain consistent until the message center is renamed to the notifications center. The term 'notifications center' introduces inconsistency into this naming. As such, the histogram name and summary now use two different names for the same thing. On 2014/05/06 01:02:32, Ilya Sherman wrote: > On 2014/05/06 00:51:41, robliao wrote: > > It's the difference between this... > > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > > > And this... > > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > > > The search results of NotificationCenter is very misleading, so that's why I > > don't really want to go there. > > Code search is only one way that people might try to understand the named term. > More likely than code search is just general awareness of Chrome's features. > Anyone who's writing a codesearch query would probably just search for the > histogram name, "GoogleNow.MessageCenter.Displayed.NotificationsVisible", and > therefore the confusion that you mentioned doesn't seem very relevant. OTOH, > somebody who's viewing the description on the dashboard would likely appreciate > the connection to a Chrome feature that they've heard of, as opposed to having > to guess. How do I know? Because I had to guess what "the message center" was, > and I'm familiar with the notifications feature that Chrome has. > > Here's another way to think of it: Histogram descriptions should be > understandable to somebody who's familiar with the code. Including both terms > makes it more likely that such a person would understand what you're referring > to. > > > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > > Might I suggest: > > > > > > """The number of Google Now cards visible when the message center (a.k.a. > the > > > notification center) is shown.""" > > > > > > On 2014/05/06 00:39:40, robliao wrote: > > > > The correct terminology is already being used. We can't add much more > > without > > > > making this unnecessarily verbose. > > > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > > Please update this text, as I requested in previous comments. > > >
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. I'm not saying that you shouldn't use "message center" in the summary. Rather, I'm encouraging you to use both terms. You'll note that in my suggested text, I kept "message center" as the primary term, to match your preferences, and simply included "a.k.a. notification center" as a hint for people unfamiliar with the term "message center". On 2014/05/06 01:16:24, robliao wrote: > I think we'll simply have to agree to disagree here. I've changed the term. > > The search results above are simply an illustration of what terminology that's > already used and is not the only justification for using the message center as > the term here. > > When constructing the summary, we should be using precise unambiguous terms that > describe the components we're working with. In the Chrome codebase, there is no > component named the Notification Center. It's called the Message Center. > > I will agree this is an unfortunate diversion of the names, but that's what we > have here. As a result, everything in the codebase resolves around this > terminology. The Message Center has Message Center tests, a message center tray > bridge, a message center bubble, a message center button bar, and so forth. > > Since this histogram name is used in the message center, it is appropriate that > the terminology remain consistent until the message center is renamed to the > notifications center. > > The term 'notifications center' introduces inconsistency into this naming. As > such, the histogram name and summary now use two different names for the same > thing. > > On 2014/05/06 01:02:32, Ilya Sherman wrote: > > On 2014/05/06 00:51:41, robliao wrote: > > > It's the difference between this... > > > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > > > > > And this... > > > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > > > > > The search results of NotificationCenter is very misleading, so that's why I > > > don't really want to go there. > > > > Code search is only one way that people might try to understand the named > term. > > More likely than code search is just general awareness of Chrome's features. > > Anyone who's writing a codesearch query would probably just search for the > > histogram name, "GoogleNow.MessageCenter.Displayed.NotificationsVisible", and > > therefore the confusion that you mentioned doesn't seem very relevant. OTOH, > > somebody who's viewing the description on the dashboard would likely > appreciate > > the connection to a Chrome feature that they've heard of, as opposed to having > > to guess. How do I know? Because I had to guess what "the message center" > was, > > and I'm familiar with the notifications feature that Chrome has. > > > > Here's another way to think of it: Histogram descriptions should be > > understandable to somebody who's familiar with the code. Including both terms > > makes it more likely that such a person would understand what you're referring > > to. > > > > > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > > > Might I suggest: > > > > > > > > """The number of Google Now cards visible when the message center (a.k.a. > > the > > > > notification center) is shown.""" > > > > > > > > On 2014/05/06 00:39:40, robliao wrote: > > > > > The correct terminology is already being used. We can't add much more > > > without > > > > > making this unnecessarily verbose. > > > > > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > > > Please update this text, as I requested in previous comments. > > > > > >
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. If someone is unfamiliar with both the message center and the notification center, what do we do then? At some point, someone is going to come across a summary of an unfamiliar feature. Code search is there like a dictionary to help you out. On 2014/05/06 01:24:24, Ilya Sherman wrote: > I'm not saying that you shouldn't use "message center" in the summary. Rather, > I'm encouraging you to use both terms. You'll note that in my suggested text, I > kept "message center" as the primary term, to match your preferences, and simply > included "a.k.a. notification center" as a hint for people unfamiliar with the > term "message center". > > On 2014/05/06 01:16:24, robliao wrote: > > I think we'll simply have to agree to disagree here. I've changed the term. > > > > The search results above are simply an illustration of what terminology that's > > already used and is not the only justification for using the message center as > > the term here. > > > > When constructing the summary, we should be using precise unambiguous terms > that > > describe the components we're working with. In the Chrome codebase, there is > no > > component named the Notification Center. It's called the Message Center. > > > > I will agree this is an unfortunate diversion of the names, but that's what we > > have here. As a result, everything in the codebase resolves around this > > terminology. The Message Center has Message Center tests, a message center > tray > > bridge, a message center bubble, a message center button bar, and so forth. > > > > Since this histogram name is used in the message center, it is appropriate > that > > the terminology remain consistent until the message center is renamed to the > > notifications center. > > > > The term 'notifications center' introduces inconsistency into this naming. As > > such, the histogram name and summary now use two different names for the same > > thing. > > > > On 2014/05/06 01:02:32, Ilya Sherman wrote: > > > On 2014/05/06 00:51:41, robliao wrote: > > > > It's the difference between this... > > > > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > > > > > > > And this... > > > > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > > > > > > > The search results of NotificationCenter is very misleading, so that's why > I > > > > don't really want to go there. > > > > > > Code search is only one way that people might try to understand the named > > term. > > > More likely than code search is just general awareness of Chrome's features. > > > > Anyone who's writing a codesearch query would probably just search for the > > > histogram name, "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > and > > > therefore the confusion that you mentioned doesn't seem very relevant. > OTOH, > > > somebody who's viewing the description on the dashboard would likely > > appreciate > > > the connection to a Chrome feature that they've heard of, as opposed to > having > > > to guess. How do I know? Because I had to guess what "the message center" > > was, > > > and I'm familiar with the notifications feature that Chrome has. > > > > > > Here's another way to think of it: Histogram descriptions should be > > > understandable to somebody who's familiar with the code. Including both > terms > > > makes it more likely that such a person would understand what you're > referring > > > to. > > > > > > > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > > > > Might I suggest: > > > > > > > > > > """The number of Google Now cards visible when the message center > (a.k.a. > > > the > > > > > notification center) is shown.""" > > > > > > > > > > On 2014/05/06 00:39:40, robliao wrote: > > > > > > The correct terminology is already being used. We can't add much more > > > > without > > > > > > making this unnecessarily verbose. > > > > > > > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > > > > Please update this text, as I requested in previous comments. > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. On 2014/05/06 01:29:19, robliao wrote: > If someone is unfamiliar with both the message center and the notification > center, what do we do then? > > At some point, someone is going to come across a summary of an unfamiliar > feature. Code search is there like a dictionary to help you out. Again, if somebody is going to search via Code search, their best bet is to search for the histogram name, which will return exactly one hit (I'd hope). Do you have any other objection to including both terms? > On 2014/05/06 01:24:24, Ilya Sherman wrote: > > I'm not saying that you shouldn't use "message center" in the summary. > Rather, > > I'm encouraging you to use both terms. You'll note that in my suggested text, > I > > kept "message center" as the primary term, to match your preferences, and > simply > > included "a.k.a. notification center" as a hint for people unfamiliar with the > > term "message center". > > > > On 2014/05/06 01:16:24, robliao wrote: > > > I think we'll simply have to agree to disagree here. I've changed the term. > > > > > > The search results above are simply an illustration of what terminology > that's > > > already used and is not the only justification for using the message center > as > > > the term here. > > > > > > When constructing the summary, we should be using precise unambiguous terms > > that > > > describe the components we're working with. In the Chrome codebase, there is > > no > > > component named the Notification Center. It's called the Message Center. > > > > > > I will agree this is an unfortunate diversion of the names, but that's what > we > > > have here. As a result, everything in the codebase resolves around this > > > terminology. The Message Center has Message Center tests, a message center > > tray > > > bridge, a message center bubble, a message center button bar, and so forth. > > > > > > Since this histogram name is used in the message center, it is appropriate > > that > > > the terminology remain consistent until the message center is renamed to the > > > notifications center. > > > > > > The term 'notifications center' introduces inconsistency into this naming. > As > > > such, the histogram name and summary now use two different names for the > same > > > thing. > > > > > > On 2014/05/06 01:02:32, Ilya Sherman wrote: > > > > On 2014/05/06 00:51:41, robliao wrote: > > > > > It's the difference between this... > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > > > > > > > > > And this... > > > > > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > > > > > > > > > The search results of NotificationCenter is very misleading, so that's > why > > I > > > > > don't really want to go there. > > > > > > > > Code search is only one way that people might try to understand the named > > > term. > > > > More likely than code search is just general awareness of Chrome's > features. > > > > > > Anyone who's writing a codesearch query would probably just search for the > > > > histogram name, "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > and > > > > therefore the confusion that you mentioned doesn't seem very relevant. > > OTOH, > > > > somebody who's viewing the description on the dashboard would likely > > > appreciate > > > > the connection to a Chrome feature that they've heard of, as opposed to > > having > > > > to guess. How do I know? Because I had to guess what "the message > center" > > > was, > > > > and I'm familiar with the notifications feature that Chrome has. > > > > > > > > Here's another way to think of it: Histogram descriptions should be > > > > understandable to somebody who's familiar with the code. Including both > > terms > > > > makes it more likely that such a person would understand what you're > > referring > > > > to. > > > > > > > > > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > > > > > Might I suggest: > > > > > > > > > > > > """The number of Google Now cards visible when the message center > > (a.k.a. > > > > the > > > > > > notification center) is shown.""" > > > > > > > > > > > > On 2014/05/06 00:39:40, robliao wrote: > > > > > > > The correct terminology is already being used. We can't add much > more > > > > > without > > > > > > > making this unnecessarily verbose. > > > > > > > > > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > > > > > Please update this text, as I requested in previous comments. > > > > > > > > > > > > > > >
https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/264123002/diff/80001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:7336: + Count of the number of cards visible when the message center is shown. I still think this is the wrong thing to do, but have updated the text for the sake of the review. On 2014/05/06 04:15:36, Ilya Sherman wrote: > On 2014/05/06 01:29:19, robliao wrote: > > If someone is unfamiliar with both the message center and the notification > > center, what do we do then? > > > > At some point, someone is going to come across a summary of an unfamiliar > > feature. Code search is there like a dictionary to help you out. > > Again, if somebody is going to search via Code search, their best bet is to > search for the histogram name, which will return exactly one hit (I'd hope). Do > you have any other objection to including both terms? > > > On 2014/05/06 01:24:24, Ilya Sherman wrote: > > > I'm not saying that you shouldn't use "message center" in the summary. > > Rather, > > > I'm encouraging you to use both terms. You'll note that in my suggested > text, > > I > > > kept "message center" as the primary term, to match your preferences, and > > simply > > > included "a.k.a. notification center" as a hint for people unfamiliar with > the > > > term "message center". > > > > > > On 2014/05/06 01:16:24, robliao wrote: > > > > I think we'll simply have to agree to disagree here. I've changed the > term. > > > > > > > > The search results above are simply an illustration of what terminology > > that's > > > > already used and is not the only justification for using the message > center > > as > > > > the term here. > > > > > > > > When constructing the summary, we should be using precise unambiguous > terms > > > that > > > > describe the components we're working with. In the Chrome codebase, there > is > > > no > > > > component named the Notification Center. It's called the Message Center. > > > > > > > > I will agree this is an unfortunate diversion of the names, but that's > what > > we > > > > have here. As a result, everything in the codebase resolves around this > > > > terminology. The Message Center has Message Center tests, a message center > > > tray > > > > bridge, a message center bubble, a message center button bar, and so > forth. > > > > > > > > Since this histogram name is used in the message center, it is appropriate > > > that > > > > the terminology remain consistent until the message center is renamed to > the > > > > notifications center. > > > > > > > > The term 'notifications center' introduces inconsistency into this naming. > > As > > > > such, the histogram name and summary now use two different names for the > > same > > > > thing. > > > > > > > > On 2014/05/06 01:02:32, Ilya Sherman wrote: > > > > > On 2014/05/06 00:51:41, robliao wrote: > > > > > > It's the difference between this... > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=NotificationCenter > > > > > > > > > > > > And this... > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=MessageCenter > > > > > > > > > > > > The search results of NotificationCenter is very misleading, so that's > > why > > > I > > > > > > don't really want to go there. > > > > > > > > > > Code search is only one way that people might try to understand the > named > > > > term. > > > > > More likely than code search is just general awareness of Chrome's > > features. > > > > > > > > Anyone who's writing a codesearch query would probably just search for > the > > > > > histogram name, > "GoogleNow.MessageCenter.Displayed.NotificationsVisible", > > > and > > > > > therefore the confusion that you mentioned doesn't seem very relevant. > > > OTOH, > > > > > somebody who's viewing the description on the dashboard would likely > > > > appreciate > > > > > the connection to a Chrome feature that they've heard of, as opposed to > > > having > > > > > to guess. How do I know? Because I had to guess what "the message > > center" > > > > was, > > > > > and I'm familiar with the notifications feature that Chrome has. > > > > > > > > > > Here's another way to think of it: Histogram descriptions should be > > > > > understandable to somebody who's familiar with the code. Including both > > > terms > > > > > makes it more likely that such a person would understand what you're > > > referring > > > > > to. > > > > > > > > > > > On 2014/05/06 00:42:41, Ilya Sherman wrote: > > > > > > > Might I suggest: > > > > > > > > > > > > > > """The number of Google Now cards visible when the message center > > > (a.k.a. > > > > > the > > > > > > > notification center) is shown.""" > > > > > > > > > > > > > > On 2014/05/06 00:39:40, robliao wrote: > > > > > > > > The correct terminology is already being used. We can't add much > > more > > > > > > without > > > > > > > > making this unnecessarily verbose. > > > > > > > > > > > > > > > > On 2014/05/06 00:33:39, Ilya Sherman wrote: > > > > > > > > > Please update this text, as I requested in previous comments. > > > > > > > > > > > > > > > > > > > > >
I think that a flag in OnNotificationDisplayed is more appropreate instead of a new method OnNotificationPoppedUp, since there may be other consumers of OnNotificationDisplayed that depend on seeing that whenever it's rendered. Additionally, I think it's best to integrate with the current MessageCenterStatsCollector than to add Yet Another MC Observer class. Did you have any motivation to make it a completely separate observer, or was that just the easy path?
On 2014/05/06 18:21:47, dewittj wrote: > I think that a flag in OnNotificationDisplayed is more appropreate instead of a > new method OnNotificationPoppedUp, since there may be other consumers of > OnNotificationDisplayed that depend on seeing that whenever it's rendered. > > Additionally, I think it's best to integrate with the current > MessageCenterStatsCollector than to add Yet Another MC Observer class. Did you > have any motivation to make it a completely separate observer, or was that just > the easy path? Adding a new method on MessageCenterObserver was cleaner and also avoided having to change all the implementations and callers. As for Yet Another MC Observer class, this definitely wasn't the easier route, and was done to keep a simple separation between the cares of Google Now and the Message Center. Should we want to stop recording stats for Google Now, this becomes trivially easy to do.
On 2014/05/06 18:21:47, dewittj wrote: > I think that a flag in OnNotificationDisplayed is more appropreate instead of a > new method OnNotificationPoppedUp, since there may be other consumers of > OnNotificationDisplayed that depend on seeing that whenever it's rendered. > > Additionally, I think it's best to integrate with the current > MessageCenterStatsCollector than to add Yet Another MC Observer class. Did you > have any motivation to make it a completely separate observer, or was that just > the easy path? Adding a new method on MessageCenterObserver was cleaner and also avoided having to change all the implementations and callers. As for Yet Another MC Observer class, this definitely wasn't the easier route, and was done to keep a simple separation between the cares of Google Now and the Message Center. Should we want to stop recording stats for Google Now, this becomes trivially easy to do.
unfortunately you still need to change all the implementors, since they rely on OnNotificationDisplayed being called for both popups and message center.
LGTM
On 2014/05/06 18:37:34, dewittj wrote: > unfortunately you still need to change all the implementors, since they rely on > OnNotificationDisplayed being called for both popups and message center. I think I hit all the main ones (specifically MessageCenterImpl, which simply forwards this on to DisplayedNotification and hits OnNotificationDisplayed). The signatures don't change, which avoids changing everyone.
On 2014/05/06 18:43:34, robliao wrote: > On 2014/05/06 18:37:34, dewittj wrote: > > unfortunately you still need to change all the implementors, since they rely > on > > OnNotificationDisplayed being called for both popups and message center. > > I think I hit all the main ones (specifically MessageCenterImpl, which simply > forwards this on to DisplayedNotification and hits OnNotificationDisplayed). > The signatures don't change, which avoids changing everyone. Looks like there are 3 implementations: https://code.google.com/p/chromium/codesearch#search/&q=::OnNotificationDispl... Did you check that network_portal_detector_impl_browsertest.cc still passes? And need to update ui/message_center/message_center_tray.cc so that onmessagecentertraychanged is passed to chromeos to update the tray icon.
On 2014/05/06 18:50:30, dewittj wrote: > On 2014/05/06 18:43:34, robliao wrote: > > On 2014/05/06 18:37:34, dewittj wrote: > > > unfortunately you still need to change all the implementors, since they rely > > on > > > OnNotificationDisplayed being called for both popups and message center. > > > > I think I hit all the main ones (specifically MessageCenterImpl, which simply > > forwards this on to DisplayedNotification and hits OnNotificationDisplayed). > > The signatures don't change, which avoids changing everyone. > > Looks like there are 3 implementations: > https://code.google.com/p/chromium/codesearch#search/&q=::OnNotificationDispl... > > Did you check that network_portal_detector_impl_browsertest.cc still passes? > And need to update ui/message_center/message_center_tray.cc so that > onmessagecentertraychanged is passed to chromeos to update the tray icon. network_portal_detector_impl_browsertest.cc still passes. Not sure why it shouldn't pass. Everyone who was getting OnNotificationDisplayed messages earlier will still get them under this. PoppedUpNotification calls the requisite NotificationDisplayed codepaths before firing its own event. [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from NetworkPortalDetectorImplBrowserTest, where TypeParam = [ RUN ] NetworkPortalDetectorImplBrowserTest.PRE_InSessionDetection [ OK ] NetworkPortalDetectorImplBrowserTest.PRE_InSessionDetection (11637 ms) [----------] 1 test from NetworkPortalDetectorImplBrowserTest (11637 ms total) [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from NetworkPortalDetectorImplBrowserTest, where TypeParam = [ RUN ] NetworkPortalDetectorImplBrowserTest.InSessionDetection [ OK ] NetworkPortalDetectorImplBrowserTest.InSessionDetection (3256 ms) [----------] 1 test from NetworkPortalDetectorImplBrowserTest (3256 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (3257 ms total) [ PASSED ] 1 test. [2/2] NetworkPortalDetectorImplBrowserTest.InSessionDetection (4716 ms) SUCCESS: all tests passed.
Sorry, I failed to notice the DisplayedNotification call in PoppedUpNotification. I think it's strange to have that there, and it means there's no good way to tell if a displayednotification call is or isn't the result of the popup. Could you make it a flag instead? You can tbr the non-me reviewers for that type of simple signature change.
On 2014/05/07 15:48:12, dewittj wrote: > Sorry, I failed to notice the DisplayedNotification call in > PoppedUpNotification. I think it's strange to have that there, and it means > there's no good way to tell if a displayednotification call is or isn't the > result of the popup. Could you make it a flag instead? You can tbr the non-me > reviewers for that type of simple signature change. > it means there's no good way to tell if a displayednotification call is or isn't the > result of the popup. This is the status quo today and why I introduced OnPoppedUpNotification. If clients don't care about popped up notifications (which is pretty much all of them except for the Google Now stats collector), they simply don't override the OnPoppedUpNotification method. If we use the flag, now everyone needs to reassess if they care about this state. On the first pass as you propose, it will result in a lot of ignored arguments because all the components do not distinguish between a popped up notification and a displayed notification. The change as proposed here avoids the issue of ignored arguments and maintains the current calling contracts.
> If we use the flag, now everyone needs to reassess if they care about this > state. On the first pass as you propose, it will result in a lot of ignored > arguments because all the components do not distinguish between a popped up > notification and a displayed notification. In either case this reassessment still needs to happen - one way silently adds a new method to the observer interface and one way adds an argument to an existing method, not so silently due to signature change. Which is more likely to cause the reassessment? Unfortunately the proposed solution only supports the particular case that Now wants: it cares about popups and not about MC. What about consumers that care when it's shown in the MC but not as popups? (Synced Notifications cares about this for example, and I'd like to deliver that information soon) > The change as proposed here avoids the issue of ignored arguments and maintains > the current calling contracts. And instead makes a MessageCenterObserver API with redundant and partially overlapping methods. Since other observers would like to know whether a displayed notification was popped up or not, and making it fully differentiable in MCO is important to knowing for sure.
On 2014/05/07 16:20:54, dewittj wrote: > > If we use the flag, now everyone needs to reassess if they care about this > > state. On the first pass as you propose, it will result in a lot of ignored > > arguments because all the components do not distinguish between a popped up > > notification and a displayed notification. > > In either case this reassessment still needs to happen - one way silently adds a > new method to the observer interface and one way adds an argument to an existing > method, not so silently due to signature change. Which is more likely to cause > the reassessment? > > Unfortunately the proposed solution only supports the particular case that Now > wants: it cares about popups and not about MC. What about consumers that care > when it's shown in the MC but not as popups? (Synced Notifications cares about > this for example, and I'd like to deliver that information soon) > > > The change as proposed here avoids the issue of ignored arguments and > maintains > > the current calling contracts. > > And instead makes a MessageCenterObserver API with redundant and partially > overlapping methods. Since other observers would like to know whether a > displayed notification was popped up or not, and making it fully differentiable > in MCO is important to knowing for sure. It still remains that this proposed parameter will be ignored by most clients. I can make the change, but it will be very disruptive.
Refactored. TBR=xiyuan FYI.
thanks! lgtm + a few comments https://codereview.chromium.org/264123002/diff/160001/chrome/browser/notifica... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/160001/chrome/browser/notifica... chrome/browser/notifications/google_now_notification_stats_collector.cc:58: (notification->notifier_id().id == kChromeNowExtensionID); To be sure you may want to check that type = APPLICATION when comparing the notifier id. https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... File ui/message_center/message_center_types.h (right): https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... ui/message_center/message_center_types.h:22: MESSAGE_CENTER = 0, In Chrome the style is to name the constants starting with the enum name. So Probably renaming enum to: enum DisplayType { DISPLAY_TYPE_CENTER = 0, DISPLAY_TYPE_POPUP }; would fit better.
https://codereview.chromium.org/264123002/diff/160001/chrome/browser/notifica... File chrome/browser/notifications/google_now_notification_stats_collector.cc (right): https://codereview.chromium.org/264123002/diff/160001/chrome/browser/notifica... chrome/browser/notifications/google_now_notification_stats_collector.cc:58: (notification->notifier_id().id == kChromeNowExtensionID); On 2014/05/07 18:56:58, dewittj wrote: > To be sure you may want to check that type = APPLICATION when comparing the > notifier id. Done. https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... File ui/message_center/message_center_types.h (right): https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... ui/message_center/message_center_types.h:22: MESSAGE_CENTER = 0, I'm partial to keeping the source portion since we're talking about source of the notification. On 2014/05/07 18:56:58, dewittj wrote: > In Chrome the style is to name the constants starting with the enum name. So > Probably renaming enum to: > > enum DisplayType { > DISPLAY_TYPE_CENTER = 0, > DISPLAY_TYPE_POPUP > }; > > would fit better.
https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... File ui/message_center/message_center_types.h (right): https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... ui/message_center/message_center_types.h:22: MESSAGE_CENTER = 0, fine with me, just please adhere to the prefix style.
https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... File ui/message_center/message_center_types.h (right): https://codereview.chromium.org/264123002/diff/160001/ui/message_center/messa... ui/message_center/message_center_types.h:22: MESSAGE_CENTER = 0, Already done. On 2014/05/07 19:59:15, dewittj wrote: > fine with me, just please adhere to the prefix style.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/264123002/200001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/264123002/200001
Message was sent while issue was closed.
Change committed as 269035 |
