1196104 - Heartbeat: Support heartbeat prompts that target private windows

Status: RESOLVED FIXED

(Firefox :: Tours, enhancement, P1)

Description

[Matthew N. [:MattN]]

Excerpts from bug 1184338:
* PBM users see an opt-in survey. They have divulged no signal to anyone, including Mozilla, that they are in PBM unless they opt-in. 
* Survey prompt offers a link. Clicking the link from PBM launches a survey tailored for PBM or recorded as actual PBM user.
* If a user has both normal and PBM windows open, the tailored survey prompt appears in PBM.
* Heartbeat must respect PBM and store nothing that would indicate what user did in PBM or that they even opened PBM.
* We may need to do work to make Heartbeat less visible. For example, we need to evaluate whether HB fires event callbacks on things like Survey prompt dismissal ("X")

We can add a private window option to the API:
* which shows the heartbeat UI in the most recent private window (if one exists)
* when the learn more link is clicked, the URL should open in the same private window

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

Bug 1196104 - Heartbeat: Support heartbeat prompts that target private windows. r=bgrins

Comment 2

[Matthew N. [:MattN]]

Code from a whitelisted domain can run: `
Mozilla.UITour.showHeartbeat("Would you like to share feedback with Mozilla? No information will be shared unless you take the survey.",
"Thank you!",
"myflowid",
"http://www.mozilla.org/?fake=bar", null, null, {
  engagementButtonLabel: "Take Survey",
  privateWindowsOnly: true,
});
`

From a Browser Console the equivalent is: `
UITour.showHeartbeat(window, {
  message: "Would you like to share feedback with Mozilla? No information will be shared unless you take the survey.",
  thankyouMessage: "Thank you!",
  flowId: "myflowid",
  engagementButtonLabel: "Take Survey",
  engagementURL: "http://www.mozilla.org/",
  privateWindowsOnly: true,
});

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

https://reviewboard.mozilla.org/r/17043/#review15277

::: browser/components/uitour/UITour.jsm:403
(Diff revision 1)
>      let tab = window.gBrowser.getTabForBrowser(browser);

May as well remove the unused `tab` variable here (unless if that's going to cause problems with an uplift, in that case maybe push that as a separate commit)

::: browser/components/uitour/test/browser_UITour_heartbeat.js:386
(Diff revision 1)
> +    let privateWin = yield BrowserTestUtils.openNewBrowserWindow({ private: true });

This test doesn't cover the case where there is a private window as the non-active window (hitting the RecentWindow.getMostRecentBrowserWindow line).  Maybe there should be a simple test similar to test_privateWindowsOnly_noneOpen that just makes sure that it opens in the correct window for that case.  Or a normal browser window could be opened after privateWin in this test

::: browser/components/uitour/test/browser_UITour_heartbeat.js:394
(Diff revision 1)
> +    yield new Promise((resolve) => {

Nit: this function is run as a task and yielding on a promise but test_privateWindowsOnly_noneOpen is using a callback only.  I think the tests would be easier to read if you extracted this pattern into a function that returns a Promise like this:

  function onPageEvent() {
    return new Promise(resolve => {
      Services.mm.addMessageListener("UITour:onPageEvent", function onPageEvent(aMessage) {
        Services.mm.removeMessageListener("UITour:onPageEvent", onPageEvent);
        SimpleTest.executeSoon(resolve);
      });
    });
  }

Then you could use `yield onPageEvent()` in all 3 cases (or onPageEvent().then() if a certain test shouldn't have taskify).

::: browser/components/uitour/test/browser_UITour_heartbeat.js:397
(Diff revision 1)
> +        SimpleTest.executeSoon(resolve);

Should this be asserting isTourBrowser(gBrowser.selectedBrowser)?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

https://reviewboard.mozilla.org/r/17043/#review15307

Ship It!

Comment 6

[Pulsebot]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent OSX browser_UITour_heartbeat.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=4392763&repo=fx-team

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

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b673659a3cc

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

Approval Request Comment
[Feature/regressing bug #]: Private Browsing with Tracking Protection measurements via HeartBeat (using UITour)
[User impact if declined]: No way to measure the (change in) user sentiment towards private browsing so we won't be able to address user concerns. Specifically this patch allows heartbeat to target users who have at least one private window open and showing the heartbeat UI in the most recent private window. We want to get a baseline in 41 to measure 42 sentiment against.
[Describe test coverage new/current, TreeHerder]: Automated tests for the button behaviour.
[Risks and why]: Low risk since this is an isolated change only affecting UITour for the heartbeat API. Worst case is that the heartbeat-only UI doesn't work which wouldn't be end of the world.
[String/UUID change made/needed]: None

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

Same comment as in https://bugzilla.mozilla.org/show_bug.cgi?id=1196102#c12 about the beta uplift.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11599331105a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Test bustage follow-up:
https://hg.mozilla.org/releases/mozilla-aurora/rev/5920e490543e

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8652001 [details]
MozReview Request: Bug 1196104 - Heartbeat: Support prompts that target private windows. r=bgrins

We need this patch in Beta41 in order to get baseline numbers. I would have preferred to stabilize this patch on Aurora channel but the sooner this gets uplifted to Beta, we can get more users to test it and identify regressions. Beta41+.

It's good to see that we are adding automated tests.

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Florin, this fix and fixes from two other related bugs will be in 41.0b6. It will be awesome if the QE team can do some testing focused on this change. Thanks!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5cfe47998df

Comment 17

[Matthew N. [:MattN]]

Beta bustage fix due to missing PrivateBrowsingUtils import.

https://hg.mozilla.org/releases/mozilla-beta/rev/e1592dfde524

Comment 18

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

(In reply to Ritu Kothari (:ritu) from comment #15)
> Florin, this fix and fixes from two other related bugs will be in 41.0b6. It
> will be awesome if the QE team can do some testing focused on this change.
> Thanks!

I believe the fixes that you're talking about are for: bug 1196102, bug 1196104, and bug 1196105. All three are connected to bug 1184338, but all three are also marked qe-verify- and have automated tests. 

Matt/Ritu, can you please clarify what level of manual QA is needed for this? Should we treat bug 1184338 as a feature and test it as a whole... or, should we just test some specific scenarios not covered by automated tests (if yes, then we could use some details on scenarios to test for each of these fixes)?

Comment 19

[Petruta Horea [:phorea]]

Used Firefox 41 beta 6 and Dev Edition 42.0a2 2015-09-02 under Win 7 64-bit, Ubuntu 12.04 32-bit and Mac OS X 10.9.5 and executed the second part of comment 2 in the browser console. Please see my results:

1. The HB prompt appears on the active window (or the one the browser console was opened from) not in the private window

2. The prompt appears for as many times the code is executed, even if it's already open (I don't know if this will still be the case in real environment testing)

3. Under Windows and Ubuntu, the text is not bold and the button is square: http://i.imgur.com/qhsDAZy.png

Matt, please let us know if these needs to be filled. Thank you!

Comment 20

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

Does anyone have any feedback on questions from comment 18 and preliminary test results in comment 19? There isn't much more that QA can do here.