1147231 - Add a module to handle flexible doorhanger notifications

Status: RESOLVED INACTIVE

(Toolkit :: General, defect)

Description

[:Paolo Amadini]

For the new login autofill interface, we need a way to build doorhanger panels that have a flexible UI structure. To do this, we're creating a Doorhangers.jsm module that can be used to build different types of doorhangers. Existing rich popup notifications, as well as standard ones, may also be implemented as specific doorhanger types using the new module.

Comment 1

[:Paolo Amadini]

Attachment: Part 1 - Anchor handling

Comment 2

[:Paolo Amadini]

This initial version can be tested in a browser scratchpad like this:

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

new Doorhangers.DoorhangerBase({
  browser: gBrowser.selectedBrowser,
  anchorType: "geo-notification-icon",
}).show();
new Doorhangers.DoorhangerBase({
  browser: gBrowser.selectedBrowser,
  anchorType: "addons-notification-icon",
}).show();

Doorhangers.find({
  browser: gBrowser.selectedBrowser,
  anchorType: "geo-notification-icon",
}).remove();
Doorhangers.find({
  browser: gBrowser.selectedBrowser,
  anchorType: "addons-notification-icon",
}).remove();

Comment 3

[:Paolo Amadini]

Attachment: Initial patch

This is a self-consistent initial implementation. For testing:

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

new Doorhangers.NotificationDoorhanger({
  browser: gBrowser.selectedBrowser,
  anchorType: "password-notification-icon",
  popupIdForStyling: "password",
  message: "Message?",
  mainAction: { label: "Test", accessKey: "T" },
});

new Doorhangers.NotificationDoorhanger({
  browser: gBrowser.selectedBrowser,
  anchorType: "geo-notification-icon",
  popupIdForStyling: "password",
  message: "Message?",
  mainAction: { label: "Test", accessKey: "T" },
});

Doorhangers.find({
  browser: gBrowser.selectedBrowser,
  anchorType: "geo-notification-icon",
}).remove();

Comment 4

[:Paolo Amadini]

Attachment: With persistence across windows

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8584153 [details] [diff] [review]
With persistence across windows

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

I think dolske wanted to look at this as well.

Comment 6

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #5)
> I think dolske wanted to look at this as well.

For reference, bug 1148026 contains the first consumer of the module that we talked about yesterday.

The next thing to define would be the lifetime of the icon, though having that as a callback method as you suggested means we can think about it in the logins fill doorhanger implementation.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8584153 [details] [diff] [review]
With persistence across windows

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

My gut still says that extending the popup notification XBL binding is better than the added maintenance and complexity of this new module. I think it would be worthwhile to implement that solution as a counter-proposal in bug 1148026.

Some of the things this module would still need to implement:
* anchor lifetime management
* dealing with notifications in background windows

::: browser/base/content/browser.css
@@ +726,5 @@
>  .notification-anchor-icon {
>    -moz-user-focus: normal;
>  }
>  
> +.notification-anchor-icon:not([showing]):not(.showing) {

I still think that having a module to manage notification anchors and is shared with Doorhangers.jsm and PopupNotifications.jsm is cleaner than hacks like this which introduce complexity to the icon box which is currently quite simple.

::: toolkit/modules/Doorhangers.jsm
@@ +28,5 @@
> +  /**
> +   * Retrieves an existing doorhanger associated with a browser and a type, or
> +   * undefined if an associated doorhanger of that type cannot be found.
> +   *
> +   * @param options

@param {Object} options

@@ +31,5 @@
> +   *
> +   * @param options
> +   *        An object with the following properties:
> +   *        {
> +   *          browser:

JSDoc has a standard way to specify object properties on an argument that you could use to define the types.

@@ +49,5 @@
> +  /**
> +   * Associates each chrome nsIXULWindow with the object handling the panel and
> +   * the anchors for that window.
> +   */
> +  chromeHandlerFor(chromeWindow) {

How are other chrome and browser handlers supposed to get registered for use? It seems like the code assumes there is only ever one of each and doesn't allow specifying it. For example, if the WebRTC doorhangers switch to this new system and assuming the social frame creates an iconbox in the toolbar, how would the code know to show the doorhanger anchored in the chat window for Hello?

@@ +57,5 @@
> +      this._chromeHandlersByChromeWindow.set(chromeWindow, chromeHandler);
> +    }
> +    return chromeHandler;
> +  },
> +  _chromeHandlersByChromeWindow: new WeakMap(),

I think either this object should be combined with `Doorhangers` and have private method underscore-prefixed or don't bother underscore prefixing members of the internal object. Does that mean they internal-internal members i.e. super internal?

