1286718 - Remove the Login Fill Doorhanger code/UI

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Matthew N. [:MattN]]

In dependencies of bug 1129528 a doorhanger was implemented to fill in saved passwords. My understanding is that UX doesn't want to move forward with this and some parts of it may be integrated into the control center panel instead.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1286718 - Remove password manager's browser_filldoorhanger.js.

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

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1286718 - Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm.

Manually revert since the context menu and insecure passwords UI are using some of this code now.

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

Comment 3

[Matthew N. [:MattN]]

Attachment: Bug 1286718 - Revert bug 1148026 to remove the initial login fill doorhanger code.

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

Comment 4

[Matthew N. [:MattN]]

Attachment: Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

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

Comment 5

[Matthew N. [:MattN]]

Paolo is away next week so I'm flagging dolske and johannh instead but only one of you is needed.

Removing this code:
* Gets rid of a lot of console spam from every pageshow event when signon.debug is true which made working on pwmgr annoying
* Removes the `loginFormForFill` concept which I think was a bad idea as it assumed we knew which of the many forms on a page we would want to fill and it was very naive (because it was only for experimental UI). Maintaining this has gotten in my way.

Comment 6

[Matthew N. [:MattN]]

Attachment: Bug 1286718 - Revert bug 1164028 to remove the additional login fill doorhanger code.

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

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8789973 [details]
Bug 1286718 - Remove password manager's browser_filldoorhanger.js.

https://reviewboard.mozilla.org/r/77992/#review76528

I can look at the code. We should probably quickly confirm with psackl that this idea is definitely abandoned before landing this.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8789977 [details]
Bug 1286718 - Revert bug 1164028 to remove the additional login fill doorhanger code.

https://reviewboard.mozilla.org/r/78000/#review76534

Comment 9

