Closed Bug 975570 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
firefox31 --- verified

People

(Reporter: bwinton, Assigned: gfritzsche)

References

Details

(Keywords: privacy-review-needed)

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
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.
Attachment #8380619 - Attachment is obsolete: true
Attachment #8380717 - Flags: review?(ttaubert)
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?
Attachment #8380717 - Flags: review?(ttaubert)
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
Attachment #8380717 - Attachment is obsolete: true
Attachment #8381664 - Flags: review?(ttaubert)
(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.
Attachment #8381664 - Flags: review?(ttaubert) → feedback+
This is hopefully fine now.
Attachment #8381664 - Attachment is obsolete: true
Attachment #8383082 - Flags: review?(ttaubert)
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.
Attachment #8383082 - Flags: review?(ttaubert) → feedback+
Attachment #8383082 - Attachment is obsolete: true
Attachment #8383162 - Flags: review?(ttaubert)
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 :)
Attachment #8383162 - Flags: review?(ttaubert) → review+
(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.
No longer blocks: 973532
bsmedberg, would we also want this uplifted to 29?
Flags: needinfo?(benjamin)
Yes, after we verify the data in nightly.
Flags: needinfo?(benjamin)
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/bab472adec47
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
(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.
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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.
Attachment #8383162 - Flags: approval-mozilla-aurora?
Attachment #8383162 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Re comment
You need to log in before you can comment on or make changes to this bug.