1471403 - Migrate <notificationbox> to a JS class

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Brian Grinstead [:bgrins]]

Even though this one uses two <children /> tag it still looks like a good candidate, because:

- the <notification> slotted into `<children includes="notification"/>` is only ever for DOM nodes created by the widget itself (so can be appended directly as a child node).
- The xul:stack created ahead of the rest of the `<children />` can just be prepended during the connectedCallback

A couple more observations:
- We'll need some small tweaks to the notificationshidden behavior since we don't have xbl:inherits available.
- It doesn't load xbl stylesheet and I'm not seeing a lot of CSS access into anon content other than https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/toolkit/themes/shared/notification.inc.css#18-24, so that should make things easier.

Links:
- https://searchfox.org/mozilla-central/source/toolkit/content/widgets/notification.xml
- https://searchfox.org/mozilla-central/search?q=%3Cnotification&path=xul

Comment 1

[Brian Grinstead [:bgrins]]

Here's an example of what the Custom Element would look like: https://github.com/bgrins/xbl-analysis/blob/gh-pages/elements/generated/Notificationbox.js

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1471403 - WIP - Migrate <notificationbox> to a Custom Element

Comment 3

[Brian Grinstead [:bgrins]]

I put up a pretty minimal migration patch. I'm seeing just a couple of test failures, although I haven't done manual testing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b7b8002e91bae46cb172d9c6561b2a4f83b466&selectedJob=204866539

Comment 4

[:Paolo Amadini]

Attachment: Bug 1471403 - Part 1 - Separate the browser panel from the notification box. r=dao,bgrins

This clarifies the intention of each caller, and opens up the possibility of converting the notificationbox element to a class that creates the DOM nodes on demand.

Comment 5

[:Paolo Amadini]

Attachment: Bug 1471403 - Part 2 - Remove support for notificationsHidden and for children that are not notifications. r=dao,bgrins

This also simplifies how Print Preview hides the chrome, and removes some leftover code.

Depends on D10577

Comment 6

[:Paolo Amadini]

It seems that comm-central would be affected by Part 2 in a few cases. I found this one:

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/suite/mailnews/messenger.xul#237

Comment 7

[:Paolo Amadini]

So, the preliminary work above allows keeping "notificationbox" as a lazily registered Custom Element, but avoid potential performance regressions by lazifying the creation of the actual "notificationbox" elements in browser.xul. These elements are the two global notification boxes and the notification box for each browser.

Comment 8

[:Paolo Amadini]

So the element wouldn't exist until getNotificationBox() is called for the first time.

Comment 9

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4585555d7ffa0dbd5abb0fea1ef333537d6817b9

Comment 10

[:Paolo Amadini]

Should removePreloadedBrowser actually remove the entire panel, rather than just the <browser> element?

https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/browser/base/content/tabbrowser.js#1737-1739

If so, we can probably fix the function and call it from here instead of using _preloadedBrowser:

https://searchfox.org/mozilla-central/rev/efc0d9172cb6a5849c6c4fc0f19d7fd5a2da9643/testing/mochitest/browser-test.js#919-924

Comment 11

[:Paolo Amadini]

Attachment: Bug 1471403 - Part 3 - Lazify the creation of "notificationbox" elements. r=dao,bgrins

Depends on D10578

Comment 12

[:Paolo Amadini]

Attachment: Bug 1471403 - Part 4 - Add a MozElements namespace for element classes. r=bgrins

This provides a way to access specific element classes before any associated Custom Element is instantiated, without creating a new global for each class. This can be useful to access static methods, create derived classes in a page, or allow lazy custom constructors.

Depends on D10892

Comment 13

[:Paolo Amadini]

Attachment: Bug 1471403 - Part 5 - Convert "notificationbox" to a custom class. r=bgrins

Depends on D10893

Comment 14

[:Paolo Amadini]

Note that I might remove everything but the test changes from Part 2, since we delete the binding in Part 5 anyways, and only there I fixed some other consumers so they don't rely on the notificationbox being a container with anonymous children.

Comment 15

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52500fc2bc59e6e2422813a7599a539eb1486104

Comment 16

[Brian Grinstead [:bgrins]]

Just a heads up that the API for <notificationbox> is going to change. Rather than going from XBL -> Custom Element it's going from XBL -> JS class that builds the DOM. And the element will only contain notifications instead of containing notifications and children. Parts 1 and Part 5 are probably the most relevant changesets to TB. Alternatively you can temporarily restore the <notification> and <notificationbox> bindings and continue to bind them to the DOM nodes.

Comment 17

[Jorg K (CEST = GMT+2)]

Thanks, filed bug 1505288.

Comment 18

[Dão Gottwald [:dao]]

(In reply to :Paolo Amadini from comment #10)
> Should removePreloadedBrowser actually remove the entire panel, rather than
> just the <browser> element?

Hmm, yeah, looks like it...

Comment 19

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b76739dce994526670a27dfffff5a7f797e0b75c

Comment 20

[:Paolo Amadini]

I'll move Part 1 to its own bug before landing, so we can better isolate any regressions.

Comment 21

[:Paolo Amadini]

Review needinfo for the remaining parts.

Comment 22

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a29fb4af4efe160a8fa4fa4b7cde088160926549

Comment 23

[:Paolo Amadini]

Combining the lazy notification box with the removal of the outer box probably pushes the performance wins further, although they are still on Windows only:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=95ae3a10a8d79204436e877923fe6120237d1164&newProject=try&newRevision=8ae3deba07e8cb3c49aa35c568489b4ad1195b30&framework=1&showOnlyConfident=1

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c594f12cc04
Part 1 - Stop using notificationsHidden and children that are not notifications in most places. r=dao,bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2a92421b50f
Part 2 - Lazify the creation of "notificationbox" elements. r=dao,bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc40fe6b994
Part 3 - Add a MozElements namespace for element classes. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6676334369f
Part 4 - Convert "notificationbox" to a custom class. r=bgrins

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6c594f12cc04
https://hg.mozilla.org/mozilla-central/rev/a2a92421b50f
https://hg.mozilla.org/mozilla-central/rev/5fc40fe6b994
https://hg.mozilla.org/mozilla-central/rev/b6676334369f