1200639 - Open a page on a second or later run and/or after some amount of time

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P1)

Description

[Verdi [:verdi] Best to slack me]

We need the ability to continue onboarding past the first run after installation. We should be able to open another page for people depending on a combination of two factors: session number and time since install. 

So, for example, we could specify that a default browser promotion is shown on the third session as long as that occurs at least 24 hours after installation. If the third session happens in the first 24 hours then the promotion isn't triggered until the first session that happens later than 24 hours after installation.

Comment 1

[Verdi [:verdi] Best to slack me]

Here's a specific use case I'd like us to try. Right now we open https://www.mozilla.org/en-US/firefox/windows-10/welcome/ for all users the first time they open Firefox after having updated to Windows 10. Let's keep that behavior for existing users but change it for new installations since 100% of them will be confronted with the default browser modal prompt and a Firefox account sign up page at this same time.

For new installations, let's open https://www.mozilla.org/en-US/firefox/windows-10/welcome/ on their third Firefox session unless less than 24 hours have elapsed since installation. In that case we should open the page on the first session that happens after more than 24 hours have elapsed since installation.

It would be great if this can be implemented in a way that allows us to replace https://www.mozilla.org/en-US/firefox/windows-10/welcome/ with another url (https://www.mozilla.org/en-US/firefox/android/ for example) or change the session and time parameters (for example 4 sessions or 48 hours) in the future.

Comment 2

[:Gijs (he/him)]

I've been asked to work on this. Verdi, is comment #1 still an accurate summary of what we want to do here? If not, want to schedule a moment to talk about this before I rush in and implement something that wasn't quite what you intended? :-)

Comment 3

[Verdi [:verdi] Best to slack me]

(In reply to :Gijs Kruitbosch from comment #2)
> I've been asked to work on this. Verdi, is comment #1 still an accurate
> summary of what we want to do here? If not, want to schedule a moment to
> talk about this before I rush in and implement something that wasn't quite
> what you intended? :-)

I think a meeting would be a good idea. I'll try to schedule something and then we can report back here.

Comment 4

[:Gijs (he/him)]

Taking this based on discussion with Verdi.

Comment 5

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

Review commit: https://reviewboard.mozilla.org/r/31447/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31447/

Comment 6

[:Gijs (he/him)]

Comment on attachment 8709467 [details]
MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

Annnnd I'm realizing this doesn't match comment #0.

Comment 7

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws

Review commit: https://reviewboard.mozilla.org/r/31495/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31495/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8709467 [details]
MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31447/diff/1-2/

Comment 9

[:Gijs (he/him)]

So an example usage would look like:

pref("browser.laterrun.pages.test.url", "http://www.mozilla.org/");
pref("browser.laterrun.pages.test.minTime", 24);
pref("browser.laterrun.pages.test.minSessionCount", 3);
pref("browser.laterrun.pages.test.requireBoth", true);


Which would open mozilla.org once *both* at least 24 hours had elapsed, or 3 sessions had been started.

The "test" thing is just a unique identifier, you could do multiple pages by using multiple identifiers. Only one page will be opened each run (though they could potentially be combined with other pages due to updates etc.).

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8709640 [details]
MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws

https://reviewboard.mozilla.org/r/31495/#review28349

