This is to add telemetry to discover how often certain options are used within the feed reader preview. This telemetry will also capture how often the preview is show to users. The telemetry should fire before a user hits the preview page to capture when they will be sent to a "always use" option. We need to be careful to establish how often the user might have used the preview page in the past to set preferences too. This will likely require two probes: - How often the user submits the form on the feed preview page (along with capturing what options they choose: application, live bookmark, web reader) - How often the user would see this page if they didn't have an application default. (along with what their default is: application, live bookmark, web reader) Regarding other probes for feeds: - Bug 1345546 is for discovering how often users see feed protocols on the web. - Bug 1347233 is related however restricted only to checking how often a user interacts with the "Subscribe to this page" button within firefox.

We should probably: - count loads of about:feeds - count submissions of the 'form' on about:feeds that subscribe with app/webapp/live bookmarks - count number of activations of "subscribe to this page" and/or its sub-items for pages that have more than 1 feed - count number of live bookmarks Note: we probably won't get meaningful numbers for what proportion of bookmarks are live bookmarks unless we add a specific probe for this, because the 'low' bucket for bookmarks count is 100, and covers 60+% of users, which means it'll be hard to combine the two histograms. We'd want a separate scalar/histogram to track this if that was something we wanted to do. I talked to folks in #telemetry, and in terms of getting data about the relative number of *clients* that submit this (rather than purely count of submissions), recording the about:feeds loads/submissions (and subscribe to this page clicks) in a scalar gets us into the main_summary table automatically, which will help with getting that data. For the live bookmarks, I think an opt-out histogram (like for 'normal' bookmarks) would do fine as long as we recorded in a granular fashion so we record 0 for people who have 0 live bookmarks (which I imagine will be the vast majority). Pretty OK with bucketing anything over 5 as "lots", on the assumption we count live bookmark roots (not items in the feed(s)). The final thing that I wonder if we should record is whether people actually click on items in their live bookmarks. Dolske and Marco, does that seem worth it? Anything else you'd like to see to help us make a decision on the future of feeds in Firefox (FFF) ? :-)

