Benjamin Smedberg Reporter
	Description • 11 years ago

We're going to be doing some experiments with the new-tab page, but we don't seem to have good data on how many people actually see the new-tab page. We should measure that!

* middle-clicking a link into a new tab should not trigger the counter, nor should opening a new window unless about:newtab is the user's home page instead of about:home
* opening a new tab with control-t or with the mouse should trigger the counter

Note: we already have NEWTAB_PAGE_PINNED_SITES_COUNT/NEWTAB_PAGE_ENABLED/NEWTAB_PAGE_BLOCKED_SITES_COUNT, but those don't actually measure usage. This measure is intended to be about usage.

This is a one-time measurement for now, so please set it to expire after a couple releases.

gfritzsche can you take this?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 1 • 11 years ago

Taking a look today.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 2 • 11 years ago

ttaubert managed to really simplify the handling for about:newtab visibility in bug 972341, this patch depends on the patches there.

	Benjamin Smedberg Reporter
	Comment 3 • 11 years ago

I was hoping to uplift this to 29 or even 28, actually. How complex are the dependencies?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 4 • 11 years ago

Alright, it's rather unlikely that we'll get this on 28 with the changes in bug 972341, so i'll do a seperate patch going for this.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 5 • 11 years ago

Tim, i tried this for a bit and still gPage._init() gets hit for every new window after a slight delay via this path:
http://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/content/newtab/newTab.js#l58
http://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/content/newtab/page.js#l29
http://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/content/newtab/page.js#l85

Do you have any ideas?

	Tim Taubert [:ttaubert] (inactive)
	Comment 6 • 11 years ago

Yes, gPage.init() is called for pages that are actually visible and for pages preloaded in the background. You need to insert your code here:

https://hg.mozilla.org/mozilla-central/annotate/a62bde1d6efe/browser/base/content/newtab/page.js#l87

So that it's executed only if this.allowBackgroundCaptures==true. This is then called for tabs opened through gBrowser.addTab(). As dicussed, you need a separate piece of code for the homepage case and you won't catch any loads caused by typing "about:newtab" into your url bar.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 11 years ago

Oh, i completely missed that it should go into the mutation observer :(
Thanks.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 8 • 11 years ago

This looks good in my local testing. Tim, can you review this?

As mentioned, this doesn't cover directly navigating to about:newtab, which does not seem important.
It also doesn't cover about:newtab displays in a OOP Window, but i assume that's not important either at this time.

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 11 years ago

Comment on attachment 8375669 [details] [diff] [review]
Add probe for when about:newtab is shown, v2

Review of attachment 8375669 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. I can't say much about the Telemetry histogram and I assume you made sure by talking to people that this will be able to display data as needed :) I also don't have an opinion about the expiration version number and assume you have confirmed that as well.

Another thought: the mutation observer is only called when the newtab page is enabled, i.e. the tiles are shown. The change in browser.js however adds a value to the histogram even if browser.newtabpage.enabled=false. As there is no good way of checking that here I think you should just check the pref as well.

This all would be so much easier with the dependency fixed :)

