1465685 - Add Password Manager and login form event telemetry for Savant Shield study

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Bianca Danforth [:bdanforth]]

This is one of a series of bugs that together comprise the Savant Shield study (tracking bug 1457226).

This bug will register and record (for the duration of the study only):
* When the user is prompted by the Password Manager
* When the user saves their login information through the Password Manager prompt
* When the user updates their login information through the Password Manager prompt
* When the user uses login information stored by the Password Manager
* When a login form is loaded
* When a login form is submitted (excluding the case from unresolved bug 1287202)

Comment 1

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add Password Manager probes for Savant Shield study;

Add 'pw_manager_use' and 'pw_manager'  probes. Note that 'pw_manager_use' takes place in the content process, whereas 'pw_manager' takes place in the parent process.

The 'pw_manager' probe has an 'extra' field called 'flow_id' as defined in the probe spec (see PHD linked in bug 1457226). This value associates actions that occur in series (i.e. ask --> save or ask --> update in this case).

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

Comment 2

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add login_form probe for Savant Shield study;

The login_form probe has an 'extra' field called 'flow_id' as defined in the probe spec (see PHD linked in bug 1457226). This value associates actions that occur in series (i.e. submit --> save). Though it should be noted that for several reasons, we should expect a higher than 1:1 ratio between 'load' and 'submit' events:
* Some sites, like Google and Amazon, have a two-step login process, and each step fires its own 'load' event. Only the second step fires a 'submit' event.
* Some sites, like Facebook and Twitter, fire multiple 'load' events on the same page.
* A common pattern for unsuccessful logins is for the site to redirect to a page with a login form. This would be a 'load' --> 'submit' --> 'load' series.
* While the 'load' event fires whether or not the Password Manager is enabled and in both private and non-private windows, the 'submit' event fires only when the Password Manager is enabled and the user is in a non-private window. This means for events that occur in a private window and/or when Password Manager is disabled, we would only capture the 'load' event.

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

Comment 3

[Bianca Danforth [:bdanforth]]

Comment on attachment 8982779 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

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

Comment 4

[Bianca Danforth [:bdanforth]]

Comment on attachment 8982780 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8982779 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/248670/#review254906


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:19
(Diff revision 2)
>        Components.Constructor("@mozilla.org/login-manager/loginInfo;1",
>                               "nsILoginInfo", "init");
>  
>  const BRAND_BUNDLE = "chrome://branding/locale/brand.properties";
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SavantShieldStudy",

Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:1728
(Diff revision 2)
>    let logger = LoginHelper.createLogger("LoginManagerPrompter");
>    return logger.log.bind(logger);
>  });
>  
>  var component = [LoginManagerPromptFactory, LoginManagerPrompter];
>  this.NSGetFactory = XPCOMUtils.generateNSGetFactory(component);

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8982780 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/248672/#review254908


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/passwordmgr/LoginManagerContent.jsm:1645
(Diff revision 2)
>  
>      LoginManagerContent._formLikeByRootElement.set(formLike.rootElement, formLike);
>  
>      return formLike;
>    },
>  };

Error: Newline required at end of file but not found. [eslint: eol-last]

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:23
(Diff revision 2)
>  XPCOMUtils.defineLazyGetter(this, "log", () => {
>    let logger = LoginHelper.createLogger("LoginManagerParent");
>    return logger.log.bind(logger);
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SavantShieldStudy",

Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:507
(Diff revision 2)
>    });
>    return this._recipeManager.initializationPromise;
>  });
>  
>  XPCOMUtils.defineLazyPreferenceGetter(LoginManagerParent, "_repromptTimeout",
>    "signon.masterPasswordReprompt.timeout_ms", 900000); // 15 Minutes

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8982779 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

Waiting for a new revision

Comment 8

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add Password Manager probes for Savant Shield study;

These probes will register and record (for the duration of the study only):
* When the user is prompted by the Password Manager
* When the user saves their login information through the Password Manager prompt
* When the user updates their login information through the Password Manager prompt
* When the user uses login information stored by the Password Manager

Both the 'pw_manager' and 'pw_manager_use' probe have an 'extra' field called 'flow_id'. This value associates actions that occur in the same tab.

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

