975570 - Measure with telemetry how many times people interact with about:newtab

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

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.

Comment 1

[Benjamin Smedberg]

Are there always 9 tiles?

Comment 2

[Jim Jeffery not reading bug-mail 1/2/11]

(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

Comment 3

[Blake Winton (:bwinton) (:☕️)]

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.  :)

Comment 4

[Georg Fritzsche [:gfritzsche]]

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Probes for tile interaction and row & column count

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

Comment 6

[Blake Winton (:bwinton) (:☕️)]

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

Thanks!

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Probe for about:newtab tile interaction

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 8

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 9

[Robert Kaiser]

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?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Probe for about:newtab tile interaction, v2

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

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Robert Kaiser (:[e-mail]) from comment #9)
> Is there a bug to make/adjust a probe for that?

I don't think there is one yet.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Probe for about:newtab tile interaction, v3

This is hopefully fine now.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

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 15

[Georg Fritzsche [:gfritzsche]]

Attachment: Probe for about:newtab tile interaction, v4

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

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 :)

Comment 17

[Georg Fritzsche [:gfritzsche]]

(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 :)

Comment 18

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bab472adec47

Comment 19

[Georg Fritzsche [:gfritzsche]]

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

Comment 20

[Georg Fritzsche [:gfritzsche]]

bsmedberg, would we also want this uplifted to 29?

Comment 21

[Benjamin Smedberg]

Yes, after we verify the data in nightly.

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bab472adec47

Comment 23

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 24

[Benjamin Smedberg]

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 25

[Georg Fritzsche [:gfritzsche]]

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.

Comment 26

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09432c0c759f

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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.

Comment 28

[Avi Halachmi (:avih)]

Re comment