1512048 - Convert tabmodalprompt to a JS module

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Brian Grinstead [:bgrins]]

This appears to be only called in one place: https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/browser/base/content/browser.js#8049.

There are two insertion points, but I think only the [includes="row"] is used: https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/toolkit/components/prompts/content/tabprompts.xml.

A couple notes:
 - If running this through the converter script, be aware that there are some #ifdefs that will be dropped and will need to be re-added with AppConstants check.
 - The attributes on xbl:content get copied onto the host node
 - It looks like there's some cleanup that could be done in the <constructor>, around the lookup of child nodes in `this.ui`. Could happen here or in a follow up.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am working on getting it converted to a JSM and have the caller pass around the JSM class instead of the element reference. The call path is quite messy but let's hope our test coverage is reliable.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e560bd5c8313954754c06466166d259d587582fa

So ... this kind of work except it is not easy for tests to get to the instances of the newly created class. I will need to investigate more about this or switch to custom element.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

I've not fixed all the tests yet.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

What I found it extremely weird is that when the tabprompt binding is forced attached with

https://searchfox.org/mozilla-central/rev/8a135a9c5a96b59269f544fcaee76d8fd5a7026a/browser/base/content/browser.js#8032

the children binding seems to be all force attached too, even when many of them are set to hidden=true in the markup (or under a parent that is hidden). I don't think I can duplicate that with my setup...

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This should work

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

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1512048 - Convert tabmodalprompt binding to JSM module r=Gijs

This converts the tabmodalprompt binding to a class, to be constructed along side with the element
by TabModalPromptBox.

TabModalPromptBox will keep the instances in a map and pass it to the callers, instead of the element.
The tests and callers can access the class instance by passing the element reference to the map.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92f9e0296e62
Convert tabmodalprompt binding to JSM module r=Gijs

Comment 11

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/92f9e0296e62

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Naming change of the variable in this test should be reverted. I will push a follow-up.

https://hg.mozilla.org/mozilla-central/rev/92f9e0296e62#l6.12

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually reading closely it right...

Comment 14

[Liviu Seplecan]

Hi Tim Guan-tin Chien, from the regression range, it seems that the bug 1512048 causes this regression. Please take a look on it.
Thanks

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am not available for making code contribution right now. Gijs, would you like to take over?

Comment 16

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Oh it is being fixed already.