Extension part of the patch lgtm. I requested one (trivial) extra test change.

FYI there are also xpcshell tests that check the creation of crash dumps. Do they need a workaround?

Thanks. I will try to land it now.

The other patch was updated and broken to the point that it is not close to landing any more. I previously anticipated the other patch to land soon, but that seems no longer the case. If you update this patch without the fix for the other patch, I'll try to get this patch landed.

Test changes look good to me.

The extension impact looks good. I have some feedback on extension test coverage but they do not need to block landing and can also be handled in a follow-up if you'd like. And I am already signing off so that you can land the patch without another review after applying the trivial parts of the feedback. Also feel free to land if there are more substantial changes in the tests only where you are confident of its correctness.

I only looked at the implementation, not the tests in detail.

The latest update of this patch makes multiple changes to the patch that were not requested nor desired. What happened there?

Thanks, the patch looks good. I will try to land the patch in a few days, after https://phabricator.services.mozilla.com/D272280

This looks close to ready. If the remaining issues are addressed without new issues, it is about ready to land.

In D272280#9435600, @rachelrusiecki wrote:

@janerik If this is accepted, would you be ale to land the patch on my behalf? I am a new contributor and do not have permission.

In D272523#9430622, @jrmuizel wrote:

In D272523#9429960, @robwu wrote:

Looks reasonable from the extensions perspective. The input is a URL and the pattern is always escaped except for wildcards, so Unicode support is probably unnecessary.

I think this could be wrong. I think the pattern a?b will match a😀b with the current code but by disabling unicode it won't.

Looks reasonable from the extensions perspective. The input is a URL and the pattern is always escaped except for wildcards, so Unicode support is probably unnecessary.

Thanks for the patch! Since this touches so many files I'd like to help you to complete the patch quickly in order to land it. The patch looks mostly good, so once the feedback below is addressed and a try push is green, I'll land it. I will create a try push at the next patch update if they address the review feedback.

The pathTemplate property has not been documented yet, so in the interest of a clear name, let's proceed with masqueTemplate. Since we're so late in the cycle we cannot uplift the change to the version where this landed, but it's good enough. If wanted we could document that it was available in an earlier version under the older name.

Updated patch. The previous revision yielded incompatible packages on 3.9 as shown in my previous comment, but with the new requirements, it works:

Is there a way for me to reproduce the issue that you were facing?

In D271658#9398025, @ahal wrote:

I believe the 3.10 requirement was needed to fix some kind of dependency conflict in https://bugzilla.mozilla.org/show_bug.cgi?id=1970766

Don't forget to run ./mach export-and-clean as documented at https://mots.readthedocs.io/en/latest/#quick-start.

One difference versus the original patch is that this patch does not skip for condprof. Originally it was needed because xpcshell extension tests temporarily fail on condprof: https://bugzilla.mozilla.org/show_bug.cgi?id=1898471

I have reviewed the tests ahead of settling the discussion in the WECG on what the origin and behavior should be. Once that part concludes I'll also look at the implementation.

Thanks for your patch and patience! I took a look at the patch and it looks quite reasonable. Aside from one implementation change, most of my feedback are in the test coverage. Most of this feedback is minor (and frankly somewhat a subjective opinion on readability), but there are also some gaps/bugs in the test coverage. If you have any questions, let me know.

Please add a unit test that verifies that the notice is shown as needed, and hidden otherwise.

aboutaddons.css changes LGTM. In dark mode, the borders looks more prominent than before (due to the color being a bit brighter), but it's not too distracting.

toolkit/mozapps/extensions/content/shortcuts.css changes LGTM. I confirmed that there is no change, as an earlier patch in this stack (D269440) adds --border-color-selected as an alias for --color-accent-primary.

The logic looks alright to me at this point, but I am not sure if it is reliable long term. Tagging Florian as additional reviewer to confirm.