Closed Bug 973532 Opened 10 years ago Closed 10 years ago

Improve about:newtab impressions telemetry probe

Categories

(Firefox :: New Tab Page, enhancement)

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox32 --- verified

People

(Reporter: gfritzsche, Assigned: ttaubert)

References

Details

(Whiteboard: p=3 s=it-32c-31a-30b.3 [qa!])

Attachments

(2 files)

Once bug 972341 we can rewrite the about:newtab impressions telemetry probe as done in attachment 8375574 [details] [diff] [review] from bug 971171.
No longer depends on: 975570
Assignee: georg.fritzsche → nobody
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Blocks: 991729
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8425082 - Flags: review?(adw)
Marco, can you please add this to the current iteration? Thanks!
Flags: needinfo?(mmucci)
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+]
Version: unspecified → Trunk
Added to Iteration 32.2
Flags: needinfo?(mmucci)
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?
Attachment #8425082 - Flags: review?(adw) → review+
(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.
Component: Tabbed Browser → New Tab Page
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.
Attachment #8426123 - Flags: review?(adw)
Attachment #8426123 - Flags: review?(georg.fritzsche)
Keywords: leave-open
Attachment #8425082 - Flags: checkin+
gfritzsche is on PTO until June
Attachment #8426123 - Flags: review?(adw) → review+
Attachment #8426123 - Flags: review?(georg.fritzsche)
https://hg.mozilla.org/mozilla-central/rev/100c11abe20e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assigning to Kamil since this is related to Telemetry.
QA Contact: kamiljoz
Depends on: 1014784
Depends on: 1014789
Depends on: 1015177
Whiteboard: p=3 s=it-32c-31a-30b.2 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
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?
(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.
(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.
Status: RESOLVED → VERIFIED
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa!]
Thank you for verifying!
Depends on: 1046645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: