Convert tabmodalprompt to a JS module

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
16 days ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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.

Updated

5 months ago
Priority: -- → P3
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.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
(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.
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...
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 10

4 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92f9e0296e62
Convert tabmodalprompt binding to JSM module r=Gijs

Comment 11

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92f9e0296e62
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

3 months ago
Summary: Convert tabmodalprompt to a Custom Element or JS module → Convert tabmodalprompt to a JS module
Comment hidden (obsolete)
Comment hidden (obsolete)
Flags: needinfo?(timdream)

Updated

2 months ago
Depends on: 1530557

Comment 14

2 months ago

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

Flags: needinfo?(timdream)

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

Flags: needinfo?(timdream) → needinfo?(gijskruitbosch+bugs)

Oh it is being fixed already.

Flags: needinfo?(gijskruitbosch+bugs)

Updated

21 days ago
Depends on: 1539757
No longer depends on: 1539757
Regressions: 1539757
You need to log in before you can comment on or make changes to this bug.