1553950 - We have way too many variants of promiseObserved

Status: NEW

(Toolkit :: General, task, P5)

Description

[Kris Maglione [:kmag]]

Currently we have, for a start, 3 general-purpose helper functions to return a promise when an observer has been notified:

https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/toolkit/components/extensions/ExtensionUtils.jsm#227-249
https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/toolkit/modules/BrowserUtils.jsm#697-719
https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/testing/modules/TestUtils.jsm#37-70

This is not the end of the world, but it would be nice if these could all be replaced with a single helper in toolkit.

However, according to this horrendous one-liner and some code reading:

hg manifest | grep '\.jsm?$' | perl -e 'for my $fn (<STDIN>) { chomp($fn); eval { open(my $fh, "<", $fn); my $meh = join("", <$fh>); if ($meh =~ /new Promise\(.*\n.*(Services.obs.addObserver|ubject.*opic)/) { print $fn, "\n"; } } }'

we actually have many, many more, with varying degrees of badness. Some don't even remove the observer. Some are multiple repetitions of the same code for different topics. Most of them make the code around them difficult to follow.

We should try to update most of these to use the shared helper, and possibly even add an ESLint rule to prevent people from adding new instances.

Comment 1

[Kris Maglione [:kmag]]

Apparently Johann promised to file a follow-up to fix some of this in bug 1529643.

Comment 2

[Johann Hofmann [:johannh]]

Okay, I declare this not urgent enough to warrant spending my time on, unfortunately. I'm happy to help/mentor anyone who wants to give it a stab, though.

Comment 3

[Kris Maglione [:kmag]]

Am I really about to get into a fight with a robot?

Comment 4

[Kris Maglione [:kmag]]

How do I get this bot to stop fighting me?

Comment 5

[Calixte Denizet (:calixte)]

I disabled the bot for this bug and we'll fix this issue.
I suppose it's a consequence of https://bugzilla.mozilla.org/show_bug.cgi?id=1553950#a106886020_546015.

Comment 6

[Marco Castelluccio [:marco]]

The problem is that a bug that is marked a regression can't be a task. In this case this bug doesn't really look like a regression but just follow-up work, so I'm removing the keyword and the regressed by link and turning it into a see also link.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Marco Castelluccio [:marco] from comment #6)

The problem is that a bug that is marked a regression can't be a task. In this case this bug doesn't really look like a regression but just follow-up work, so I'm removing the keyword and the regressed by link and turning it into a see also link.

Thanks. It would be nice if the bot could tell us why it is making these sorts of changes.

Comment 8

[Suhaib Mujahid [:suhaib]]

(In reply to Kris Maglione [:kmag] from comment #7)

Thanks. It would be nice if the bot could tell us why it is making these sorts of changes.

Thank you for your feedback. I filed an issue it followup on this: https://github.com/mozilla/relman-auto-nag/issues/1698