::: browser/components/nsBrowserContentHandler.js:455
(Diff revision 1)
> -      var uri = ios.newFileURI(file);
> +      var fileURI = ios.newFileURI(file);
>        openWindow(null, this.chromeURL, "_blank", 

Can you remove this trailing whitespace while you're here?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8709467 [details]
MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

https://reviewboard.mozilla.org/r/31447/#review28159

::: browser/modules/LaterRun.jsm:22
(Diff revision 1)
> +// Number of minutes we've been up (in total)
> +const kSessionLengthPref = "browser.laterrun.bookkeeping.sessionTime";

Please include unit (minutes) in variable and pref name.

::: browser/modules/LaterRun.jsm:28
(Diff revision 1)
> +const kSelfDestructTimeLimit = 10000;

kSelfDestructTimeLimitMinutes

::: browser/modules/LaterRun.jsm:30
(Diff revision 1)
> +const kSessionGranularity = 5 * 60 * 1000; // 5 minutes in ms

kSessionGranularityMS (or change unit to minutes to coincide with kSelfDestructTimeLimit)

::: browser/modules/LaterRun.jsm:31
(Diff revision 2)
> +  constructor({pref, minTime, minSessionCount, requireBoth, url}) {
> +    this.pref = pref;
> +    this.minTime = minTime || 0;

The name minTime is a bit confusing to me. Maybe it should be renamed to minHours? At that, we should just expand the name to be minimumHours and minimumSessionCount.

::: browser/modules/LaterRun.jsm:138
(Diff revision 2)
> +  // Return a URL for display as a 'later run' page if its criteria are matched,
> +  // or null otherwise.
> +  // NB: will only return one page at a time; if multiple pages match, it's up
> +  // to the preference service which one gets shown first, and the next one
> +  // will be shown next startup instead.
> +  getURL() {

We should require that the URL be over HTTPS. There is no check currently for that.

::: browser/modules/test/xpcshell/test_LaterRun.js:19
(Diff revision 2)
> +  pages = pages.filter(page => page.pref == kPagePrefRoot + "test_LaterRun_unittest.");
> +  Assert.equal(pages.length, 1, "Got 1 page");

Can you add a comment that this is necessary in case Firefox ships with some of these prefs set?

::: browser/modules/test/xpcshell/test_LaterRun.js:60
(Diff revision 2)
> +  Assert.ok(!page.applies({hoursSinceInstall: 1, sessionCount: 3}),
> +            "Does not apply when page has already run.");
> +  Assert.ok(!page.applies({hoursSinceInstall: 1, sessionCount: 4}),
> +            "Does not apply when page has already run.");
> +  Assert.ok(!page.applies({hoursSinceInstall: 10, sessionCount: 2}),
> +            "Does not apply when page has already run.");
> +  Assert.ok(!page.applies({hoursSinceInstall: 20, sessionCount: 2}),
> +            "Does not apply when page has already run.");
> +  Assert.ok(!page.applies({hoursSinceInstall: 10, sessionCount: 3}),
> +            "Does not apply when page has already run.");
> +  Assert.ok(!page.applies({hoursSinceInstall: 1, sessionCount: 1}),
> +            "Does not apply when page has already run.");

The assertion comment here should be unique per Assert, or else when one fails it may not be obvious which one failed since Assert.ok will not show the arguments to page.applies. At the least, these could be "#1 Does not apply ...", "#2 Does not apply ...", etc.

::: browser/modules/test/xpcshell/test_LaterRun.js:81
(Diff revision 2)
> +  let pages = LaterRun.readPages();
> +  pages = pages.filter(page => page.pref == kPagePrefRoot + "test_LaterRun_unittest.");
> +  Assert.equal(pages.length, 1, "Should only be 1 matching page");

Ditto on the pages.filter necessity question from above.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8709467 [details]
MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

https://reviewboard.mozilla.org/r/31447/#review28375

Sorry, forgot to click "Ship It" earlier.

Comment 13

[:Gijs (he/him)]

Comment on attachment 8709640 [details]
MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31495/diff/1-2/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8709467 [details]
MozReview Request: Bug 1200639 - add LaterRun.jsm to show pages on the Nth run of a new profile, r?jaws

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31447/diff/2-3/

Comment 15

[:Gijs (he/him)]

Comment on attachment 8709640 [details]
MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws

Note that at least some of the review comments were on an old version of the diff, so I'm renewing the r? to make sure you're OK'ing the current incarnation. I believe I've addressed all the still-applicable feedback, and I added a test for requiring secure URLs.

Comment 16

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8709640 [details]
MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws

https://reviewboard.mozilla.org/r/31495/#review28561

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/31447/#review28563

::: browser/modules/LaterRun.jsm:133
(Diff revisions 2 - 3)
> +      if (pageData.url && pageData.url.startsWith("https")) {

Maybe this is absurd but I think we should either parse the URL and make sure that the scheme is https or check that the string starts with https:// because it looks like this would accept "httpsmalicioussite.com"

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5406de49fa30
https://hg.mozilla.org/integration/fx-team/rev/3b2ed8b2afdc

Comment 19

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> https://reviewboard.mozilla.org/r/31447/#review28563
> 
> ::: browser/modules/LaterRun.jsm:133
> (Diff revisions 2 - 3)
> > +      if (pageData.url && pageData.url.startsWith("https")) {
> 
> Maybe this is absurd but I think we should either parse the URL and make
> sure that the scheme is https or check that the string starts with https://
> because it looks like this would accept "httpsmalicioussite.com"

Not absurd. Fixed in the latest incantation, with some Cu.reportError() magic to alert people dev'ing this why it's "not working".

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5406de49fa30
https://hg.mozilla.org/mozilla-central/rev/3b2ed8b2afdc

Comment 21

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b4
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing.

Actual Results: 
As expected