Factor login manager dialog display function from browser to toolkit

VERIFIED FIXED in Firefox 42

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: rittme, Assigned: rittme)

Tracking

Trunk
mozilla42
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox42 verified)

Details

(Whiteboard: pwmgr42, )

Attachments

(1 attachment)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Assignee: nobody → bernardo
Blocks: 433238
Status: NEW → ASSIGNED
Iteration: --- → 42.2 - Jul 27
Flags: qe-verify+
Assignee

Comment 1

4 years ago
Bug 1187529 - Factored viewPasswords function from security.js to loginHelper.js r?MattN
Attachment #8638835 - Flags: review?(MattN+bmo)
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 = "") {`
Attachment #8638835 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 4

4 years ago
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
Attachment #8638835 - Attachment description: MozReview Request: Bug 1187529 - Factored viewPasswords function from security.js to loginHelper.js r?MattN → MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r?MattN
Attachment #8638835 - Flags: review+ → review?(MattN+bmo)
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=""]
Attachment #8638835 - Flags: review?(MattN+bmo) → review+
Assignee

Comment 6

4 years ago
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
Attachment #8638835 - Attachment description: MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r?MattN → MozReview Request: Bug 1187529 - Factored viewPasswords function from pageinfo/security.js to LoginHelper.jsm. r=MattN
Attachment #8638835 - Flags: review+ → review?(MattN+bmo)
Assignee

Updated

4 years ago
Keywords: checkin-needed
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!
Attachment #8638835 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/c607be4ebbd8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Whiteboard: pwmgr42
Points: --- → 3
Flags: firefox-backlog+
QA Contact: kjozwiak
Is there anything I can do here Matt?
Flags: needinfo?(MattN+bmo)
Assignee

Comment 11

4 years ago
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.
What Bernardo said.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(MattN+bmo)
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.