@@ +79,5 @@
> +   * object, keeping the mapping updated. The specified browser must currently
> +   * be associated to either no BrowserHandler or a BrowserHandler that will be
> +   * discarded.
> +   */
> +  changeBrowserForHandler(browserHandler, otherBrowser) {

I'm not really convinced that we need a separate DoorhangersInternal object with only 3 methods (2 of which have only one caller). The sheer number of objects in this file make the mental modal more confusing.

@@ +92,5 @@
> + * Handles state for a chrome browser window, in particular panel and anchors.
> + *
> + * When created, this object registers its event listeners.
> + */
> +this.DoorhangersInternal.ChromeHandler = function (chromeWindow) {

I think ChromeHandler is ambiguous. ChromeWindowHandler or even WindowHandler is more clear to me. I don't think most people would think that window was referring to a content window when talking about doorhangers. Since this isn't just talking about any browser chrome and depends on a <tabbrowser> it should be more clear that this isn't something you would use for a single <browser> in the chrome.

@@ +186,5 @@
> +    // We share the anchor container visibility with PopupNotifications.jsm.
> +    if (visibleAnchorTypes.length == 0) {
> +      this.anchorContainer.removeAttribute("showing-Doorhangers");
> +      if (!this.anchorContainer.hasAttribute("showing-PopupNotifications")) {
> +        this.anchorContainer.hidden = true;

Same here

@@ +195,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Creates a new anchor element for the speficied type and returns it.

Typo:speficied

@@ +261,5 @@
> +    return new Promise(resolve => {
> +      let listener = () => {
> +        this.panel.removeEventListener("popuphidden", listener);
> +        resolve();
> +      }

Nit: missing semicolon

@@ +348,5 @@
> +   * <browser> element. This may change when tabs are moved across windows.
> +   */
> +  get chromeHandler() {
> +    let chromeWindow = this.browser.ownerDocument.defaultView;
> +    return DoorhangersInternal.chromeHandlerFor(chromeWindow);

Why not just inline the chromeHandlerFor method since it's only called once?

::: toolkit/modules/PopupNotifications.jsm
@@ +737,5 @@
>        if (!haveNotifications) {
>          if (useIconBox) {
> +          this.iconBox.removeAttribute("showing-PopupNotifications");
> +          if (!this.iconBox.hasAttribute("showing-Doorhangers")) {
> +            this.iconBox.hidden = true;

I don't like that the two modules have to know about each other and that's why I think a module that handles the anchor icons would be cleaner.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8584153 [details] [diff] [review]
With persistence across windows

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

I couldn't convince myself that the added code to maintain and the complexity was worth it given that PopupNotifications can already basically do the same things (and if not, we can probably add a few line fixes to make things right). I implemented LoginDoorhangers.jsm (see bug 1148026 comment 3) without the 600 lines of code being added here for Doorhangers.jsm. Note that that file will continue to increase in size as it adds other necessary features to support other doorhangers and as bugs are found.

I much prefer sticking with what we have (PopupNotifications) which is well tested both in automated ways and through years of experience in the field.

== Summary ==
Benefits:
* Nicer API

Costs:
* Maintainability - We are duplicating the implementation of doorhangers in a new module even though much of the logic will be the same.
* Development costs of Doorhangers.jsm - There is still quite a bit of work to be done to make Doorhangers.jsm production-ready including writing of tests, implementing many of the options that PopupNotifications has for anchor lifetime, window events (e.g. activation), etc. and I don't think it makes sense to do that for marginal benefit. With more testing, bugs will be found that need to be fixed too.
* Fragmentation - Having some doorhangers using one code path and other a totally separate one will be confusing to developers for implementing new doorhangers and to debug issues reported.

My proposals to implement the login fill doorhanger (bug 1148026):
A) Use PopupNotifications.jsm to handle the anchor and panel visibility
** We can make a wrapper around it if desired e.g. something like bug 1148026 comment 3
B) Only use anchor management logic from PopupNotifications.jsm and create our own panel when the anchor icon is clicked.

These proposals don't duplicate large chunks of code and don't introduce fragmentation since we're not introducing a general purpose library. Option A has more testing and requires less development time than B.

Comment 9

[:Paolo Amadini]

I think comment 8 is fundamentally right.

The main benefit of Doorhangers.jsm that is not listed there applies to other doorhangers. In fact this module does already most of what PopupNotifications.jsm does but with much less lines of code (remember that it has 607 lines but only 335 if you remove comments, compared to the existing module that has 1047 lines but still 857 without comments).

Its architecture allows for the removal of the hacks that most of the other doorhangers have to use, with a net win in code complexity. Having tried to work with PopupNotifications.jsm for enhancing the login capture doorhanger, I also think that Doorhangers.jsm would really save many hours of developer work for future doorhanger implementations.

That said, I agree we can move forward with some glue code in bug 1148026 for the login fill doorhanger. The new module in this bug should probably be introduced at some point, but together with a plan for migrating existing doorhangers from PopupNotifications.jsm.

Comment 10

[:Paolo Amadini]

Comment on attachment 8584153 [details] [diff] [review]
With persistence across windows

Given that this was reviewed in relation to bug 1148026, removing the flags and keeping the patch around for future reference in case we want to consider this refactoring.

Comment 11

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #9)
> Its architecture allows for the removal of the hacks that most of the other
> doorhangers have to use, with a net win in code complexity. Having tried to
> work with PopupNotifications.jsm for enhancing the login capture doorhanger,
> I also think that Doorhangers.jsm would really save many hours of developer
> work for future doorhanger implementations.

I don't think starting fresh is required to remove hacks added to PopupNotifications.jsm (especially the social stuff) since PopupNotifications exposes a tiny public API meaning it's easy to cleanup the internals without breaking compat.

> That said, I agree we can move forward with some glue code in bug 1148026
> for the login fill doorhanger. The new module in this bug should probably be
> introduced at some point, but together with a plan for migrating existing
> doorhangers from PopupNotifications.jsm.

As I said above, I think PopupNotifications is fixable so a wholesale replacement isn't necessary IMO.

Comment 12

[:Paolo Amadini]

Attachment: Bug 1147231 - Part 1 - Add a module to handle flexible doorhanger notifications.

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

Comment 13

[:Paolo Amadini]

Attachment: Bug 1147231 - Part 2 - Add a skeleton of the login fill doorhanger.

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

Comment 14

[:Paolo Amadini]

With add-on compatibility out of the equation, it should now be possible to simplify PopupNotifications.jsm and adjust the API to make it more similar to this proposal. In fact, the migration to Custom Elements that is part of the XBL removal project will already allow to create a structure based on inherited classes, and remove code complexity related to XBL construction.

Anyways, I'm closing this specific bug and storing the old code, in case it will be needed for reference:

https://reviewboard-hg.mozilla.org/gecko/rev/35abae6c46616b0f4e4a994118dba872e890d985