use Services.telemetry.canRecordBase, enable snippets by default #3057
Conversation
There was a problem hiding this comment.
This looks good. In addition to the inline comments, two questions:
-
it seems surprising that the snippets telemetry is not governed by both the global telemetry pref and the a-s telemetry pref, the way TelemetrySender is. Maybe the snippets code wants to call into the TelemetrySender code to check for enablement, rather than calling into the Service directly?
-
Since we're turning on snippets by default, are there any prefs we need to touch in m-c so the mochitests from the rest of the tree don't start exploding with the "non-local network requests" error?
system-addon/lib/SnippetsFeed.jsm
Outdated
| onboardingFinished: Services.prefs.getBoolPref(ONBOARDING_FINISHED_PREF) | ||
| }; | ||
|
|
||
| this.store.dispatch(ac.BroadcastToContent({type: at.SNIPPETS_DATA, data})); | ||
| } | ||
| _refreshCanRecordBase() { | ||
| // TODO: There is currently no way to listen for changes to this value; | ||
| // bug is filed here https://bugzilla.mozilla.org/show_bug.cgi?id=1386318 |
There was a problem hiding this comment.
Maybe mention that for now, we're doing the best effort of calling this each time a new tab is initialized.
system-addon/lib/SnippetsFeed.jsm
Outdated
| @@ -15,6 +15,7 @@ XPCOMUtils.defineLazyModuleGetter(this, "ProfileAge", | |||
| // Url to fetch snippets, in the urlFormatter service format. | |||
| const SNIPPETS_URL_PREF = "browser.aboutHomeSnippets.updateUrl"; | |||
| const ONBOARDING_FINISHED_PREF = "browser.onboarding.notification.finished"; | |||
| const FXA_USERNAME_PREF = "services.sync.username"; | |||
There was a problem hiding this comment.
Does sending this need data review?
There was a problem hiding this comment.
Because it's already implemented in existing snippets, I would assume not
|
Do we want to hold off on merging this? Or have a temporary mozilla-central-patches to prevent pine failures see https://bugzilla.mozilla.org/show_bug.cgi?id=1386350 ? |
|
Eh, actually last time we had a separate PR to add the patch. That make it simple to revert that one PR. |
Fix #3056 and fix #3007.
I've updated the PR to also fix TelemetrySender and related tests, as well as updating the value on every new tab init.'
I also snuck a
.hasFxAccountproperty into snippets data