973532 - Improve about:newtab impressions telemetry probe

Status: VERIFIED FIXED

(Firefox :: New Tab Page, enhancement)

Description

[Georg Fritzsche [:gfritzsche]]

Once bug 972341 we can rewrite the about:newtab impressions telemetry probe as done in attachment 8375574 [details] [diff] [review] from bug 971171.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-973532-Let-the-preloader-s-docShells-be-inactive.patch

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Marco, can you please add this to the current iteration? Thanks!

Comment 3

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8425082 [details] [diff] [review]
0001-Bug-973532-Let-the-preloader-s-docShells-be-inactive.patch

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

Nice.

Either here or in a new bug, we should make a small search-related change.  We should either move gSearch.setUpInitialState's body to gSearch.init and call gSearch.init from gPage.onPageVisible, or we can still have gSearch.init and setUpInitialState, but init would not call setUpInitialState, and gPage would call init and setUpInitialState as this patch does.

The reason is that currently, allowBackgroundCaptures isn't properly set on pages that bypass the preloader, so as a hedge, gSearch.init requests the initial search state, but so does setUpInitialState, which gPage calls from its mutation observer.  For pages in the preloader, gSearch.init's request does nothing since content.js isn't loaded.  Now that we're correctly checking the page's visibility, gSearch can request its initial state only once, when the page truly becomes visible.

I can file a new bug if you don't want this to block this one.

::: browser/base/content/newtab/page.js
@@ +198,4 @@
>      }
> +  },
> +
> +  onPageVisible: function () {

Nit: onPageFirstVisible?

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Drew Willcoxon :adw from comment #4)
> Either here or in a new bug, we should make a small search-related change. 
> We should either move gSearch.setUpInitialState's body to gSearch.init and
> call gSearch.init from gPage.onPageVisible, or we can still have
> gSearch.init and setUpInitialState, but init would not call
> setUpInitialState, and gPage would call init and setUpInitialState as this
> patch does.

I went with the second suggestion as gGrid calls gSearch.setWidth() which fails if gSearch.init() hasn't been called before.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/c3b86a5d52b6

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-973532-Remove-legacy-NEWTAB_PAGE_SHOWN-probe-for.patch

I almost forgot that now that we record NEWTAB_PAGE_SHOWN always when the newtab page is shown, we can remove the special code we added for when about:newtab is the homepage. Otherwise we currently that twice.

Comment 8

[Benjamin Smedberg]

gfritzsche is on PTO until June

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/100c11abe20e

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/100c11abe20e

Comment 12

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

Assigning to Kamil since this is related to Telemetry.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-05-27-03-02-02-mozilla-central/

Went through all of the cases listed below and ensured that they are all being counted under about:telemetry:
* NEWTAB_PAGE_DIRECTORY_AFFILIATE_SHOWN
* NEWTAB_PAGE_DIRECTORY_ORGANIC_SHOWN
* NEWTAB_PAGE_DIRECTORY_SPONSORED_SHOWN
* NEWTAB_PAGE_SHOWN

- loading about:newtab from the location bar
- loading about:newtab via the "CTRL + T" shortcut
- loading about:newtab via "+" next to the tabs
- loading about:newtab from the menu bar
- setting about:newtab as the home page and opening fx
- while about:newtab is set as the home page, select the "Home" button
- Opening a new window while about:newtab is set as the home page
- Opening a new window (e10s) while about:newtab is set as the home page
- loading about:newtab via bookmark
- Ensured when "browser.newtabpage.enabled;false", about:newtab is not being counted

Possible issues found:

- set fx to restore from last session, open two about:newtab and close/re-open fx (about:newtab not being counted)
- open a few about:newtab and close/re-open fx and select "Restore Previous Session" (about:newtab not being counted)
- about:newtab is still being counted under "Private Browsing", not 100% sure if telemetry data should be logged while the user is in a private session

Before completing verification, please let me know if the above three issues are already known or do they warrant a separate bug?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #13)
> - set fx to restore from last session, open two about:newtab and
> close/re-open fx (about:newtab not being counted)
> - open a few about:newtab and close/re-open fx and select "Restore Previous
> Session" (about:newtab not being counted)

I assume that you didn't actually click-to-select to non-selected tabs after restarting? We restore tabs lazily which means they are loaded when selected.

> - about:newtab is still being counted under "Private Browsing", not 100%
> sure if telemetry data should be logged while the user is in a private
> session

I don't see a problem with this one if you navigate to about:newtab directly. We don't reveal anything interesting.

Comment 15

[Kamil Jozwiak [:kjozwiak]]

(In reply to Tim Taubert [:ttaubert] from comment #14)
> I assume that you didn't actually click-to-select to non-selected tabs after
> restarting? We restore tabs lazily which means they are loaded when selected.

Yup, that's correct. Went through the following:
- selected "Restore Previous Session" and clicked on each about:newtab, ensured they're being counted
- selected "Show windows from last time" and clicked on each about:newtab, ensured they're being counted
- ensured that restored about:newtab(s) are counted once when clicked (going back and forth shouldn't count as multiple visits)
 
> I don't see a problem with this one if you navigate to about:newtab
> directly. We don't reveal anything interesting.

Agreed. Wasn't 100% certain so asked just in case :) Thanks Tim, much appreciated.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Thank you for verifying!