Now we know how many people see about:newtab, but it would be nice to know how many people interact with about:newtab. At a minimum, knowing the number of clicks on newtab tiles per session would be useful. Even better, knowing the number of clicks per tile (numbered 1-9 from top-left to bottom-right) might give us an idea of how good our algorithm is for choosing what to display where.

Are there always 9 tiles?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1) > Are there always 9 tiles? That's the 'default' but can be changed by the end user: Prefs: browser.newtabpage.columns browser.newtabpage.rows

In addition to that, if NEWTAB_PAGE_ENABLED is false, there may be 0 (visible) tiles. But that might be accounted for by a different bug. :)

How about adding a probe that only counts the first 9 tiles normally (enumerated with 9+1 or exponential with high=10)?

If we are fine with the above plus gathering column & row count, this seems to do it.

I can't think of any particular problems with the above. :) Thanks!

bsmedberg suggested just putting all non-default row/column configurations in the same bucket, which sounds much better as we don't need to map against row/columns anymore. So this just adds NEWTAB_PAGE_SITE_CLICKED, it counts the clicked indices (0-8) for default row/column configurations. All non-default configs are counted as index 9.

Comment on attachment 8380717 [details] [diff] [review] Probe for about:newtab tile interaction Review of attachment 8380717 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/sites.js @@ +169,5 @@ > controls[i].addEventListener("click", this, false); > + > + let link = this.node.querySelector(".newtab-link"); > + let that = this; > + link.addEventListener("click", function Site_trackSiteClick(aEvent) { Please add a click event listener to this.node and check event.target to see which site has been clicked. Also it would be cleaner to use "this" as the event handler. It might be better to remove the code above that adds event listeners to all the controls and instead just have one handler that checks class names of event.target and does the right thing then. ::: toolkit/components/telemetry/Histograms.json @@ +4045,5 @@ > }, > + "NEWTAB_PAGE_SITE_CLICKED": { > + "expires_in_version": "35", > + "kind": "enumerated", > + "n_values": 9, 0-9 sounds like 10 values to me, no?

I think it might also be interesting to measure how often about:newtab is shown with actually blank tiles, as "Directory Tiles" would only be shown there. Is there a bug to make/adjust a probe for that?

This should be better. The usage of .parentElement is due to the clicks happening on the span elements here: http://hg.mozilla.org/mozilla-central/annotate/e3daaa4c73dd/browser/base/content/newtab/grid.js#l126

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #9) > Is there a bug to make/adjust a probe for that? I don't think there is one yet.

Comment on attachment 8381664 [details] [diff] [review] Probe for about:newtab tile interaction, v2 Review of attachment 8381664 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/sites.js @@ +167,4 @@ > > let controls = this.node.querySelectorAll(".newtab-control"); > for (let i = 0; i < controls.length; i++) > controls[i].addEventListener("click", this, false); We don't need this as it's handled by the catch-all click listener. @@ +183,5 @@ > * Handles all site events. > */ > handleEvent: function Site_handleEvent(aEvent) { > switch (aEvent.type) { > case "click": Can you please move this into a separate function, like this.onClick()? The switch statement is getting messy. @@ +184,5 @@ > */ > handleEvent: function Site_handleEvent(aEvent) { > switch (aEvent.type) { > case "click": > + if (aEvent.target.parentElement.classList.contains("newtab-link")) { We should check if the target or its parent element are .newtab-link.

This is hopefully fine now.

Comment on attachment 8383082 [details] [diff] [review] Probe for about:newtab tile interaction, v3 Review of attachment 8383082 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/sites.js @@ +179,5 @@ > + * Handles site click events. > + */ > + _onClick: function Site_onClick(aEvent) { > + if (aEvent.target.classList.contains("newtab-link") || > + aEvent.target.parentElement.classList.contains("newtab-link")) { It would be better to have a variable for the target here |let target = aEvent.target|. @@ +191,5 @@ > + } > + Services.telemetry.getHistogramById("NEWTAB_PAGE_SITE_CLICKED") > + .add(index); > + return; > + } Please move all this code in the if branch to another function, like this._recordSiteClicked() or similar.

Comment on attachment 8383162 [details] [diff] [review] Probe for about:newtab tile interaction, v4 Review of attachment 8383162 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/newtab/sites.js @@ +196,5 @@ > + _onClick: function Site_onClick(aEvent) { > + let target = aEvent.target; > + if (target.classList.contains("newtab-link") || > + target.parentElement.classList.contains("newtab-link")) { > + this._recordSiteClicked(this.cell.index); Passing this.cell.index here is a little weird because we can easily access it from _recordSiteClicked() but it's also doesn't really disturb me :)

(In reply to Tim Taubert [:ttaubert] from comment #16) > Passing this.cell.index here is a little weird because we can easily access > it from _recordSiteClicked() but it's also doesn't really disturb me :) Minor point, i know, but: If we're factoring this part out it could just as well not depend on where exactly the index is coming from :)

This is in a rather different part than the about:newtab impressions probe, so not blocking it's improvement.

bsmedberg, would we also want this uplifted to 29?

Yes, after we verify the data in nightly.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #21) > Yes, after we verify the data in nightly. ni? for that so we don't lose track of this.

I grabbed a dataset yesterday and just posted to firefox-dev about it. It looks like the system works, so we should uplift this to 29 to get it for 29b1 next week.

Comment on attachment 8383162 [details] [diff] [review] Probe for about:newtab tile interaction, v4 [Approval Request Comment] Bug caused by (feature/regressing bug #): n/a User impact if declined: No metrics on about:newtab interactions. Testing completed (on m-c, etc.): baked on m-c, data sanity-checked. Risk to taking this patch (and alternatives if risky): Low-risk, just adds telemtry probe and does a little required refactoring. String or IDL/UUID changes made by this patch: none.

Verified on Windows 7 x64, Ubuntu 13.10 x86, Mac OS 10.8, using: - latest Nightly 31.0a1 (build ID: 20140324030203) - latest Aurora 30.0a2 (build ID: 20140324150430) - Firefox 29 beta 2 (build ID: 20140324101726) Clicks per tile are recorded with indices (0-8) when using the default configuration, and with index 9 whenever using non-default configuration.

Re comment