[Johann Hofmann [:johannh]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

https://reviewboard.mozilla.org/r/77998/#review76578

::: browser/base/content/browser.xul
(Diff revision 1)
>                    <image id="addons-notification-icon" class="notification-anchor-icon install-icon" role="button"
>                           tooltiptext="&urlbar.addonsNotificationAnchor.tooltip;"/>
>                    <image id="indexedDB-notification-icon" class="notification-anchor-icon indexedDB-icon" role="button"
>                           tooltiptext="&urlbar.indexedDBNotificationAnchor.tooltip;"/>
> -                  <image id="login-fill-notification-icon" class="notification-anchor-icon login-icon" role="button"
> -                         tooltiptext="&urlbar.loginFillNotificationAnchor.tooltip;"/>

Remove urlbar.loginFillNotificationAnchor.tooltip from browser.dtd as well.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:474
(Diff revision 1)
>    /**
> -   * Called to indicate whether a login form on the currently loaded page is
> -   * present or not. This is one of the factors used to control the visibility
> +   * Called to indicate whether an insecure password field is present so
> +   * insecure password UI can know when to show.
> -   * of the password fill doorhanger.
>     */
> -  updateLoginFormPresence(browser, { loginFormOrigin, loginFormPresent,
> +  updateLoginFormPresence(browser, { loginFormOrigin,

Remove loginFormOrigin here

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:475
(Diff revision 1)
> -   * Called to indicate whether a login form on the currently loaded page is
> -   * present or not. This is one of the factors used to control the visibility
> +   * Called to indicate whether an insecure password field is present so
> +   * insecure password UI can know when to show.
> -   * of the password fill doorhanger.
>     */
> -  updateLoginFormPresence(browser, { loginFormOrigin, loginFormPresent,
> +  updateLoginFormPresence(browser, { loginFormOrigin,
>                                       hasInsecureLoginForms }) {

I'm not sure if updateLoginFormPresence is still the correct name for this. We also don't need to pass around an object, a boolean that represents hasInsecureLoginForms should do it.

Following conventions, since we have a (kind of) getter above, we could call this setHasInsecureLoginForms(browser, hasInsecureLoginForms)

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8789974 [details]
Bug 1286718 - Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm.

https://reviewboard.mozilla.org/r/77994/#review76586

The changes look good afaict but I think we should change the updateLoginFormPresence name as well.

Also, are we sure that we won't need this code to show the password fill component that is planned for the control center?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:454
(Diff revision 1)
>  
>    /**
>     * Compute whether there is a login form on any frame of the current page, and
>     * notify the parent process. This is one of the factors used to control the
>     * visibility of the password fill doorhanger anchor.
>     */

This comment needs updating.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:456
(Diff revision 1)
>     * Compute whether there is a login form on any frame of the current page, and
>     * notify the parent process. This is one of the factors used to control the
>     * visibility of the password fill doorhanger anchor.
>     */
>    _updateLoginFormPresence(topWindow) {
>      log("_updateLoginFormPresence", topWindow.location.href);

We should probably change this function name as well.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:468
(Diff revision 1)
> -    // Store the actual form to use on the state for the top-level document.
> -    let topState = this.stateForDocument(topWindow.document);
> -    topState.loginFormForFill = getFirstLoginForm(topWindow);
> -    log("_updateLoginFormPresence: topState.loginFormForFill", topState.loginFormForFill);
> -
>      // Determine whether to show the anchor icon for the current tab.

I think this comment is outdated now

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:471
(Diff revision 1)
>      // Determine whether to show the anchor icon for the current tab.
>      let messageManager = messageManagerFromWindow(topWindow);
>      messageManager.sendAsyncMessage("RemoteLogins:updateLoginFormPresence", {
> -      loginFormOrigin,
> -      loginFormPresent: !!topState.loginFormForFill,
>        hasInsecureLoginForms: hasInsecureLoginForms(topWindow, false),

Depending on the kind of serialization done in sendAsyncMessage (do we need to pass an object?) we could also just pass a boolean instead of an object here.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8789975 [details]
Bug 1286718 - Revert bug 1148026 to remove the initial login fill doorhanger code.

https://reviewboard.mozilla.org/r/77996/#review76592

Comment 12

[Matthew N. [:MattN]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> I can look at the code. We should probably quickly confirm with psackl that
> this idea is definitely abandoned before landing this.

rfeeley (CC'd) was the one who told me the rest of UX didn't like it and regardless of whether we want to do this in the future it's causing problems as mentioned in comment 5.

I did leave some foundation for us to be able to show the saved logins in control center in the future and that's why I left the `updateLoginFormPresence` method and name.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

https://reviewboard.mozilla.org/r/77998/#review76578

> Remove urlbar.loginFillNotificationAnchor.tooltip from browser.dtd as well.

Good catch!

> I'm not sure if updateLoginFormPresence is still the correct name for this. We also don't need to pass around an object, a boolean that represents hasInsecureLoginForms should do it.
> 
> Following conventions, since we have a (kind of) getter above, we could call this setHasInsecureLoginForms(browser, hasInsecureLoginForms)

I was leaving this to support the future control center work to show saved logins though perhaps we would show that section regardless of whether a login form was present in which case we wouldn't need this. I'll rename it.

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

https://reviewboard.mozilla.org/r/77998/#review76578

> I was leaving this to support the future control center work to show saved logins though perhaps we would show that section regardless of whether a login form was present in which case we wouldn't need this. I'll rename it.

I think leaving the message data as an object is more common and readable on the receiving side since it's easy to know what `data.hasInsecureLoginForms` is but not so much what `data` is.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8789973 [details]
Bug 1286718 - Remove password manager's browser_filldoorhanger.js.

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

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8789974 [details]
Bug 1286718 - Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm.

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

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8789975 [details]
Bug 1286718 - Revert bug 1148026 to remove the initial login fill doorhanger code.

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

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

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

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 8789977 [details]
Bug 1286718 - Revert bug 1164028 to remove the additional login fill doorhanger code.

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

Comment 20

[Johann Hofmann [:johannh]]

Comment on attachment 8789974 [details]
Bug 1286718 - Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm.

https://reviewboard.mozilla.org/r/77994/#review76908

Makes sense, thanks!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:454
(Diff revision 2)
>  
>    /**
>     * Compute whether there is a login form on any frame of the current page, and
>     * notify the parent process. This is one of the factors used to control the
>     * visibility of the password fill doorhanger anchor.
>     */

Nit: this comment is outdated now

Comment 21

[Johann Hofmann [:johannh]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

https://reviewboard.mozilla.org/r/77998/#review76910

Thanks! I think it shouldn't be a problem changing this method again once we know how we want to extend it.

Comment 22

[Matthew N. [:MattN]]

Comment on attachment 8789974 [details]
Bug 1286718 - Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm.

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

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8789975 [details]
Bug 1286718 - Revert bug 1148026 to remove the initial login fill doorhanger code.

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

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8789976 [details]
Bug 1286718 - Revert bug 1149975 to remove the additional login fill doorhanger code.

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

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8789977 [details]
Bug 1286718 - Revert bug 1164028 to remove the additional login fill doorhanger code.

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

Comment 26

[Matthew N. [:MattN]]

psackl deferred to rfeeley who told me before a separate doorhanger wasn't in the plans anymore.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/331f9d101a83
Remove password manager's browser_filldoorhanger.js. r=johannh
https://hg.mozilla.org/integration/autoland/rev/a5f0a08902a7
Manually revert LoginManagerContent's _updateLoginFormPresence and fillForm. r=johannh
https://hg.mozilla.org/integration/autoland/rev/f25222e5731a
Revert bug 1148026 to remove the initial login fill doorhanger code. r=johannh
https://hg.mozilla.org/integration/autoland/rev/5e8106a481ec
Revert bug 1149975 to remove the additional login fill doorhanger code. r=johannh
https://hg.mozilla.org/integration/autoland/rev/8bf7bb7bd560
Revert bug 1164028 to remove the additional login fill doorhanger code. r=johannh

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f25222e5731a
https://hg.mozilla.org/mozilla-central/rev/5e8106a481ec
https://hg.mozilla.org/mozilla-central/rev/8bf7bb7bd560