Looks good to me, thanks Leo!

Thanks for the update, I will move the review to Sasha. Accepting, but I feel like one of my comments about the unused variable was misunderstood?
Overall, we should decide which behavior we want for root + moz:scope. I'm ok if we just want to make them exclusive, even if we only do it in a follow up. But the behavior and implementation is slightly surprising because of this.

Actually this seems to break a few features, at least the download events which rely on getBrowsingContextById to get a browsing context from a regular browsing context id
See https://searchfox.org/firefox-main/rev/e7114bcf28158b46e516178c06201e4849201eba/remote/shared/NavigationManager.sys.mjs#765-770

That should work, thanks!

Thanks!

Thanks for the patch!

Thanks!

Thanks!

Alright, thanks for the cleanup!

Thanks, looks good to me

As mentioned on matrix, some issues when testing the patch.

• using browsingContext.close with the browsingContext id of a chrome browsing context still doesn't produce any error (presumably because we don't use _getNavigable here)
• more important: if you pass a "root" to getTree pointing to a chrome browsing context id, we return information about chrome contexts, even if moz:scope is content or not set

Alright, CI looks good to me, I think we can land the patch. Thanks a lot for addressing all the comments!

In D274481#9548034, @jow13_122 wrote:

Hi @jdescottes,

the spin modifier class was only ever applied to the loader element (<DebuggerImage name="loader" className="spin" />), so in my refactoring I narrowed semantically the .dbg-img.spin to .dbg-img-loader.spin - in devtools/client/debugger/src/components/shared/DebuggerImage.css line 114.

I did this because I thought the loader is the only component that will ever spin, but let me know if it should be kept more generic as .dbg-img.spin.

thanks for the review Sasha!

As explained in the bug at https://bugzilla.mozilla.org/show_bug.cgi?id=2003857#c10, I think this approach can be an acceptable solution for us, but I know adding timers is not great overall so let's discuss. On my end I won't have time to investigate it much more this week, so if we don't want to go for this I would propose to update the expectations for now and work on this again next year.

I am moving this later in the queue

I'll leave the final review for James, lgtm.

Alright, this works for me, I think the new parameter name is a good fit, thanks for the update Henrik.

Alright, thanks for the clarification!

Thanks! I had something slightly different in mind, but that's ok too.