Comment 9

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add login_form probe for Savant Shield study;

This probe will register and record (for the duration of the study only):
* When a login form is loaded
* When a login form is submitted (excluding the case from unresolved bug 1287202)

The login_form probe has an 'extra' field called 'flow_id'. This value associates actions that occur in the same tab.

It should be noted that for several reasons, we should expect a higher than 1:1 ratio between 'load' and 'submit' events:
* Some sites, like Google and Amazon, have a two-step login process, and each step fires its own 'load' event. Only the second step fires a 'submit' event.
* Some sites, like Facebook and Twitter, fire multiple 'load' events on the same page.
* A common pattern for unsuccessful logins is for the site to redirect to a page with a login form. This would be a 'load' --> 'submit' --> 'load' series.
* While the 'load' event fires whether or not the Password Manager is enabled and in both private and non-private windows, the 'submit' event fires only when the Password Manager is enabled and the user is in a non-private window. This means for events that occur in a private window and/or when Password Manager is disabled, we would only capture the 'load' event.

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

Comment 10

[Bianca Danforth [:bdanforth]]

:pdehaan, could you provide QA review on this patch? Instructions for obtaining the test build are here: https://github.com/biancadanforth/gecko-dev/issues/5

Comment 11

[Bianca Danforth [:bdanforth]]

I neglected to mention that for the event telemetry to show up in `about:telemetry#events-tab`, the study pref `shield.savant.enabled` needs to be set to `true`. Normandy will take care of turning this pref ON and OFF when the study is officially launched.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8983709 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249532/#review256042

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:116
(Diff revision 1)
>          AutoCompletePopup.removeLogin(login);
>          break;
>        }
> +
> +      case "LoginStats:LoginFillSuccessful": {
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;

This assumes that the fill is happening in the selected tab but that's not necessarily the case for a page load in a background tab with autofill. I believe you need to use `msg.target..selectedTab.linkedPanel` as msg.target should be the <browser> IIRC.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:820
(Diff revision 1)
>      let histogramName = type == "password-save" ? "PWMGR_PROMPT_REMEMBER_ACTION"
>                                                  : "PWMGR_PROMPT_UPDATE_ACTION";
>      let histogram = Services.telemetry.getHistogramById(histogramName);
>      histogram.add(PROMPT_DISPLAYED);
>  
> +    const tab_id = browser.ownerGlobal.gBrowser.selectedTab.linkedPanel;
> +    Services.telemetry.recordEvent("savant", "pw_manager", "ask", null,
> +                                  { subcategory: "prompt", flow_id: tab_id });

I'm surprised we don't want to separate the remember ask from the update ask like we do in the histogram. I would think we would want to in order to correlate them better.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:825
(Diff revision 1)
>      let histogramName = type == "password-save" ? "PWMGR_PROMPT_REMEMBER_ACTION"
>                                                  : "PWMGR_PROMPT_UPDATE_ACTION";
>      let histogram = Services.telemetry.getHistogramById(histogramName);
>      histogram.add(PROMPT_DISPLAYED);
>  
> +    const tab_id = browser.ownerGlobal.gBrowser.selectedTab.linkedPanel;

IIRC, doorhangers have their own unique ID which would be more useful and to directly correctly the doorhanger showing with the action taken on that doorhanger but then you would be able to use it with the "pw_manager_use" so this is fine for now.

Note that this will re-use the same ID if the user uses the password manager for multiple sites in the same tab.

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:825
(Diff revision 1)
>      let histogramName = type == "password-save" ? "PWMGR_PROMPT_REMEMBER_ACTION"
>                                                  : "PWMGR_PROMPT_UPDATE_ACTION";
>      let histogram = Services.telemetry.getHistogramById(histogramName);
>      histogram.add(PROMPT_DISPLAYED);
>  
> +    const tab_id = browser.ownerGlobal.gBrowser.selectedTab.linkedPanel;

Again you shouldn't assume gBrowser.selected\* is correct, especially when you already have \`browser\` available:
```js
browser.ownerGlobal.gBrowser.getTabForBrowser(browser).linkedPanel
```

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8983710 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249534/#review256048

::: commit-message-92a21:2
(Diff revision 1)
> +Bug 1465685 - Add login_form probe for Savant Shield study; r=MattN
> +

As a high-level comment that I already conveyed to you live, I question whether the subcategories are useful.

::: commit-message-92a21:9
(Diff revision 1)
> +* When a login form is loaded
> +* When a login form is submitted (excluding the case from unresolved bug 1287202)
> +
> +The login_form probe has an 'extra' field called 'flow_id'. This value associates actions that occur in the same tab.
> +
> +It should be noted that for several reasons, we should expect a higher than 1:1 ratio between 'load' and 'submit' events:

Note that I've also seen multiple submit events for the same login page due to bugs which haven't been prioritized to fix.

::: commit-message-92a21:10
(Diff revision 1)
> +* Some sites, like Google and Amazon, have a two-step login process, and each step fires its own 'load' event. Only the second step fires a 'submit' event.
> +* Some sites, like Facebook and Twitter, fire multiple 'load' events on the same page.

It seems like it's going to be hard to get useful correlations between the two events given these issues.

::: commit-message-92a21:13
(Diff revision 1)
> +
> +It should be noted that for several reasons, we should expect a higher than 1:1 ratio between 'load' and 'submit' events:
> +* Some sites, like Google and Amazon, have a two-step login process, and each step fires its own 'load' event. Only the second step fires a 'submit' event.
> +* Some sites, like Facebook and Twitter, fire multiple 'load' events on the same page.
> +* A common pattern for unsuccessful logins is for the site to redirect to a page with a login form. This would be a 'load' --> 'submit' --> 'load' series.
> +* While the 'load' event fires whether or not the Password Manager is enabled and in both private and non-private windows, the 'submit' event fires only when the Password Manager is enabled and the user is in a non-private window. This means for events that occur in a private window and/or when Password Manager is disabled, we would only capture the 'load' event.

Isn't this going to really mess up the data? It seems like we should either not record the 'load' in these cases or record with the 'load' event that we're in these cases. For PWMGR_FORM_AUTOFILL_RESULT we include the reasons for not filling so there is a 1:1 mapping between a load and an accumulation in that probe.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:97
(Diff revision 1)
>                            data.newPasswordField,
>                            data.oldPasswordField,
>                            msg.objects.openerTopWindow,
>                            msg.target);
> +
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;

