Closed Bug 1200639 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Tours, defect, P1)

43 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox43 --- affected
firefox46 --- fixed

People

(Reporter: verdi, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [Onboarding])

Attachments

(2 files)

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.
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.
Whiteboard: [fxgrowth] → [fxprivacy] [triage]
Whiteboard: [fxprivacy] [triage]
Priority: -- → P2
Whiteboard: [Onboarding]
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? :-)
Flags: needinfo?(mverdi)
(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.
Flags: needinfo?(mverdi)
Priority: P2 → P1
Taking this based on discussion with Verdi.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8709467 - Flags: review?(jaws)
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/
Attachment #8709467 - Flags: review?(jaws)
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 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?
Attachment #8709640 - Flags: review?(jaws) → review+
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 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 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/
Attachment #8709640 - Attachment description: MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r?jaws → MozReview Request: Bug 1200639 - part 0: fix eslint issues in nsBrowserContentHandler.js, r=jaws
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 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.
Attachment #8709640 - Flags: review+ → review?(jaws)
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
Attachment #8709640 - Flags: review?(jaws) → review+
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"
(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".
https://hg.mozilla.org/mozilla-central/rev/5406de49fa30
https://hg.mozilla.org/mozilla-central/rev/3b2ed8b2afdc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
You need to log in before you can comment on or make changes to this bug.