1849308 - Remove the handling of "tabmodal-dialog-loaded" observer notifications

Status: RESOLVED FIXED

(Remote Protocol :: Agent, task, P1)

Description

[Alexandra Borovova [:Sasha]]

tabmodal-dialog-loaded notification should have been deprecated in Firefox 89 and probably can be cleaned up.

Comment 1

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1849308 - [remote] Clean up tabmodal-dialog-loaded observer listener.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hi Gijs, we stumbled over the usage of tabmodal-dialog-loaded in Marionette. There we have a comment that this observer is deprecated since Firefox 89, but we can still see that observer notification used in a couple of places:

https://searchfox.org/mozilla-central/search?q=tabmodal-dialog-loaded&path=

Do you think it is safe to get it removed for Marionette or should we wait until any code has been cleaned-up from using this observer. If the latter is there a plan when it will get removed? Thanks!

Comment 3

[:Gijs (he/him)]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #2)

Hi Gijs, we stumbled over the usage of tabmodal-dialog-loaded in Marionette. There we have a comment that this observer is deprecated since Firefox 89, but we can still see that observer notification used in a couple of places:

https://searchfox.org/mozilla-central/search?q=tabmodal-dialog-loaded&path=

I mean, all of these are in "remote", or in tests, apart from the place that fires the observer notification.

The blame on tests is from bug 1686315 and similar, so I think is primarily for backwards compat at a time when both prompt implementations could be used. The implementation was behind a pref, see e.g. https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/browser/actors/PromptParent.sys.mjs#21-26 .

The pref defaults to true nowadays.

But off-hand it looks like a substantial amount of "what if the pref is off" code still exists.

Do you think it is safe to get it removed for Marionette

What usecases is marionette catering for? Do we care about people who flip this pref in about:config / when starting Firefox using marionette?

I would argue we probably shouldn't and we should remove support.

or should we wait until any code has been cleaned-up from using this observer. If the latter is there a plan when it will get removed? Thanks!

I can't quickly find an existing bug to track its removal. I don't see any reason not to remove all of it at this point, though.

Comment 4

[:Gijs (he/him)]

The one caveat is that the coverage information on https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/toolkit/components/prompts/src/CommonDialog.sys.mjs#252 suggests that there are still a miniscule number of consumers...

... but I suspect this might be because of e.g. https://searchfox.org/mozilla-central/rev/78af6dbe0591376d9d5d678adaf21aa69dcc3506/browser/base/content/test/tabPrompts/browser_openPromptInBackgroundTab.js#26-29 which purposefully tests the old implementation.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to :Gijs (he/him) from comment #3)

What usecases is marionette catering for? Do we care about people who flip this pref in about:config / when starting Firefox using marionette?

We do not have any specific use-case for it in Marionette, but only carrying it around while we have support for it in Firefox. Nevertheless given that the new API is enabled by default for quite some time and we want to keep testing the default setting of the browser, we are happy to get everything under /remote updated and code covering this old feature removed.

I would argue we probably shouldn't and we should remove support.

Sounds good for us. We will do.

or should we wait until any code has been cleaned-up from using this observer. If the latter is there a plan when it will get removed? Thanks!
I can't quickly find an existing bug to track its removal. I don't see any reason not to remove all of it at this point, though.

I filed bug 1849543 for the removal in Toolkit and Browser.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bd1fea585a71
[remote] Clean up tabmodal-dialog-loaded observer listener. r=webdriver-reviewers,whimboo

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/bd1fea585a71