1148026 - Add a skeleton of the login fill doorhanger

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[:Paolo Amadini]

This is a doorhanger implemented using Doorhangers.jsm, inside a specific LoginDoorhangers.jsm module.

Comment 1

[:Paolo Amadini]

Attachment: Work in progress

Comment 2

[:Paolo Amadini]

To display the doorhanger, from a browser scratchpad:

Cu.import("resource://gre/modules/LoginDoorhangers.jsm");
window.focus();
new LoginDoorhangers.FillDoorhanger({
  browser: gBrowser.selectedBrowser,
  filterString: "www",
}).show();

Comment 3

[Matthew N. [:MattN]]

Attachment: WIP - Counter-proposal not using Doorhangers.jsm

While reviewing bug 1147231 I couldn't convince myself that the added code to maintain and the complexity was worth it so I tried implemented similar behaviour using PopupNotifications.jsm and I still couldn't see worthwhile benefits of Doorhangers.jsm compared to the costs.

Comment 4

[:Paolo Amadini]

Comment on attachment 8586429 [details] [diff] [review]
WIP - Counter-proposal not using Doorhangers.jsm

As discussed yesterday, I think we still need a way to control the anchor without showing the panel, and handle an existing anchor:

Cu.import("resource://gre/modules/LoginDoorhangers.jsm");

let notification = LoginDoorhangers.find({
  browser: gBrowser.selectedBrowser,
});

if (!notification) {
  notification = new LoginDoorhangers.FillDoorhanger({
    browser: gBrowser.selectedBrowser,
    filterString: "www",
  });
}

notification.show();

notification.remove();

Comment 5

[:Paolo Amadini]

Comment on attachment 8586429 [details] [diff] [review]
WIP - Counter-proposal not using Doorhangers.jsm

Review of attachment 8586429 [details] [diff] [review]:
-----------------------------------------------------------------

Do you think we can split this into BaseDoorhanger glue code and FillDoorhanger specific code?

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm
@@ +99,5 @@
> +
> +  bind(notification) {
> +    this.element = this.element || this.chomeDocument.getElementById("login-fill-doorhanger");
> +    this.list = this.list || this.chomeDocument.getElementById("login-fill-list");
> +    this.filter = this.filter || this.chomeDocument.getElementById("login-fill-filter");

We have to make sure element references are not cached as the notification can be moved across windows. Its state (like the applied filter) should be preserved.

Comment 6

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #4)
> As discussed yesterday, I think we still need a way to control the anchor
> without showing the panel, and handle an existing anchor:

I think that shouldn't be hard with `dismissed: true` as an option to PopupNotifications.show

Comment 7

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #5)
> Do you think we can split this into BaseDoorhanger glue code and
> FillDoorhanger specific code?

I really don't think we should as the glue should be minimal. The main contents of LoginDoorhanger.jsm should be actually building the UI that goes in the fill notification.

> ::: toolkit/components/passwordmgr/LoginDoorhangers.jsm
> @@ +99,5 @@
> > +
> > +  bind(notification) {
> > +    this.element = this.element || this.chomeDocument.getElementById("login-fill-doorhanger");
> > +    this.list = this.list || this.chomeDocument.getElementById("login-fill-list");
> > +    this.filter = this.filter || this.chomeDocument.getElementById("login-fill-filter");
> 
> We have to make sure element references are not cached as the notification
> can be moved across windows. Its state (like the applied filter) should be
> preserved.

Yeah, the `||` was just a quick hack for the demo to fit your API. I don't think we need to stick with that API and should just make methods that make sense for our the fill doorhanger's use cases.

Comment 8

[:Paolo Amadini]

Comment on attachment 8586429 [details] [diff] [review]
WIP - Counter-proposal not using Doorhangers.jsm

Does this patch actually work, or is it just an example? I've tried it locally but the anchor icon doesn't appear. The main thing the other module did was to avoid the XBL binding, which we can probably do even without the new module, but may require changes to PopupNotifications.jsm? If so, can you stub the module changes you think are best, so that I can proceed from there?

Comment 9

[:Paolo Amadini]

Attachment: Not using Doorhangers.jsm

This worked, I wasn't passing the right argument to show() when testing.

I've updated it to keep the notification dismissed, using an API with a "browser" setter matching the usage in bug 1149975.

Comment 10

[:Paolo Amadini]

Actual working test code, for the record:

Cu.import("resource://gre/modules/LoginDoorhangers.jsm");

let doorhanger = LoginDoorhangers.FillDoorhanger.find({
  browser: gBrowser.selectedBrowser,
});

if (!doorhanger) {
  window.focus();
  doorhanger = new LoginDoorhangers.FillDoorhanger({
    browser: gBrowser.selectedBrowser,
    filterString: "www",
  });
}

doorhanger.remove();

Comment 11

[:Paolo Amadini]

Attachment: MozReview Request: bz://1148026/paolo

/r/8343 - Bug 1148026 - Add a skeleton of the login fill doorhanger. r=MattN

Pull down this commit:

hg pull -r 23d55d2b959d7b8b3e438830b6fbcf406b1fee36 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[:Paolo Amadini]

I've polished the patch so that it can land independently, and I've added a temporary label stating the panel is there for testing only.

The patch in bug 1149975, which will cause the doorhanger to be actually displayed, will be conditional on an about:config preference. For now, the code in comment 10 (except the "remove" call) is required to display the panel.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8602755 [details]
MozReview Request: bz://1148026/paolo

https://reviewboard.mozilla.org/r/8341/#review7093

Awesome! Looking forward to trying this out.

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:11
(Diff revision 1)
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";

Nit: Put the two `const` on adjacent lines

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:44
(Diff revision 1)
> +   * Associates the doorhanger with its browser.
> +   *
> +   * This may change during the lifetime of the doorhanger, in case the web page
> +   * is moved to a different chrome window by the swapDocShells method.

May want to note that it also makes the icon appear for the browser as initially I thought it was simply setting up the association for future use.

Comment 14

[Matthew N. [:MattN]]

Between this bug and bug 1149975 we should get some tests

Comment 15

[:Paolo Amadini]

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/201c323dcf16

Comment 18

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1148026 - Add a skeleton of the login fill doorhanger. r=MattN