::: toolkit/components/telemetry/Histograms.json
@@ +4016,5 @@
>    },
> +  "NEWTAB_PAGE_SHOWN": {
> +    "expires_in_version": "35",
> +    "kind": "boolean",
> +    "description": "Count of when about:newtab was shown."

Maybe "Number of times about:newtab was shown." ?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 10 • 11 years ago

Ah, good point on the pref. How about this?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 11 • 11 years ago

Comment on attachment 8376178 [details] [diff] [review]
Add probe for when about:newtab is shown, v3

bsmedberg, can you sign off on the telemetry parts?

	Benjamin Smedberg Reporter
	Comment 12 • 11 years ago

Comment on attachment 8376178 [details] [diff] [review]
Add probe for when about:newtab is shown, v3

I'm a little surprised by "or window" in the histogram description because when I open a new window I get about:home not about:newtab. Is that something which happens by default or is it something that only happens if you change your homepage from about:home to about:newtab?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 11 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> Is that
> something which happens by default or is it something that only happens if
> you change your homepage from about:home to about:newtab?

I only meant to describe the case where you change the homepage to about:newtab.

	Tim Taubert [:ttaubert] (inactive)
	Comment 14 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #13)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)
> > Is that
> > something which happens by default or is it something that only happens if
> > you change your homepage from about:home to about:newtab?
> 
> I only meant to describe the case where you change the homepage to
> about:newtab.

This does also come in handy should we decide/get around to fix bug 725579.

	Tim Taubert [:ttaubert] (inactive)
	Comment 15 • 11 years ago

Comment on attachment 8376178 [details] [diff] [review]
Add probe for when about:newtab is shown, v3

Review of attachment 8376178 [details] [diff] [review]:
-----------------------------------------------------------------

Did some quick checks whether we add a value to the histogram in all cases and the patch works as expected, thanks!

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 16 • 11 years ago

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

	Phil Ringnalda (:philor)
	Comment 17 • 11 years ago

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 18 • 11 years ago

Comment on attachment 8376178 [details] [diff] [review]
Add probe for when about:newtab is shown, v3

Requesting uplift approval per comment 3.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: No metrics on actual about:newtab impressions.
Testing completed (on m-c, etc.): Checked expected use-cases, now baking on m-c.
Risk to taking this patch (and alternatives if risky): Low-risk, it just adds a new telemetry probe without changing existing behaviour.
String or IDL/UUID changes made by this patch: None.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/e18e57eeb6b7
https://hg.mozilla.org/releases/mozilla-beta/rev/ecb9afc1ea95

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 20 • 11 years ago

I would like to see this verified on Beta and Aurora once the builds are out so we can trust the data.
We can check the counts in about:telemetry, "Histograms" - filter with e.g. "newtab".

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ecb9afc1ea95

	Tracy Walker [:tracy]
	Comment 22 • 11 years ago

From last Nightly with new profile, one new tab opened:

NEWTAB_PAGE_SHOWN
1 samples, average = 1, sum = 1

0 |  0  0%
1 |#########################  1  100%

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 23 • 11 years ago

For the Beta & Aurora verification it would be nice to confirm that we don't count opening a new tab with the newtab page disabled (either by directly setting browser.newtabpage.enabled=false or using the top-right icon on the newtab page).

	u279076
	Comment 24 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #23)
> For the Beta & Aurora verification it would be nice to confirm that we don't
> count opening a new tab with the newtab page disabled (either by directly
> setting browser.newtabpage.enabled=false or using the top-right icon on the
> newtab page).

This bug is not deemed a high priority to verify before we release Firefox 28. Is there some reason we should re-evaluate that position?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 25 • 11 years ago

I mainly added the details for people checking here due to "good first verify".
This is not critical but might be good to check this works the same in Beta & Aurora before basing decisions on the data.

	u279076
	Comment 26 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> This is not critical but might be good to check this works the same in Beta
> & Aurora before basing decisions on the data.

Fair enough, flagging this for verification along those lines.

	Ada [:adalucinet]
	Comment 27 • 11 years ago

Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0

On Firefox 28 beta 6 (Build ID: 20140224220227) and latest Aurora from 2014-02-26, about:telemetry/Histograms works as expected, the counter is triggered when I open a New tab (File->New tab; ctrl/cmd+T or click on + button) or when the homepage is set to about:newtab in Options/General. With browser.newtabpage.enabled pref set to false, no results are displayed for NEWTAB_PAGE_SHOWN. Althought, if I:

1. Open a new tab page; NEWTAB_PAGE_SHOWN=1
2. Click on Hide the new tab page button (in the upper right corner); NEWTAB_PAGE_SHOWN=1
3. Close the new tab from step 2, open another new tab page and NEWTAB_PAGE_SHOWN=2 

For the next new tab pages opened, the counter is not triggered. Is this expected? Because when I click on Show the new tab page, the counter is triggered right away.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 28 • 11 years ago

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #27)
> For the next new tab pages opened, the counter is not triggered. Is this
> expected? Because when I click on Show the new tab page, the counter is
> triggered right away.

No, it's not expected but at least on Nightly it only triggers for the first new tab page that you open after that (does that match what you see on Beta and Aurora?).
On the other hand we don't count the impression when you click the "show the new tab page button" (only for new tabs afterwards).

If it matches the Nightly behaviour i don't think it matters enough to fix Beta/Aurora.

	Ada [:adalucinet]
	Comment 29 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #28)

> No, it's not expected but at least on Nightly it only triggers for the first
> new tab page that you open after that (does that match what you see on Beta
> and Aurora?).

Yes, it's exactly the same. 
Although, since for "Show the new tab page" button the counter is triggered with the next New tab page opened, shouldn't it be the same for "Hide the new tab page" button? The behavior should be consistent. 
Is there any bug logged on this matter or should I file one?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 30 • 11 years ago

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #29)
> Although, since for "Show the new tab page" button the counter is triggered
> with the next New tab page opened, shouldn't it be the same for "Hide the
> new tab page" button? The behavior should be consistent. 

This is overcounting by 1 for hiding (next new tab) and undercounting by 1 for showing (the new tab you just clicked on).
ttaubert and i didn't think it mattered enough to spend more time on this.