Same issue about selectedTab as the previous commit.

Comment 14

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983709 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249532/#review256042

> This assumes that the fill is happening in the selected tab but that's not necessarily the case for a page load in a background tab with autofill. I believe you need to use `msg.target..selectedTab.linkedPanel` as msg.target should be the <browser> IIRC.

Yes, you're right: `msg.target` is the <browser> element. Though `msg.target.selectedTab` was undefined. I did find this: `msg.target.ownerGlobal.gBrowser.getTabForBrowser(msg.target)._tPos`, which returns the tab index.

> I'm surprised we don't want to separate the remember ask from the update ask like we do in the histogram. I would think we would want to in order to correlate them better.

That's a good idea. I have updated it.

> Again you shouldn't assume gBrowser.selected\* is correct, especially when you already have \`browser\` available:
> ```js
> browser.ownerGlobal.gBrowser.getTabForBrowser(browser).linkedPanel
> ```

Yes actually I am using `browser.ownerGlobal.gBrowser.getTabForBrowser(browser)._tPos` which looks to be the actual tab index rather than a document fragment identifier like `linkedPanel`.

Comment 15

[Bianca Danforth [:bdanforth]]

Comment on attachment 8983710 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249534/#review256048

> As a high-level comment that I already conveyed to you live, I question whether the subcategories are useful.

These categories were intended to be the event category used to register the events in Events.yaml. For reasons I can explain if you're curious, we ended up using a single "savant" event category and subordinated the original event categories.

> Note that I've also seen multiple submit events for the same login page due to bugs which haven't been prioritized to fix.

Ah, good to know; do you know what bug number(s)?

> It seems like it's going to be hard to get useful correlations between the two events given these issues.

Yes it might be tricky, although we do also have the benefit of a timestamp and some other event information: if for example, 3 login form load events take place very close together in time in the same tab, we may be able to infer that these were generated by the same site. I believe Su, our analyst, can create some filter expressions to remove some of this noise.