(In reply to :Gijs (slow, PTO recovery mode) from comment #2) > - count number of activations of "subscribe to this page" and/or its > sub-items for pages that have more than 1 feed That opens the preview page, and from there you still have to subscribe, so likely it is already accounted by the previous probes. > - count number of live bookmarks I don't think this is extremely useful by itself, what matters is their usage, not their existence, for example a user may have 2 live bookmarks he never uses, while another one could have 1 used everyday. We also used to have a live bookmark among default bookmarks, that could be completely unused and present in lots of old profiles. Maybe we should count something like the number of times a live bookmark is opened in a day. Also, per their nature live bookmarks are something seldomly added but frequently used, by the few users who can actually find the UI to add them. We'll surely measure low numbers, and thus I'm unsure what we would discover from that, is the cost worth the benefit? Bug 1345546 sounded surely more compelling. My opinion is that basing our decisions on these numbers won't be particularly useful, because we will measure usage of an half baked and never promoted feature and because of its "seldom addition" nature. Numbers could maybe be useful to drive a decision about a potential feature like news discovery (was not that part of Activity Stream and Pocket?). But most of that depends on the outcomes of Bug 1345546 already.

I assume about:feeds is prohibited from being iframed? (i.e. we don't need to differentiate top-level loads from iframes) Fine to do the telemetry together, but otherwise the possible removal of about:feeds should be disjoint from what to do with Live Bookmarks... It seems conceivable we could remove one but not the other, or that they might be removed on different timescales. > count number of activations of "subscribe to this page" and/or its sub-items for pages that have more than 1 feed Hmm, what's the usecase for adding this? I don't think it's directly relevant to our purposes: it is still usage if the user subscribes to any feed. [Maybe somehow useful for determining what a replacement addon API would need to do? Although IME it's common enough that if you want to support feeds, you need to allow selecting from 1 of N.] For LiveBookmarks, I also agree that usage is more important than just existence. So: - Number of times a Live Bookmarks folder is opened (in case usage is just passive title-checking) - Number of times an item within a Live Bookmark is clicked The count of LiveBookmarks might still be useful to put those numbers in context and understand the difference between casual users and heavy users. In the end it doesn't matter (w.r.t. usage) if you've got 1 LiveBookmark you click on 1000 times a day, or 1000 LiveBookmarks you click each day. But it could still be useful to know the existing count if the rate of Live Bookmark creation is so low that it's hard to understand usage by looking at just a few weeks of telemetry... I use a web-hosted feed reader almost daily, but only add feeds maybe a couple of times a year. If that. (But I don't use Firefox's UI to do that, so I'm still neutral ;). Do we have telemetry on how often users add _normal_ bookmarks? That would also be useful for context... As Marco notes, the challenge with this kind of stuff is that creating Live Bookmarks (even assuming it was a popular feature) was never going to be as common as, say, opening a tab or clicking links. And so it would be useful to be able to compare against something somewhat similar to understand usage. I'd expect that the absolute usage will still be low enough to justify removal, but that low relative usage will make the case watertight.

(In reply to Justin Dolske [:Dolske] from comment #4) > I assume about:feeds is prohibited from being iframed? (i.e. we don't need > to differentiate top-level loads from iframes) It should be, yes. > Fine to do the telemetry together, but otherwise the possible removal of > about:feeds should be disjoint from what to do with Live Bookmarks... It > seems conceivable we could remove one but not the other, or that they might > be removed on different timescales. OK, but if we remove about:feeds, we'll need to remove the UI entrypoints that open the feed from our live bookmarks UI (unless we want to show users the raw XML tree that this comes up with, which seems... unlikely. > > count number of activations of "subscribe to this page" and/or its sub-items for pages that have more than 1 feed > > Hmm, what's the usecase for adding this? I don't think it's directly > relevant to our purposes: it is still usage if the user subscribes to any > feed. [Maybe somehow useful for determining what a replacement addon API > would need to do? Although IME it's common enough that if you want to > support feeds, you need to allow selecting from 1 of N.] Hmm. This would be the UI entrypoints we have in various places -- but they all load about:feeds, I think, unless the user has somehow set it up so it immediately creates a live bookmark. (There is UI on about:feeds that suggests this is possible, but it sure doesn't work for me, and I suspect it's been broken ever since we did some e10s work on about:feeds to make it not be *completely* broken. > Do we have telemetry on how often users add _normal_ bookmarks? That would > also be useful for context... As Marco notes, the challenge with this kind > of stuff is that creating Live Bookmarks (even assuming it was a popular > feature) was never going to be as common as, say, opening a tab or clicking > links. And so it would be useful to be able to compare against something > somewhat similar to understand usage. I'd expect that the absolute usage > will still be low enough to justify removal, but that low relative usage > will make the case watertight. I don't know. I didn't see any from a casual look at telemetry histograms when I looked a while back, but it's possible I missed something. Marco?

(In reply to :Gijs from comment #5) > > Do we have telemetry on how often users add _normal_ bookmarks? Nothing I know about, we only have the number of bookmarks, that is not even precise due to things that are bookmarks just for technical reasons (tags for example). I'm also not sure how we'd measure "how often" a bookmark is added, what would we measure, days from the last addition? additions in the last month? How would we differentiate from bookmarks created by Sync (if we care to)?

(In reply to Marco Bonardo [::mak] from comment #6) > (In reply to :Gijs from comment #5) > > > Do we have telemetry on how often users add _normal_ bookmarks? > > Nothing I know about, we only have the number of bookmarks, that is not even > precise due to things that are bookmarks just for technical reasons (tags > for example). It's OK, the exponential histogram there means it's hard to draw any conclusions there anyway ;-) > I'm also not sure how we'd measure "how often" a bookmark is added, what > would we measure, days from the last addition? additions in the last month? I think we'd have a scalar or histogram that we'd report for each session indicating how many bookmarks / live bookmarks were added. Then we can just get the sum total across all users across a given set of builds, and the sum total across all users for live bookmarks across the same builds and in the same timeframe / set of submissions, and compare the two. :chutten, does that sound right? > How would we differentiate from bookmarks created by Sync (if we care to)? I guess I hope/assume that there is a clean point where we can make this differentiation. If that's too much work, I could live with counting the items created by sync as if they were also created on that machine. Given that the different machines each have their own telemetry client ID etc., this would slightly over-count created bookmarks for sync users, but I can live with that.

That'd do it. (Also: using FXA_CONFIGURED as a proxy for how many users may be using sync at any one time suggests that the number of users syncing bookmarks will be sufficiently low for a broad-strokes analysis of this type) Scalars would be ideal for this situation. One to record bookmark additions, one to add live bookmark additions.

Note- I would also like to know how many live bookmarks are in the bookmarks bar, assuming it's enabled.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, Marco and :chutten, could you please give some feedback on the approach(es) this patch takes? I haven't tested this at all, but it'd be useful to know if these measures make sense, if my use of telemetry scalar APIs is correct, and if I'm adding things at the right point in the 'stack' of where various actions happen. Separately, Marco, can you perhaps point me to an automated test that I can base a simple test (or two) on that check that these probes do what I expect them to do?

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, Phabricator will make everything better, I hope. Like not clearing feedback flags on attachments when updating the review request...

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review227200 It looks more or less OK, with some caveats. For tests, I don't think we have a comprehensive test for the livemarks interaction (from showing the preview to subscribing and visiting an entry). And the browser feeds UI is lacking tests in general. Places Telemetry has a test at toolkit/components/places/tests/unit/test_telemetry.js, but it's xpcshell, not good for testing ui events, may be ok for counts feeds discovery is at browser/base/content/test/general/browser_feed_discovery.js I don't think there's a test about the subscribe to feed through live bookmarks button, but you can directly use PlacesUtils.livemarks.addLivemark browser/components/places/tests/browser/browser_views_liveupdate.js may have some hints to open menus and find Places nodes ::: toolkit/components/places/PlacesDBUtils.jsm:1099 (Diff revision 2) > + async _telemetryForFeeds() { > + let db = await PlacesUtils.promiseDBConnection(); > + let attr_id = "(SELECT id FROM moz_anno_attributes WHERE name = 'livemark/feedURI')"; > + let liveBookmarkParents = await db.execute( > + `SELECT p.guid FROM moz_bookmarks b > + LEFT JOIN moz_items_annos a if you only care about livemarks, both LEFT JOIN should just be JOIN. Every livemark has both the anno and a parent. and you could also join moz_anno_attributes. If you want to shorten the query just keep the ON inline with the JOINs, I don't care that much if we are at 83 chars instead of 80... ::: toolkit/components/places/PlacesDBUtils.jsm:1110 (Diff revision 2) > + > + const xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore); > + if (xulStore.getValue("chrome://browser/content/browser.xul", > + "PersonalToolbar", "collapsed") == "false") { > + Services.telemetry.scalarSet("browser.feeds.liveBookmarkToolbarCount", > + liveBookmarkParents.filter(row => row.getResultByIndex(0) == PlacesUtils.bookmarks.toolbarGuid).length); I don't think this measures what we expect, this seems to only care about direct children of the toolbar (and it's tricky to get a whole hierarchy atm, in practice it's not measurable). In my case I have 12 RSS feeds into a folder named "RSS Feeds" that I keep on the toolbar, so I would be measured wrongly. I expect anyone having more than a couple feeds will likely keep them in a folder and not spread around. Indeed, I'd also not complain if these would disappear completely from Places views and just be listed by the RSS toolbarbutton. There's no strict reason why livemarks must be bookmarks...

(In reply to Marco Bonardo [::mak] from comment #14) > It looks more or less OK, with some caveats. > For tests, I don't think we have a comprehensive test for the livemarks > interaction (from showing the preview to subscribing and visiting an entry). OK, thanks, these hints were helpful anyway. > ::: toolkit/components/places/PlacesDBUtils.jsm:1110 > (Diff revision 2) > > + > > + const xulStore = Cc["@mozilla.org/xul/xulstore;1"].getService(Ci.nsIXULStore); > > + if (xulStore.getValue("chrome://browser/content/browser.xul", > > + "PersonalToolbar", "collapsed") == "false") { > > + Services.telemetry.scalarSet("browser.feeds.liveBookmarkToolbarCount", > > + liveBookmarkParents.filter(row => row.getResultByIndex(0) == PlacesUtils.bookmarks.toolbarGuid).length); > > I don't think this measures what we expect, this seems to only care about > direct children of the toolbar (and it's tricky to get a whole hierarchy > atm, in practice it's not measurable). Oh. That's unfortunate. Is there literally no reasonable way to get that information? Do these items get built when the bookmarks toolbar is displayed, and could we piggyback on that, for instance (so when we construct a livemark folder item that's a DOM descendant of the bookmarks toolbar, or whatever) ? Or does that happen entirely lazily?

(In reply to :Gijs from comment #15) > Oh. That's unfortunate. Is there literally no reasonable way to get that > information? Do these items get built when the bookmarks toolbar is > displayed, and could we piggyback on that, for instance (so when we > construct a livemark folder item that's a DOM descendant of the bookmarks > toolbar, or whatever) ? Or does that happen entirely lazily? From a menu view you could probably walk up the parent chain and see if you end up to the toolbar, but that menu view should be open... otherwise from the database you should use a recursive query to fetch all the parents, that is not particularly efficient (we lack an adjacency table from quite some time). The table is a flat (id, parent) one indeed. I don't see an efficient solution, if this runs on idle (as I suspect) maybe the best solution is the WITH RECURSIVE query walking up the parents chain in SQL...

(In reply to Marco Bonardo [::mak] from comment #16) > (In reply to :Gijs from comment #15) > > Oh. That's unfortunate. Is there literally no reasonable way to get that > > information? Do these items get built when the bookmarks toolbar is > > displayed, and could we piggyback on that, for instance (so when we > > construct a livemark folder item that's a DOM descendant of the bookmarks > > toolbar, or whatever) ? Or does that happen entirely lazily? > > From a menu view you could probably walk up the parent chain and see if you > end up to the toolbar, but that menu view should be open... otherwise from > the database you should use a recursive query to fetch all the parents, that > is not particularly efficient (we lack an adjacency table from quite some > time). The table is a flat (id, parent) one indeed. > I don't see an efficient solution, if this runs on idle (as I suspect) maybe > the best solution is the WITH RECURSIVE query walking up the parents chain > in SQL... OK. This seems like a bad idea, then. (In reply to Nick Nguyen [:osunick] from comment #9) > Note- I would also like to know how many live bookmarks are in the bookmarks > bar, assuming it's enabled. Nick, given the other telemetry we're collecting here (total number of live bookmarks, number of times people open live bookmark popups, number of times people open individual items, number of times people load feed preview pages, number of times people subscribe to a feed) and given that this data is inefficient to actually get for every user, how important is this data (and why) ? Would it be better to just leave that bit unimplemented?

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review227464 The use of scalars seems appropriate here. A couple of mentions of convention, but otherwise you should be good for data-r? (( Of course, I don't know if the scalars are recording what the descriptions claim, but that's why you have :mak )) ::: toolkit/components/telemetry/Scalars.yaml:267 (Diff revision 2) > record_in_processes: > - 'main' > > +# The following section contains (RSS/Atom) feed usage > +browser.feeds: > + previewLoaded: The convention for scalar probe names are snake_case. ::: toolkit/components/telemetry/Scalars.yaml:277 (Diff revision 2) > + expires: "65" > + kind: uint > + notification_emails: > + - gijs@mozilla.com > + release_channel_collection: opt-out > + record_in_processes: Is this accumualted to in chrome-only code? ::: toolkit/components/telemetry/Scalars.yaml:279 (Diff revision 2) > + notification_emails: > + - gijs@mozilla.com > + release_channel_collection: opt-out > + record_in_processes: > + - 'main' > + - 'content' A line of whitespace between scalar definitions is typical

I am fine with leaving the inefficient telemetry off, as I don't think many people are doing what I do with livemarks.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review227464 > Is this accumualted to in chrome-only code? This is recorded in system-privileged ("chrome") code that runs in the content process when e10s is on, and in the parent when e10s is off, so we need both processes listed to make this work.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, It's data collection review request time! What questions will you answer with this data? - how many people actively use the feed reader and live bookmarks functionality we ship, and which bits of that functionality (e.g. do people look at feeds but not subscribe, subscribe but not via Firefox, or use live bookmarks but never load the feed preview page anymore) Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements? Some example responses: - the feed reader code especially is a pain for performance and security reasons. The live bookmark and feed reader code is not well-tested and a significant portion of our bookmarks code, styling and UI could be simplified a bit if we removed live bookmarks as well. If we discover that these features are heavily used, we may make changes to improve performance and security, and better support users of those features. If we discover that they are almost entirely unused, we will likely remove all or part of the feature, thus aiding maintainability and freeing up developer time to work on things our users do use. We may consider providing some of the functionality via webextensions instead, which would then hopefully be more actively maintained by those people who have an interest in such features. What alternative methods did you consider to answer these questions? Why were they not sufficient? - we could have surveyed users, but this would likely have had sampling problems (also because it seems, purely from some initial google search popularity research, that some specific geographic regions may feature heavier use than others). Additionally we would likely have come up against difficulties in explaining to survey users what a feed is, or which bits of our user interface we refer to when talking about "live bookmarks" or "subscription". Ultimately technical measures seemed more likely to get us objective answers. Can current instrumentation answer these questions? - We hope so. We will be able to see the spread of the number of live bookmarks people have, and compare the number of loads of feed pages with the number of page loads in total, as well as comparing the number of users who load feed pages or subscribe to feeds with the number of total clients, etc. List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data collection categories on the found on the Mozilla wiki. Note that the data steward reviewing your request will characterize your data collection based on the highest (and most sensitive) category. - tracking bug for all of these is this bug (bug 1350349), and all data is category 2 (user interaction data and/or counts of items). Measures: - number of times the feed preview page loads - number of times the user clicks the 'subscribe' button in the feed preview page - number of live bookmarks the user has - number of live bookmarks the user has directly on the bookmarks toolbar (if open; will record 0 if bookmarks toolbar is hidden) - number of times the user opens a live bookmark popup - number of times the user clicks a feed-provided item in a live bookmark popup How long will this data be collected? Choose one of the following: - time-scoped for 5 releases initially. At that point I'd expect us to have a better idea about usage so we can formulate a plan to move forward, which will likely obsolete some or all of the measures. If we choose to put effort into some or all of these features, we may want to extend some of the probes to see if usage increases if usability etc. improves. What populations / channels / countries will you measure? - all channels/regions/locales/etc (no filters) If this data collection is default on, what is the opt-out mechanism for users? - regular telemetry opt-out Please provide a general description of how you will analyze this data. - Using the distribution/evolution tools on TMO, and with custom spark queries or other methods to see opt-out/release data. Where do you intend to share the results of your analysis? - that depends on the outcome of the analysis, but likely through bugs, maybe blogposts or release notes if warranted (both for decisions about removal or expending more effort on improving the feature).

(In reply to :Gijs from comment #22) > Can current instrumentation answer these questions? > - We hope so. We will be able to see the spread of the number of live > bookmarks people have, and compare the number of loads of feed pages with > the number of page loads in total, as well as comparing the number of users > who load feed pages or subscribe to feeds with the number of total clients, > etc. I think I may have misread this question. If it's supposed to be about the measures we already have - no, we don't have existing instrumentation that can answer these questions. There was some telemetry about custom feed protocols, which we removed when we removed those protocols.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, Re-setting the data-review request...

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review227200 > I don't think this measures what we expect, this seems to only care about direct children of the toolbar (and it's tricky to get a whole hierarchy atm, in practice it's not measurable). > In my case I have 12 RSS feeds into a folder named "RSS Feeds" that I keep on the toolbar, so I would be measured wrongly. I expect anyone having more than a couple feeds will likely keep them in a folder and not spread around. > > Indeed, I'd also not complain if these would disappear completely from Places views and just be listed by the RSS toolbarbutton. There's no strict reason why livemarks must be bookmarks... I decided to keep this as-is. We could equally remove it entirely if you prefer. Per discussion with Nick I don't think we care enough about this specific aspect of live bookmark use to make it worth doing a recursive sql query.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review228446 Data-review only > Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? Yes, Scalars.yml > Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available. Yes, Firefox controls > If the request is for permanent data collection, is there someone who will monitor the data over time? non-pemanent, expires in 65 > Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? Type 2 > Is the data collection request for default-on or default-off? default-on > Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)? no > Is the data collection covered by the existing Firefox privacy notice? Yes > Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate) Probes expire in 65

(In reply to :Gijs from comment #26) > I decided to keep this as-is. We could equally remove it entirely if you > prefer. Per discussion with Nick I don't think we care enough about this > specific aspect of live bookmark use to make it worth doing a recursive sql > query. I'd suggest to remove it. As it is it may be misread and bring to wrong decisions. There's nothing in the commit message or in the scalar description that clarifies this count is just the number of livemarks who are direct children of the toolbar and not in folders. And since it's a partial count, its value seems very low. I assume the original intent was "number of livemarks the user cares to have handy", but every user does that differently.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review228580 r=me without the partial toolbar count ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:4 (Diff revision 4) > +"use strict"; > + > + > + lots of empty space, and please add the PD license boilerplate. ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:21 (Diff revision 4) > + const kSubscribed = kScalarPrefix + "feed_subscribed"; > + const kLivemarkCount = kScalarPrefix + "livebookmark_count"; > + const kLivemarkOnToolbarCount = kScalarPrefix + "livebookmark_on_toolbar_count"; > + const kLivemarkOpened = kScalarPrefix + "livebookmark_opened"; > + const kLivemarkItemOpened = kScalarPrefix + "livebookmark_item_opened"; > + trailing spaces, but likely eslint would complain already :) ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:25 (Diff revision 4) > + const kLivemarkItemOpened = kScalarPrefix + "livebookmark_item_opened"; > + > + let scalarForContent = gMultiProcessBrowser ? "content" : "parent" > + let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, FEED_URI); > + // Ensure we get telemetry from the content process: > + await BrowserTestUtils.waitForCondition(() => { please use the TestUtils handle, we should really remove the BrowserTestUtils one... (Bug 940882) ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:40 (Diff revision 4) > + let old = PlacesCommandHook.addLiveBookmark; > + let createBMPromise = new Promise(resolve => { > + PlacesCommandHook.addLiveBookmark = function(...args) { resolve(args); }; > + }); > + registerCleanupFunction(() => PlacesCommandHook.addLiveBookmark = old); > + await BrowserTestUtils.synthesizeMouseAtCenter("#subscribeButton", {}, tab.linkedBrowser); Can we just use document.getElementById("subscribeButton").click()? ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:62 (Diff revision 4) > + registerCleanupFunction(async () => { > + for (let mark of livemarks) { > + await PlacesUtils.livemarks.removeLivemark(mark); > + } > + }); > + CustomizableUI.setToolbarVisibility("PersonalToolbar", true); registerCleanupFunction unsetting this? Ideally the test should also not assume the toolbar is not visible by default... but likely we have far more broken tests about that. ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:73 (Diff revision 4) > + "Should have created two livemarks and counted them."); > + Assert.equal(snapshot.parent[kLivemarkOnToolbarCount], 1, > + "Should have created the livemark in the bookmarks toolbar and counted it."); > + > + // Check we count opening the livemark popup: > + await BrowserTestUtils.waitForCondition( ditto ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:75 (Diff revision 4) > + "Should have created the livemark in the bookmarks toolbar and counted it."); > + > + // Check we count opening the livemark popup: > + await BrowserTestUtils.waitForCondition( > + () => document.querySelector("#PersonalToolbar .bookmark-item[livemark]")); > + let livemarkOnToolbar = document.querySelector("#PersonalToolbar .bookmark-item[livemark]"); I think waitForCondition resolves to the return value of the conditionFn, so you could let livemarkOnToolbar = await TestUtils.waitForCond... ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:78 (Diff revision 4) > + await BrowserTestUtils.waitForCondition( > + () => document.querySelector("#PersonalToolbar .bookmark-item[livemark]")); > + let livemarkOnToolbar = document.querySelector("#PersonalToolbar .bookmark-item[livemark]"); > + let popup = livemarkOnToolbar.querySelector("menupopup"); > + let popupShownPromise = BrowserTestUtils.waitForEvent(popup, "popupshown"); > + EventUtils.synthesizeMouseAtCenter(livemarkOnToolbar, {}); maybe click() works here too. ::: browser/components/feeds/test/browser/browser_telemetry_checks.js:85 (Diff revision 4) > + Assert.equal(getSnapShot().parent[kLivemarkOpened], 1, "Should count livemark opening"); > + > + // And opening an item in the popup: > + await BrowserTestUtils.waitForCondition( > + () => popup.querySelector("menuitem.bookmark-item")); > + popup.querySelector("menuitem.bookmark-item").doCommand(); ditto (TestUtils and the resolved value) ::: browser/components/places/PlacesUIUtils.jsm:752 (Diff revision 4) > where = "tab"; > } > > this._openNodeIn(aNode, where, window); > + let view = this.getViewForNode(aEvent.target); > + if (view.controller.hasCachedLivemarkInfo(aNode.parent)) { since getViewforNode can return null, just in case: if (view && view. ::: toolkit/components/places/PlacesDBUtils.jsm:1058 (Diff revision 4) > + let liveBookmarkParents = await db.execute( > + `SELECT p.guid FROM moz_bookmarks b > + JOIN moz_items_annos a ON a.item_id = b.id > + JOIN moz_anno_attributes aa ON a.anno_attribute_id = aa.id > + JOIN moz_bookmarks p ON b.parent = p.id > + WHERE aa.name = 'livemark/feedURI'`); you can speed up this query by adding WHERE b.type = :folder and folder: PlacesUtils.bookmarks.TYPE_FOLDER Also, if we don't collect count on the toolbar, you can avoid the parent join and do a simple count(*)

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, https://reviewboard.mozilla.org/r/221000/#review228580 > Can we just use document.getElementById("subscribeButton").click()? No, because the subscribe button lives in the content process. This does the click in the right process. > maybe click() works here too. It doesn't (work). I also tried .doCommand which also doesn't work. I kept it like this for now.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/36f9602d7654 collect data for feed reader usage to evaluate its future, r=mak,data-review=liuche

Marco, how would you feel about uplifting this so we get data sooner rather than later?

I don't have code concerns, the changes look harmless.

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: got to wait a long time to get information about feed reader usage, but also means we have to keep shipping and supporting and fixing security issues in the feed reader code for the lifetime of 60esr. This makes us slower. [Is this code covered by automated tests?]: yes! [Has the fix been verified in Nightly?]: automatically, yes. Also, manually, yes, because the telemetry dist viewer shows reasonable-looking values for all these scalars. [Needs manual test from QE? If yes, steps to reproduce]: nope [List of other uplifts needed for the feature/fix]: n/a [Is the change risky?]: not really [Why is the change risky/not risky?]: the code changes are straightforward, and have both automated test coverage and verification via tmo showing it works. Also, based on our existing knowledge from the feed/pcast protocol and the preliminary results from the telemetry on nightly, feeds aren't heavily used and so even in the unlikely event that anything would go wrong, impact would be very limited. [String changes made/needed]: nope

Comment on attachment 8951727 [details] Bug 1350349 - collect data for feed reader usage to evaluate its future, Kind of late in beta to add a bunch of telemetry but I like the idea of fixing feed reader bugs before the next ESR, so let's go for it.

Backed out changeset 63366815c258 (bug 1350349) for bc failures in browser/components/feeds/test/browser/browser_telemetry_checks.js on a CLOSED TREE Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&revision=72a0030b41dc6c325b178911d7ae5e20c1348dba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=164813708&repo=mozilla-beta&lineNumber=11424 Backout: https://hg.mozilla.org/releases/mozilla-beta/rev/47d2222746944315b59dd76e97640aef66df9b49

Ugh. The orange is a result of the promise from TestUtils.waitForCondition not resolving with the result from the test condition like it does on Nightly. That is, https://hg.mozilla.org/releases/mozilla-beta/rev/63366815c258d2be09d71785ab7aeba260245520#l6.29 + let previewCount = await TestUtils.waitForCondition(() => { + let snapshot = getSnapShot()[scalarForContent]; + return snapshot && snapshot[kPreviewLoaded]; + }); + Assert.equal(previewCount, 1, "Should register the preview in telemetry."); on Nightly, `previewCount` gets set because we resolve with `conditionPassed`: https://dxr.mozilla.org/mozilla-central/source/testing/modules/TestUtils.jsm#137 but we don't on beta: https://dxr.mozilla.org/mozilla-beta/source/testing/modules/TestUtils.jsm#139 This changed in bug 1428845. This is unfortunate, for obvious reasons... I'll create a beta-only patch that works around this.

(In reply to :Gijs from comment #35) > [Is this code covered by automated tests?]: yes! > [Has the fix been verified in Nightly?]: automatically, yes. Also, manually, > yes, because the telemetry dist viewer shows reasonable-looking values for > all these scalars. > [Needs manual test from QE? If yes, steps to reproduce]: nope Setting qe-verify- based on Gijs' assessment on manual testing needs and the fact that this fix has automated coverage.