1187529 - Factor login manager dialog display function from browser to toolkit

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Bernardo Rittmeyer [:rittme]]

Since we want to show the login manager dialog from some password manager features, we could factor the code that displays it from the Page Info dialog and move it to the loginHelper module.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

Bug 1187529 - Factored viewPasswords function from security.js to loginHelper.js r?MattN

Comment 2

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cdd8d490f499

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8638835 [details]
MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

https://reviewboard.mozilla.org/r/14133/#review12733

::: toolkit/components/passwordmgr/LoginHelper.jsm:276
(Diff revision 1)
> +    }
> +    else {

} else {

::: browser/base/content/pageinfo/security.js:164
(Diff revision 1)
>    viewPasswords : function()
>    {

While you're here can you remove the new line since this file is already partly using that newer style anyways.

::: browser/base/content/pageinfo/security.js:166
(Diff revision 1)
> -    var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
> +    LoginHelper.viewPasswords(this._getSecurityInfo().hostName, window);

s/viewPasswords/openPasswordManager/

::: toolkit/components/passwordmgr/LoginHelper.jsm:265
(Diff revision 1)
> +   * @param {string} filterString
> +   *                 the filterString parameter to pass to the login manager dialog

Document window

::: toolkit/components/passwordmgr/LoginHelper.jsm:268
(Diff revision 1)
> +  viewPasswords : function(filterString, window)
> +  {

Don't put a newline here. The file you copied this from what using old style.

::: toolkit/components/passwordmgr/LoginHelper.jsm:270
(Diff revision 1)
> +    var wm = Cc["@mozilla.org/appshell/window-mediator;1"]
> +                       .getService(Components.interfaces.nsIWindowMediator);
> +    var win = wm.getMostRecentWindow("Toolkit:PasswordManager");

let win = Services.wm.getMostRecentWindow("Toolkit:PasswordManager");

::: browser/base/content/pageinfo/security.js:1
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

Nit In the commit message, replace 
"security.js to loginHelper.js"
with
pageinfo/security.js to LoginHelper.jsm"

::: toolkit/components/passwordmgr/LoginHelper.jsm:268
(Diff revision 1)
> +  viewPasswords : function(filterString, window)

Put window first since it's required, and make filterString default to "" and use new method syntax.

`openPasswordManager(window, filterString = "") {`

Comment 4

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8638835 [details]
MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r?MattN

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8638835 [details]
MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

https://reviewboard.mozilla.org/r/14133/#review12759

::: toolkit/components/passwordmgr/LoginHelper.jsm:263
(Diff revision 2)
> +   * Open the password manager window

Nit: add a period.

::: toolkit/components/passwordmgr/LoginHelper.jsm:268
(Diff revision 2)
> +   * @param {string} filterString
> +   *                 the filterString parameter to pass to the login manager dialog
> +   */
> +  openPasswordManager(window, filterString = "") {

The @param should be updated to reflect that this is now optional: [filterString=""]

Comment 6

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8638835 [details]
MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8638835 [details]
MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN

https://reviewboard.mozilla.org/r/14133/#review12763

Ship It!

Comment 8

[Pulsebot]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c607be4ebbd8

Comment 10

[Kamil Jozwiak [:kjozwiak]]

Is there anything I can do here Matt?

Comment 11

[Bernardo Rittmeyer [:rittme]]

Not really, I think I mistakenly flagged this as qe-verify+.
You can verify that opening the password manager from the Page Info dialog still works as expected, but this would probably be all.

Comment 12

[Matthew N. [:MattN]]

What Bernardo said.

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following m-c build:
- https://ftp.mozilla.org/pub/firefox/nightly/2015-08-10-03-02-05-mozilla-central/

OS's Used:

- Windows 10 x64 -> PASSED
- Windows 8.1 x64 (VM) -> PASSED
- OSX 10.10.4 x64 -> PASSED
- Ubuntu 14.04.2 x64 (VM) -> PASSED

- ensured the password manager opens when opening "Page Info" via "Right Click -> View Page Info"
- ensured the password manager opens when opening "Page Info" via the Control Centre
- ensured passwords are being listed correctly under the password manager