> Isn't this going to really mess up the data? It seems like we should either not record the 'load' in these cases or record with the 'load' event that we're in these cases. For PWMGR_FORM_AUTOFILL_RESULT we include the reasons for not filling so there is a 1:1 mapping between a load and an accumulation in that probe.

Hm... yes. Though as you mention there are lots of other reasons things are not going to be 1:1. I just tried to log in twice at Twitter.com, and I saw about a dozen login related events. I have added a boolean `canRecordSubmit` that evaluates to true for non-private windows with Password Manager enabled.

How can I easily know whether or not Password Manager is enabled? That's the only part I still need to add. You can see in my latest patch it is currently just set to `true`.

Comment 16

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add Password Manager probes for Savant Shield study;

These probes will register and record (for the duration of the study only):
* When the user is prompted by the Password Manager
* When the user saves their login information through the Password Manager prompt
* When the user updates their login information through the Password Manager prompt
* When the user uses login information stored by the Password Manager

Both the 'pw_manager' and 'pw_manager_use' probe have an 'extra' field called 'flow_id'. This value corresponds to the tab index in the window.

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

Comment 17

[Bianca Danforth [:bdanforth]]

Attachment: Bug 1465685 - Add login_form probe for Savant Shield study;

This probe will register and record (for the duration of the study only):
* When a login form is loaded
* When a login form is submitted (excluding the case from unresolved bug 1287202)

The login_form probe has an 'extra' field called 'flow_id'. This value associates actions that occur in the same tab.

It should be noted that for several reasons, we should expect a higher than 1:1 ratio between 'load' and 'submit' events:
* Some sites, like Google and Amazon, have a two-step login process, and each step fires its own 'load' event. Only the second step fires a 'submit' event.
* Some sites, like Facebook and Twitter, fire multiple 'load' events on the same page.
* A common pattern for unsuccessful logins is for the site to redirect to a page with a login form. This would be a 'load' --> 'submit' --> 'load' series.
* Unlike 'load', the 'submit' event fires only when the Password Manager is enabled and the user is in a non-private window. 'Load' events will have a 'canRecordSubmit' key in the 'extra' field which will be true if a submit event for that form can be recorded.

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

