Closed Bug 1148026 Opened 5 years ago Closed 4 years ago

Add a skeleton of the login fill doorhanger

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 4 obsolete files)

This is a doorhanger implemented using Doorhangers.jsm, inside a specific LoginDoorhangers.jsm module.
Iteration: --- → 39.3 - 30 Mar
Points: --- → 5
Flags: qe-verify-
Flags: firefox-backlog+
Attached patch Work in progress (obsolete) — Splinter Review
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
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();
Depends on: 1147231
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
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.
No longer depends on: 1147231
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 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.
Blocks: 1149975
(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
(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.
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
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?
Attachment #8586429 - Flags: feedback?(MattN+bmo)
Attached patch Not using Doorhangers.jsm (obsolete) — Splinter Review
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.
Attachment #8586429 - Attachment is obsolete: true
Attachment #8586429 - Flags: feedback?(MattN+bmo)
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();
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Attached file MozReview Request: bz://1148026/paolo (obsolete) —
/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/
Attachment #8602755 - Flags: review?(MattN+bmo)
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.
Attachment #8583979 - Attachment is obsolete: true
Attachment #8595904 - Attachment is obsolete: true
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.
Attachment #8602755 - Flags: review?(MattN+bmo) → review+
Between this bug and bug 1149975 we should get some tests
https://hg.mozilla.org/mozilla-central/rev/201c323dcf16
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attachment #8602755 - Attachment is obsolete: true
Attachment #8619886 - Flags: review+
Blocks: 1267107
You need to log in before you can comment on or make changes to this bug.