Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2626643002:
Stop showing notifications if they're ignored (Closed)
DescriptionStop showing notifications if they’re ignored
Record metrics for interactions with notifications. Stop exposing
openUrl() to native code, but keep it around; we use our own intent for
notification taps to track metrics now.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
Review-Url: https://codereview.chromium.org/2626643002
Cr-Commit-Position: refs/heads/master@{#443550}
Committed: https://chromium.googlesource.com/chromium/src/+/3cf8939974888a7d5425c244f8962822a642d346
Patch Set 1 #Patch Set 2 : Merge actions histogram together #Patch Set 3 : Properly hook up to Android #Patch Set 4 : Spellcheck #Patch Set 5 : rebase #
Total comments: 17
Patch Set 6 : Formatting, explicit #
Total comments: 10
Patch Set 7 : Address comments #
Total comments: 1
Patch Set 8 : Cache metrics when native library not loaded #Patch Set 9 : String.equals() #Dependent Patchsets: Messages
Total messages: 42 (27 generated)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Stop showing notifications if they're ignored
Record metrics for interactions with notifications.
BUG=675561
==========
to
==========
Stop showing notifications if they’re ignored
Record metrics for interactions with notifications.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
==========
Description was changed from
==========
Stop showing notifications if they’re ignored
Record metrics for interactions with notifications.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
==========
to
==========
Stop showing notifications if they’re ignored
Record metrics for interactions with notifications. Stop exposing
openUrl() to native code, but keep it around; we use our own intent for
notification taps to track metrics now.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
==========
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sfiera@chromium.org changed reviewers: + asvitkine@chromium.org, bauerb@chromium.org
bauerb: most of it asvitkine: histograms changes Does the setup of Impressions and Actions make sense to you? Thanks!
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); Do you actually know that the native library has been loaded at this point? What if Chrome had been killed in the mean time? https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:82: auto profile = ProfileManager::GetLastUsedProfile()->GetOriginalProfile(); Can you use the actual type here? Profile* isn't that much longer. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:83: auto prefs = profile->GetPrefs(); And I would say for PrefService* too. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_features.cc (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_features.cc:12: extern const char kContentSuggestionsNotificationsIgnoredLimitParam[] = Do you need to repeat the extern if it's already on the declaration? https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_metrics.h:15: }; Empty line afterwards please. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_prefs.h (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_prefs.h:8: extern const char kContentSuggestionsConsecutiveIgnoredPrefName[]; Prefs in chrome/ go in chrome/common/pref_names.{h,cc} (this also goes for kNotificationIDWithinCategory, which I should have caught previously; sorry).
https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); On 2017/01/11 15:21:49, Bernhard Bauer wrote: > Do you actually know that the native library has been loaded at this point? What > if Chrome had been killed in the mean time? Ah, no, I don't (checked; it may not be). I really don't know about this sort of thing. How do I start it up? https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:82: auto profile = ProfileManager::GetLastUsedProfile()->GetOriginalProfile(); On 2017/01/11 15:21:49, Bernhard Bauer wrote: > Can you use the actual type here? Profile* isn't that much longer. Done. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/android/... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:83: auto prefs = profile->GetPrefs(); On 2017/01/11 15:21:49, Bernhard Bauer wrote: > And I would say for PrefService* too. Done. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_features.cc (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_features.cc:12: extern const char kContentSuggestionsNotificationsIgnoredLimitParam[] = On 2017/01/11 15:21:49, Bernhard Bauer wrote: > Do you need to repeat the extern if it's already on the declaration? No, removed. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_metrics.h:15: }; On 2017/01/11 15:21:49, Bernhard Bauer wrote: > Empty line afterwards please. Done. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_prefs.h (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_prefs.h:8: extern const char kContentSuggestionsConsecutiveIgnoredPrefName[]; On 2017/01/11 15:21:49, Bernhard Bauer wrote: > Prefs in chrome/ go in chrome/common/pref_names.{h,cc} (this also goes for > kNotificationIDWithinCategory, which I should have caught previously; sorry). Should the registration code also move somewhere else? kNotificationIDWithinCategory is going to go away so that we can track multiple notifications. Should it still move, if we're just going to get rid of it?
https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); On 2017/01/11 15:41:51, sfiera wrote: > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > Do you actually know that the native library has been loaded at this point? > What > > if Chrome had been killed in the mean time? > > Ah, no, I don't (checked; it may not be). I really don't know about this sort of > thing. How do I start it up? That's the thing. Loading the native library is quite expensive (around 300ms I think?), and broadcasts are received on the UI thread, so you don't really want to block it while you're initializing the native side. I think you'll just have to make do with staying on the Android side of things. You can use the Android SharedPreferences class to store your values, and then call into Java from native to read it. https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... File chrome/browser/ntp_snippets/ntp_snippets_prefs.h (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/browser/ntp_snip... chrome/browser/ntp_snippets/ntp_snippets_prefs.h:8: extern const char kContentSuggestionsConsecutiveIgnoredPrefName[]; On 2017/01/11 15:41:52, sfiera wrote: > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > Prefs in chrome/ go in chrome/common/pref_names.{h,cc} (this also goes for > > kNotificationIDWithinCategory, which I should have caught previously; sorry). > > Should the registration code also move somewhere else? No, registration is in the same place where the pref is used, just the names themselves are declared in a single location. > kNotificationIDWithinCategory is going to go away so that we can track multiple > notifications. Should it still move, if we're just going to get rid of it? No, that's fine then. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:59: auto prefs = profile->GetPrefs(); Use PrefService* in the other places as well? https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:76: static bool IsDisabledForProfile(Profile* profile) { Is this just a convenience method to get rid of "ntp_snippets::ContentSuggestionsNotificationHelper::"? It should probably go into an anonymous namespace instead of being marked static (the static is necessary below so that the methods can be declared in the generated header file, but not here), but then could you simply import it into the anonymous namespace? https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:81: static void NotificationTapped(JNIEnv* env, const JavaParamRef<jclass>&) { Add a commented out variable name for the class?
lgtm % comments https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:103: static void NotificationExpired(JNIEnv* env, const JavaParamRef<jclass>&) { You can use @JNINamespace("ntp_snippets") on the Java class and then put these in the same namespace as the rest of the file. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right): https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_metrics.h:8: #include "base/feature_list.h" Nit: Include not used?
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); On 2017/01/11 15:58:24, Bernhard Bauer wrote: > On 2017/01/11 15:41:51, sfiera wrote: > > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > > Do you actually know that the native library has been loaded at this point? > > What > > > if Chrome had been killed in the mean time? > > > > Ah, no, I don't (checked; it may not be). I really don't know about this sort > of > > thing. How do I start it up? > > That's the thing. Loading the native library is quite expensive (around 300ms I > think?), and broadcasts are received on the UI thread, so you don't really want > to block it while you're initializing the native side. I think you'll just have > to make do with staying on the Android side of things. You can use the Android > SharedPreferences class to store your values, and then call into Java from > native to read it. In the OpenUrlReceiver, we're going to be spinning up the native library anyway, to actually open the URL, if there's a way to not block. In the other two, I guess it would be good to not load the native library. I can store preferences on the Java side, but can I also update histograms? If not, is there a pattern for updating histograms later, when the native library starts up? I assume we could get bad timestamps on our data points, but probably not more than a day, given our fetching schedule. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:59: auto prefs = profile->GetPrefs(); On 2017/01/11 15:58:24, Bernhard Bauer wrote: > Use PrefService* in the other places as well? Done. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:76: static bool IsDisabledForProfile(Profile* profile) { On 2017/01/11 15:58:24, Bernhard Bauer wrote: > Is this just a convenience method to get rid of > "ntp_snippets::ContentSuggestionsNotificationHelper::"? It should probably go > into an anonymous namespace instead of being marked static (the static is > necessary below so that the methods can be declared in the generated header > file, but not here), but then could you simply import it into the anonymous > namespace? "using ContentSuggestionsNotificationHelper::IsDisabledForProfile" doesn't work because it's a class member. Since the static functions are now in the namespace, I moved the implementation to the anonymous namespace. The class member is now just exposing that function to other files. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:81: static void NotificationTapped(JNIEnv* env, const JavaParamRef<jclass>&) { On 2017/01/11 15:58:24, Bernhard Bauer wrote: > Add a commented out variable name for the class? Done. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:103: static void NotificationExpired(JNIEnv* env, const JavaParamRef<jclass>&) { On 2017/01/11 16:45:59, Alexei Svitkine (slow) wrote: > You can use @JNINamespace("ntp_snippets") on the Java class and then put these > in the same namespace as the rest of the file. That's great, thanks. https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/ntp_sni... File chrome/browser/ntp_snippets/ntp_snippets_metrics.h (right): https://codereview.chromium.org/2626643002/diff/100001/chrome/browser/ntp_sni... chrome/browser/ntp_snippets/ntp_snippets_metrics.h:8: #include "base/feature_list.h" On 2017/01/11 16:45:59, Alexei Svitkine (slow) wrote: > Nit: Include not used? Done.
https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); On 2017/01/12 11:26:23, sfiera wrote: > On 2017/01/11 15:58:24, Bernhard Bauer wrote: > > On 2017/01/11 15:41:51, sfiera wrote: > > > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > > > Do you actually know that the native library has been loaded at this > point? > > > What > > > > if Chrome had been killed in the mean time? > > > > > > Ah, no, I don't (checked; it may not be). I really don't know about this > sort > > of > > > thing. How do I start it up? > > > > That's the thing. Loading the native library is quite expensive (around 300ms > I > > think?), and broadcasts are received on the UI thread, so you don't really > want > > to block it while you're initializing the native side. I think you'll just > have > > to make do with staying on the Android side of things. You can use the Android > > SharedPreferences class to store your values, and then call into Java from > > native to read it. > > In the OpenUrlReceiver, we're going to be spinning up the native library anyway, > to actually open the URL, if there's a way to not block. What do you mean by not block? > In the other two, I > guess it would be good to not load the native library. Yup. (The difference in openUrl() is that it starts a new activity, which can do all sorts of things, as it is user-visible and in the foreground. The receiver itself OTOH runs hidden in the background, so doing long-running and/or intensive work in there is discouraged). > I can store preferences on the Java side, but can I also update histograms? If > not, is there a pattern for updating histograms later, when the native library > starts up? I assume we could get bad timestamps on our data points, but probably > not more than a day, given our fetching schedule. There is CachedMetrics for recording metrics before native startup, but that only caches in memory, so you'd have to also serialize the queued up metrics in Java. Do we actually use any metrics with timestamps? AFAIK, histograms just get accumulated without recording a timestamp (UserMetricsAction use timestamps, but we don't seem to record any of these). https://codereview.chromium.org/2626643002/diff/120001/chrome/browser/android... File chrome/browser/android/ntp/content_suggestions_notification_helper.cc (right): https://codereview.chromium.org/2626643002/diff/120001/chrome/browser/android... chrome/browser/android/ntp/content_suggestions_notification_helper.cc:92: const JavaParamRef<jclass> /* class_object */&) { Put the ampersand before the variable name?
It's preferable to not load the native library, but how important is it? It looks like existing notification code already loads it on dismissal, and I'm not sure I have the Android expertise to write the component we need to track metrics without it. https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java (right): https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: nativeNotificationTapped(); On 2017/01/12 12:13:52, Bernhard Bauer wrote: > On 2017/01/12 11:26:23, sfiera wrote: > > On 2017/01/11 15:58:24, Bernhard Bauer wrote: > > > On 2017/01/11 15:41:51, sfiera wrote: > > > > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > > > > Do you actually know that the native library has been loaded at this > > point? > > > > What > > > > > if Chrome had been killed in the mean time? > > > > > > > > Ah, no, I don't (checked; it may not be). I really don't know about this > > sort > > > of > > > > thing. How do I start it up? > > > > > > That's the thing. Loading the native library is quite expensive (around > 300ms > > I > > > think?), and broadcasts are received on the UI thread, so you don't really > > want > > > to block it while you're initializing the native side. I think you'll just > > have > > > to make do with staying on the Android side of things. You can use the > Android > > > SharedPreferences class to store your values, and then call into Java from > > > native to read it. > > > > In the OpenUrlReceiver, we're going to be spinning up the native library > anyway, > > to actually open the URL, if there's a way to not block. > > What do you mean by not block? Here, it's not safe yet to call the native method, because openUrl() is asynchronous. If we could schedule something to update the metrics later, without blocking inside this function, it would be fine—basically, CachedMetrics would work perfectly here, I think. > > In the other two, I > > guess it would be good to not load the native library. > > Yup. (The difference in openUrl() is that it starts a new activity, which can do > all sorts of things, as it is user-visible and in the foreground. The receiver > itself OTOH runs hidden in the background, so doing long-running and/or > intensive work in there is discouraged). What about notifications from websites? They seem to be hooked up to call into native code when dismissed, but don't display anything. > > I can store preferences on the Java side, but can I also update histograms? If > > not, is there a pattern for updating histograms later, when the native library > > starts up? I assume we could get bad timestamps on our data points, but > probably > > not more than a day, given our fetching schedule. > > There is CachedMetrics for recording metrics before native startup, but that > only caches in memory, so you'd have to also serialize the queued up metrics in > Java. So, for the dismiss/timeout cases, CachedMetrics is helpful as an example, but isn't usable in our own implementation, is it? What should I be looking at for the serialization? I feel like I'm out of my element here, with my primitive mental model of Chrome as either having started up or not :) > Do we actually use any metrics with timestamps? AFAIK, histograms just get > accumulated without recording a timestamp (UserMetricsAction use timestamps, but > we don't seem to record any of these). It's not explicitly recorded, but something lets UMA graph histograms over time, so there's some sort of implicit timestamp (from collection time, maybe). If we wanted to look at the timeline of a metric, we might want to keep in mind that it could be off by a day, but off by a day is probably fine.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/12 12:50:25, sfiera wrote: > It's preferable to not load the native library, but how important is it? It > looks like existing notification code already loads it on dismissal, and I'm not > sure I have the Android expertise to write the component we need to track > metrics without it. > > https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java > (right): > > https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: > nativeNotificationTapped(); > On 2017/01/12 12:13:52, Bernhard Bauer wrote: > > On 2017/01/12 11:26:23, sfiera wrote: > > > On 2017/01/11 15:58:24, Bernhard Bauer wrote: > > > > On 2017/01/11 15:41:51, sfiera wrote: > > > > > On 2017/01/11 15:21:49, Bernhard Bauer wrote: > > > > > > Do you actually know that the native library has been loaded at this > > > point? > > > > > What > > > > > > if Chrome had been killed in the mean time? > > > > > > > > > > Ah, no, I don't (checked; it may not be). I really don't know about this > > > sort > > > > of > > > > > thing. How do I start it up? > > > > > > > > That's the thing. Loading the native library is quite expensive (around > > 300ms > > > I > > > > think?), and broadcasts are received on the UI thread, so you don't really > > > want > > > > to block it while you're initializing the native side. I think you'll just > > > have > > > > to make do with staying on the Android side of things. You can use the > > Android > > > > SharedPreferences class to store your values, and then call into Java from > > > > native to read it. > > > > > > In the OpenUrlReceiver, we're going to be spinning up the native library > > anyway, > > > to actually open the URL, if there's a way to not block. > > > > What do you mean by not block? > > Here, it's not safe yet to call the native method, because openUrl() is > asynchronous. If we could schedule something to update the metrics later, > without blocking inside this function, it would be fine—basically, CachedMetrics > would work perfectly here, I think. Ah yes. In fact, CachedMetrics will submit the metrics automatically on startup, so you could just directly use it. > > > In the other two, I > > > guess it would be good to not load the native library. > > > > Yup. (The difference in openUrl() is that it starts a new activity, which can > do > > all sorts of things, as it is user-visible and in the foreground. The receiver > > itself OTOH runs hidden in the background, so doing long-running and/or > > intensive work in there is discouraged). > > What about notifications from websites? They seem to be hooked up to call into > native code when dismissed, but don't display anything. I'm not sure what tradeoffs went into that decision, but for this instance it seems excessive to start the native side for any interaction with any notification, in particular if that's only needed to record metrics. > > > I can store preferences on the Java side, but can I also update histograms? > If > > > not, is there a pattern for updating histograms later, when the native > library > > > starts up? I assume we could get bad timestamps on our data points, but > > probably > > > not more than a day, given our fetching schedule. > > > > There is CachedMetrics for recording metrics before native startup, but that > > only caches in memory, so you'd have to also serialize the queued up metrics > in > > Java. > > So, for the dismiss/timeout cases, CachedMetrics is helpful as an example, but > isn't usable in our own implementation, is it? What should I be looking at for > the serialization? I think implementing a generic mechanism to serialize cached metrics might be a bit overkill -- can we just store some flags in Android SharedPreferences and flush them on native startup? > I feel like I'm out of my element here, with my primitive mental model of Chrome > as either having started up or not :) > > > Do we actually use any metrics with timestamps? AFAIK, histograms just get > > accumulated without recording a timestamp (UserMetricsAction use timestamps, > but > > we don't seem to record any of these). > > It's not explicitly recorded, but something lets UMA graph histograms over time, > so there's some sort of implicit timestamp (from collection time, maybe). If we > wanted to look at the timeline of a metric, we might want to keep in mind that > it could be off by a day, but off by a day is probably fine. Hm, but that time is the submission time to the server, right? I guess it could still happen that we miss an upload window and the metrics therefore land in the bucket for the next day, but yeah, I'd be okay with that (in particular as upload is always delayed a bit -- we would just increase the delay).
Sounds good. Should I just do the flush as part of our KeyedService's initialization, or is there a better way to listen for native startup? On Thu, Jan 12, 2017 at 2:42 PM, <bauerb@chromium.org> wrote: > On 2017/01/12 12:50:25, sfiera wrote: >> It's preferable to not load the native library, but how important is it? >> It >> looks like existing notification code already loads it on dismissal, and >> I'm > not >> sure I have the Android expertise to write the component we need to track >> metrics without it. >> >> > https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... >> File >> > chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java >> (right): >> >> > https://codereview.chromium.org/2626643002/diff/80001/chrome/android/java/src... >> > chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotificationHelper.java:45: >> nativeNotificationTapped(); >> On 2017/01/12 12:13:52, Bernhard Bauer wrote: >> > On 2017/01/12 11:26:23, sfiera wrote: >> > > On 2017/01/11 15:58:24, Bernhard Bauer wrote: >> > > > On 2017/01/11 15:41:51, sfiera wrote: >> > > > > On 2017/01/11 15:21:49, Bernhard Bauer wrote: >> > > > > > Do you actually know that the native library has been loaded at >> > > > > > this >> > > point? >> > > > > What >> > > > > > if Chrome had been killed in the mean time? >> > > > > >> > > > > Ah, no, I don't (checked; it may not be). I really don't know >> > > > > about > this >> > > sort >> > > > of >> > > > > thing. How do I start it up? >> > > > >> > > > That's the thing. Loading the native library is quite expensive >> > > > (around >> > 300ms >> > > I >> > > > think?), and broadcasts are received on the UI thread, so you don't > really >> > > want >> > > > to block it while you're initializing the native side. I think >> > > > you'll > just >> > > have >> > > > to make do with staying on the Android side of things. You can use >> > > > the >> > Android >> > > > SharedPreferences class to store your values, and then call into >> > > > Java > from >> > > > native to read it. >> > > >> > > In the OpenUrlReceiver, we're going to be spinning up the native >> > > library >> > anyway, >> > > to actually open the URL, if there's a way to not block. >> > >> > What do you mean by not block? >> >> Here, it's not safe yet to call the native method, because openUrl() is >> asynchronous. If we could schedule something to update the metrics later, >> without blocking inside this function, it would be fine—basically, > CachedMetrics >> would work perfectly here, I think. > > Ah yes. In fact, CachedMetrics will submit the metrics automatically on > startup, > so you could just directly use it. > >> > > In the other two, I >> > > guess it would be good to not load the native library. >> > >> > Yup. (The difference in openUrl() is that it starts a new activity, >> > which > can >> do >> > all sorts of things, as it is user-visible and in the foreground. The > receiver >> > itself OTOH runs hidden in the background, so doing long-running and/or >> > intensive work in there is discouraged). >> >> What about notifications from websites? They seem to be hooked up to call >> into >> native code when dismissed, but don't display anything. > > I'm not sure what tradeoffs went into that decision, but for this instance > it > seems excessive to start the native side for any interaction with any > notification, in particular if that's only needed to record metrics. > >> > > I can store preferences on the Java side, but can I also update > histograms? >> If >> > > not, is there a pattern for updating histograms later, when the native >> library >> > > starts up? I assume we could get bad timestamps on our data points, >> > > but >> > probably >> > > not more than a day, given our fetching schedule. >> > >> > There is CachedMetrics for recording metrics before native startup, but >> > that >> > only caches in memory, so you'd have to also serialize the queued up >> > metrics >> in >> > Java. >> >> So, for the dismiss/timeout cases, CachedMetrics is helpful as an example, >> but >> isn't usable in our own implementation, is it? What should I be looking at >> for >> the serialization? > > I think implementing a generic mechanism to serialize cached metrics might > be a > bit overkill -- can we just store some flags in Android SharedPreferences > and > flush them on native startup? > >> I feel like I'm out of my element here, with my primitive mental model of > Chrome >> as either having started up or not :) >> >> > Do we actually use any metrics with timestamps? AFAIK, histograms just >> > get >> > accumulated without recording a timestamp (UserMetricsAction use >> > timestamps, >> but >> > we don't seem to record any of these). >> >> It's not explicitly recorded, but something lets UMA graph histograms over > time, >> so there's some sort of implicit timestamp (from collection time, maybe). >> If > we >> wanted to look at the timeline of a metric, we might want to keep in mind >> that >> it could be off by a day, but off by a day is probably fine. > > Hm, but that time is the submission time to the server, right? I guess it > could > still happen that we miss an upload window and the metrics therefore land in > the > bucket for the next day, but yeah, I'd be okay with that (in particular as > upload is always delayed a bit -- we would just increase the delay). > > https://codereview.chromium.org/2626643002/ -- 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.
On 2017/01/12 14:06:18, sfiera wrote: > Sounds good. Should I just do the flush as part of our KeyedService's > initialization, or is there a better way to listen for native startup? I think either would work, but doing it from native KeyedService initialization is probably easier, because there we already have code that runs on browser startup (Java code can get notified about native startup, but it requires plumbing a callback). Thanks!
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
OK! Ready for another look.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by sfiera@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM, thanks!
The CQ bit was checked by sfiera@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2626643002/#ps150001 (title: "String.equals()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1484308971397050,
"parent_rev": "eefdba8c290aa873aba574b2458ae689d65f1397", "commit_rev":
"3cf8939974888a7d5425c244f8962822a642d346"}
Message was sent while issue was closed.
Description was changed from
==========
Stop showing notifications if they’re ignored
Record metrics for interactions with notifications. Stop exposing
openUrl() to native code, but keep it around; we use our own intent for
notification taps to track metrics now.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
==========
to
==========
Stop showing notifications if they’re ignored
Record metrics for interactions with notifications. Stop exposing
openUrl() to native code, but keep it around; we use our own intent for
notification taps to track metrics now.
There are at three problems exposed by the metrics in their current
state:
* At (first?) startup, we get multiple consecutive fetches for some
reason. This results in two impressions, although the notifications
have the same data, and Android only shows one. (In practice, users
aren’t likely to frantically tap away on startup, the way I do, so
these notifications would be suppressed by the frontmost-checking,
which I’m trying to avoid)
* None of the “HIDE” actions are conditional on the notification still
being visible, so we record a HIDE_DEADLINE for each notification,
even if the user tapped or swiped it away, and we record
HIDE_FRONTMOST every time Chrome becomes frontmost, even if there’s
no notification.
Broadly speaking, we want the “Actions” histogram to match the units of
the “Impressions” histogram, so that we can infer the fraction of
displayed notifications that received each action. To do that, we should
keep a list of which notifications we believe are being shown in prefs
(to support Android L, which doesn’t have the getActiveNotifications()
API) …in the next CL.
BUG=675561
Review-Url: https://codereview.chromium.org/2626643002
Cr-Commit-Position: refs/heads/master@{#443550}
Committed:
https://chromium.googlesource.com/chromium/src/+/3cf8939974888a7d5425c244f896...
==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/chromium/src/+/3cf8939974888a7d5425c244f896... |