Comment 18

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256160

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:132
(Diff revision 1)
> +      }
> +
> +      case "LoginStats:LoginEncountered": {
> +        const win = msg.target.ownerGlobal;
> +        const isPrivateWindow = PrivateBrowsingUtils.isWindowPrivate(win);
> +        const isPwMgrEnabled = true;

Here is where I need to check if Password Manager is enabled; is there an easy way to do this?

Comment 19

[Bianca Danforth [:bdanforth]]

Also, apologies I corrupted my local repo and lost the original references to the commits. Thankfully all your comments and my responses are above in the comments here. I created an issue for the concern you had about data for login form load and submit; I think I have a solution except I don't know how to determine if Password Manager is enabled.

Comment 20

[Matthew N. [:MattN]]

(In reply to Bianca Danforth [:bdanforth] from comment #19)
> Also, apologies I corrupted my local repo and lost the original references
> to the commits. Thankfully all your comments and my responses are above in
> the comments here. I created an issue for the concern you had about data for
> login form load and submit; I think I have a solution except I don't know
> how to determine if Password Manager is enabled.

For the future you could have pulled them back in using the command on mozreview. Either way it should have still worked to push them for review if you used the correct command to push.

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256160

> Here is where I need to check if Password Manager is enabled; is there an easy way to do this?

You should send the information (private and enabled) along with the message from content instead. They are both easily accessible there.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249930/#review256358

::: commit-message-cec4a:9
(Diff revision 1)
> +* When the user is prompted by the Password Manager
> +* When the user saves their login information through the Password Manager prompt
> +* When the user updates their login information through the Password Manager prompt
> +* When the user uses login information stored by the Password Manager
> +
> +Both the 'pw_manager' and 'pw_manager_use' probe have an 'extra' field called 'flow_id'. This value corresponds to the tab index in the window.

As a nit: We generall use pwmgr and never pw_manager so it would be better to align with what we already do IMO

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:116
(Diff revision 1)
> +        const tab_id = msg.target.ownerGlobal.gBrowser
> +                      .getTabForBrowser(msg.target)._tPos.toString();

So you're changing the identifier now again? Why? This is even less useful since the IDs will get re-used when tabs open and close. You should also handle multiple windows like linkedPanel did:

https://dxr.mozilla.org/mozilla-central/rev/f7fd9b08c0156be5b5cd99de5ed0ed0b98d93051/browser/base/content/tabbrowser.js#4023

I would rather you go back to the linkedPanel

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256368

Comment 24

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249930/#review256358

> As a nit: We generall use pwmgr and never pw_manager so it would be better to align with what we already do IMO

Okay sure I can change it to pwmgr for consistency.

> So you're changing the identifier now again? Why? This is even less useful since the IDs will get re-used when tabs open and close. You should also handle multiple windows like linkedPanel did:
> 
> https://dxr.mozilla.org/mozilla-central/rev/f7fd9b08c0156be5b5cd99de5ed0ed0b98d93051/browser/base/content/tabbrowser.js#4023
> 
> I would rather you go back to the linkedPanel

Good points! cmore had suggested tabID, but you're right that we have less information with tabID. Will change it back.

Comment 25

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256160

> You should send the information (private and enabled) along with the message from content instead. They are both easily accessible there.

Okay good idea, that does make it easier. I will add that.

Comment 26

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

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

Comment 27

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

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

Comment 28

[Matthew N. [:MattN]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249930/#review256450

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:116
(Diff revision 2)
>          AutoCompletePopup.removeLogin(login);
>          break;
>        }
> +
> +      case "LoginStats:LoginFillSuccessful": {
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;

As I said before, do not use `gBrowser.selected*`, you need to use the linkedPanel for the target tab/browser. I gave you the code snippet at the bottom of comment 12 which can be adapted to this case like so:
```js
msg.target.ownerGlobal.gBrowser.getTabForBrowser(msg.target).linkedPanel
```

Comment 29

[Matthew N. [:MattN]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256452

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:445
(Diff revision 2)
> -    messageManager.sendAsyncMessage("LoginStats:LoginEncountered");
> +    messageManager.sendAsyncMessage("LoginStats:LoginEncountered",
> +                                    { isPrivateWindow, isPwmgrEnabled });

Nit: one property per line for object literals:
```js
messageManager.sendAsyncMessage("LoginStats:LoginEncountered", {
  isPrivateWindow,
  isPwmgrEnabled: gEnabled,
});
```
This helps to track changes to objects in blame.

I would then also remove the `isPwmgrEnabled` const since it won't be useful.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:97
(Diff revision 2)
>                            data.newPasswordField,
>                            data.oldPasswordField,
>                            msg.objects.openerTopWindow,
>                            msg.target);
> +
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;

Same issue as the other commit about `gBrowser.selected*`

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:127
(Diff revision 2)
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;
> +        Services.telemetry.recordEvent("savant", "login_form", "load", null,
> +                                      { subcategory: "encounter", flow_id: tab_id,

Nit: you could rename the `tab_id` variable to `flow_id` to use the shorthand syntax without the colon.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:129
(Diff revision 2)
> +
> +      case "LoginStats:LoginEncountered": {
> +        const canRecordSubmit = (!data.isPrivateWindow && data.isPwmgrEnabled).toString();
> +        const tab_id = msg.target.ownerGlobal.gBrowser.selectedTab.linkedPanel;
> +        Services.telemetry.recordEvent("savant", "login_form", "load", null,
> +                                      { subcategory: "encounter", flow_id: tab_id,

Really the object literal for every recordEvent call should have on property per line. I thought we already had an eslint rule enforcing this but I guess not everywhere/yet.

Comment 30

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

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

Comment 31

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

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

Comment 32

[Matthew N. [:MattN]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249930/#review256464

FYI I didn't do manual testing as QA will do that.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:120
(Diff revision 3)
> +      case "LoginStats:LoginFillSuccessful": {
> +        const flow_id = msg.target.ownerGlobal.gBrowser.getTabForBrowser(msg.target).linkedPanel;
> +        Services.telemetry.recordEvent("savant", "pwmgr_use", "use", null,
> +                                      {
> +                                        subcategory: "feature",
> +                                        flow_id

Nit: missing trailing comma

::: toolkit/components/passwordmgr/nsLoginManagerPrompter.js:830
(Diff revision 3)
> +    const promptType = type == "password-save" ? "save" : "update";
> +    const flow_id = browser.ownerGlobal.gBrowser.getTabForBrowser(browser).linkedPanel;
> +    Services.telemetry.recordEvent("savant", "pwmgr", "ask", promptType,
> +                                  {
> +                                    subcategory: "prompt",
> +                                    flow_id

Same here and elsewhere. eslint should catch this…

Comment 33

[Matthew N. [:MattN]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

https://reviewboard.mozilla.org/r/249932/#review256466

Thanks!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:447
(Diff revision 3)
> +
>      let messageManager = messageManagerFromWindow(window);
> -    messageManager.sendAsyncMessage("LoginStats:LoginEncountered");
> +    messageManager.sendAsyncMessage("LoginStats:LoginEncountered",
> +                                    {
> +                                      isPrivateWindow,
> +                                      isPwmgrEnabled: gEnabled

Missing trailing comma here and elsewhere

Comment 34

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

https://reviewboard.mozilla.org/r/249930/#review256464

> Nit: missing trailing comma

I ran `./mach eslint` and got 0 errors and 0 warnings both for the case without and with the trailing comma. Not exactly sure how it applies the rules or knows which rules to look from. I added the commas.

Comment 35

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

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

Comment 36

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984086 [details]
Bug 1465685 - Add login_form probe for Savant Shield study;

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

Comment 37

[Peter deHaan [:pdehaan]]

R+ from QA.

Ran through a bunch of flows and got the following (deduped) events in about:telemetry

| category | method     | object | Value  | extra
|----------|------------|--------|--------|-------
| savant   | login_form | load   | null   | {"subcategory": "encounter", "flow_id": "panel-3-4", "canRecordSubmit": "true"}
| savant   | login_form | submit | null   | {"subcategory": "encounter", "flow_id": "panel-3-4"}
| savant   | pwmgr      | ask    | save   | {"subcategory": "prompt", "flow_id": "panel-3-4"}
| savant   | pwmgr      | ask    | update | {"subcategory": "prompt", "flow_id": "panel-3-4"}
| savant   | pwmgr      | save   | null   | {"subcategory": "prompt", "flow_id": "panel-3-4"}
| savant   | pwmgr      | update | null   | {"subcategory": "prompt", "flow_id": "panel-3-4"}
| savant   | pwmgr_use  | use    | null   | {"subcategory": "feature", "flow_id": "panel-3-4"}

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8d6fa6ad403
Add Password Manager probes for Savant Shield study; r=MattN
https://hg.mozilla.org/integration/autoland/rev/8ca33f3d8339
Add login_form probe for Savant Shield study; r=MattN

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a8d6fa6ad403
https://hg.mozilla.org/mozilla-central/rev/8ca33f3d8339

Comment 40

[Bianca Danforth [:bdanforth]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

Approval Request Comment
[Feature/Bug causing the regression]: There is no regression. The tracking bug for this study is 1457226.
[User impact if declined]: None. This patch is part of a priority Shield study.
[Is this code covered by automated tests?]: There are no study-specific tests, but all code has been run on the try server for regression testing and has been approved by both a Firefox peer and QA (pdehaan).
[Has the fix been verified in Nightly?]: The study’s functionality has been verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: This request applies to all patches in this bug. In general, here is a comprehensive list of bugs whose patches need to be uplifted (in this order) to complete the study: 1462725, 1465698, 1465707, 1465703, 1465704, 1465697, 1465685, 1465694
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is an observational pref-flip study with no branches and no UI. It adds some event telemetry probes in various places and a study module to enable/disable collection. Each patch has been QA tested and R+ by pdehaan.
[String changes made/needed]: None.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984085 [details]
Bug 1465685 - Add Password Manager probes for Savant Shield study;

Telemetry code needed to support the Fx61 Savant work. Approved for 61.0b13.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/41f2ad70c0ea
https://hg.mozilla.org/releases/mozilla-beta/rev/63275d610ea3