Paul Silaghi, QA [:pauly] Reporter
	Description • 10 years ago

"NEWTAB_PAGE_DIRECTORY_AFFILIATE_SHOWN"
"NEWTAB_PAGE_DIRECTORY_ORGANIC_SHOWN"
"NEWTAB_PAGE_DIRECTORY_SPONSORED_SHOWN"
is not updated when:
1. enter about:newtab in the location bar and press enter
2. open NTP, restart FF, press the "Restore Previous Session" button (is not counted after restart)
3. open NTP, set "show my windows and tabs from last time", restart FF (is not counted after restart)
4. set about:newtab as homepage and set when FF starts "show my home page"
5. set about:newtab as homepage, press the "home" icon from the toolbar

	Georg Fritzsche [:gfritzsche]
	Comment 1 • 10 years ago

Looks like these probes miss one of the spots NEWTAB_PAGE_SHOWN hooks into:
http://dxr.mozilla.org/mozilla-central/search?q=NEWTAB_PAGE_SHOWN&case=true

(In reply to Paul Silaghi, QA [:pauly] from comment #0)
> 1. enter about:newtab in the location bar and press enter

At least for NEWTAB_PAGE_SHOWN we decided we don't care about those (minor and specific technical use-case).

	Ed Lee :Mardak
	Comment 2 • 10 years ago

(In reply to Paul Silaghi, QA [:pauly] from comment #0)
> 2. open NTP, restart FF, press the "Restore Previous Session" button (is not
> counted after restart)
> 3. open NTP, set "show my windows and tabs from last time", restart FF (is
> not counted after restart)
> 4. set about:newtab as homepage and set when FF starts "show my home page"
> 5. set about:newtab as homepage, press the "home" icon from the toolbar
gfritzsche, are you saying the browser/base/content/browser.js probe covers all these items #2-5?

Any idea why the Page_init code doesn't cover those cases? Or ideas on a good way to reuse the telemetry probes in Page_init instead of copying it over to browser.js?

	Georg Fritzsche [:gfritzsche]
	Comment 3 • 10 years ago

(In reply to Ed Lee :Mardak from comment #2)
> gfritzsche, are you saying the browser/base/content/browser.js probe covers
> all these items #2-5?

I'm pretty sure about 4. and 5., not about 2. and 3. (might want to check if NEWTAB_PAGE_SHOWN handles them for comparison).

> Any idea why the Page_init code doesn't cover those cases? Or ideas on a
> good way to reuse the telemetry probes in Page_init instead of copying it
> over to browser.js?

No great way right now from talking about this with ttaubert, bug 971171 has the context.
Bug 972341 will allow us to just do changes in page.js like bug 973532 wants to.

	Ed Lee :Mardak
	Comment 4 • 10 years ago

This could be fixed by bug 973532 when it refactors the probes into Page_pageShownTelemetry of attachment 8375574 [details] [diff] [review].

	Georg Fritzsche [:gfritzsche]
	Comment 5 • 10 years ago

(In reply to Ed Lee :Mardak from comment #4)
> This could be fixed by bug 973532 when it refactors the probes into
> Page_pageShownTelemetry of attachment 8375574 [details] [diff] [review].

If you want to use the metrics before bug 973532 lands though (not clear when that will happen and if it makes 31) you should fix bug 972936 to match the NEWTAB_PAGE_SHOWN implementation.

	Ed Lee :Mardak
	Comment 6 • 10 years ago

clarkbw, see comment 0 for various cases that are not being counted correctly. Not sure if we need to fix things now or just wait for bug 973532 to get fixed (which depends on bug 972341).

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 7 • 10 years ago

bug 972341 seems to be stalled a bit, maybe we can get an update on the progress for that? The situations described aren't too worrisome for me but we want to be as accurate as possible.

	Ed Lee :Mardak
	Comment 8 • 10 years ago

ttaubert, do you think we should try to get bug 972341 prioritized? Or should we just refactor the telemetry impressions code to NewTabUtils.Telemetry.countImpression.. although DIRECTORY_*_SHOWN counting is only relevant to browser code.

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

Submitted a new bunch of patches to bug 972341, I hope we can get this resolved soon.

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

Patch moved to bug 973532.