1901354 - Figure out why SpecialMessageActions is in toolkit, and move it into browser if it doesn't need to be in toolkit

Status: ASSIGNED

(Firefox :: Messaging System, task, P2)

Description

[Shane Hughes [:aminomancer]]

SpecialMessageActions and the toolkit/components/messaging-system folder in general are a bit of an oddity, in that these files are in the toolkit directory but repeatedly reference and rely on files in the browser directory. So they violate a general rule that toolkit code should not rely on browser code, since toolkit code is supposed to be generic to all mozrunner apps.

Does SpecialMessageActions run in any other apps? Are there any unhandled exceptions in that case? I suppose, due to the lazy imports, it is possible to use it without throwing if you are careful in which actions you choose. But does thunderbird actually use it? Do we really need to make it available to other apps?

This directory was added by Kate Hudson in bug 1633368. She's no longer with us, so I'll ask Mardak since he made some comments on the patch and some early changes to the directory. Thanks!

Comment 1

[Ed Lee :Mardak]

The linked bug 1633368 was part of x-man aka experiment manager now known as nimbus. I don't recall the full context, but I would guess it's in toolkit for the same reason why nimbus is in toolkit as it expected to be generally usable outside of browser/ even if only for other toolkit/components, e.g., search, password manager.

Presumably the broader toolkit/components/messaging-system expected ASRouter/etc. to eventually move there as a more generic messaging system router. This might be more doable now that there's browser/components/asrouter split out ??

But especially if moving more to toolkit (and even without), there will still be a need to do browser-only actions from messages. I'm not sure what's the correct pattern, but some browser module could register additional actions to SpecialMessageActions at runtime?

Comment 2

[Shane Hughes [:aminomancer]]

(In reply to Ed Lee :Mardak from comment #1)

But especially if moving more to toolkit (and even without), there will still be a need to do browser-only actions from messages. I'm not sure what's the correct pattern, but some browser module could register additional actions to SpecialMessageActions at runtime?

That makes sense. I guess for the moment, we could make SpecialMessageActions a class and add BrowserSpecialMessageActions as a special instance of it. Then we can leave the files where they are and revisit the move to toolkit in the future when there's more free time. Thanks!

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

From what I can see, it's only referenced outside of browser/ in some infrastructure directories (testing, taskcluster), and that includes in comm-central. The current state of the world makes the documentation unnecessarily confusing because there are two "Messaging System" doc directories in firefox-source-docs, and it's unclear why (and my sense from past discussions is that people reading the code don't always find both, even though they are cross-linked).

Unless we have an explicit reason and plan to invest the effort to move the rest of this stuff to toolkit (and I'm not aware of any expressed interest in that, though perhaps someone else is), I wonder if it wouldn't make more sense from an ROI/clarity/complexity standpoint to just pull that stuff out of toolkit and fold it into ASRouter.

I'd be interested in hearing Punam's thoughts here...

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Part of my thinking here is that, in my experience, it's less an issue of free time and more an issue of priorities. Until and unless the separation would provide somebody who wants it a non-trivial amount of value, I have a hard time imagining prioritizing this above other work.

Comment 5

[:Gijs (out for now; he/him)]

See also bug 1898012 which I filed a few weeks back because of a similar conundrum.

We need to make some decisions about which parts of the intersecting bits (nimbus, messaging system, asrouter, jexl) we actually need/want on Android and/or Thunderbird or other toolkit consumers beyond Firefox.

Dependencies from Firefox on toolkit bits are fine. Dependencies the other way around are not.

It's not clear to me if/how Thunderbird relies on Nimbus/ASRouter/MessagingSystem at all today. AFAICT Android uses Nimbus only through other means and doesn't need any of the JS bits in m-c for it.

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Dan Mosedale (:dmosedale, :dmose) from comment #4)

Part of my thinking here is that, in my experience, it's less an issue of free time and more an issue of priorities. Until and unless the separation would provide somebody who wants it a non-trivial amount of value, I have a hard time imagining prioritizing this above other work.

Value from resolving some of this:

• we make it clear which parts are actually available outside of Desktop Firefox and for what applications we need/want to run tests for those parts, and where we can/cannot depend on them. Note that right now, AIUI, C++ code in Gecko can use Nimbus in some ways, and so we probably need all the functional bits for that to exist in toolkit. I don't know how much of the discussed parts are entailed by this.
• we avoid shipping unnecessary code with Android where binary/installer size is paramount
• people stop copy-pasting code with "bad" dependencies (ie from toolkit to browser) around the place
• the docs and code are easier to follow and reason about by people outside of the teams working directly on Nimbus/MS/ASRouter

So I think we should at least get to some level of clarity here.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Agreed that the bug you filed (bug 1898012) really encapsulates the high-order decision here.

Re prioritization, my statement was unclear. Based on my knowledge at the time I wrote that, I was having a hard time imagining prioritizing moving things toward living in toolkit rather than in browser, but I'm now somewhat less sure. I'll add some comments to the other bug about the options you've raised there.

I agree that getting more clarity (and ideally resolution) than we have now is relatively high value.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

the docs and code are easier to follow and reason about by people outside of the teams working directly on Nimbus/MS/ASRouter

s/outside/inside and outside/

:-)

Comment 9

[Punam Dahiya [:pdahiya]]

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

(In reply to Dan Mosedale (:dmosedale, :dmose) from comment #4)

Part of my thinking here is that, in my experience, it's less an issue of free time and more an issue of priorities. Until and unless the separation would provide somebody who wants it a non-trivial amount of value, I have a hard time imagining prioritizing this above other work.

Value from resolving some of this:

• we make it clear which parts are actually available outside of Desktop Firefox and for what applications we need/want to run tests for those parts, and where we can/cannot depend on them. Note that right now, AIUI, C++ code in Gecko can use Nimbus in some ways, and so we probably need all the functional bits for that to exist in toolkit. I don't know how much of the discussed parts are entailed by this.
• we avoid shipping unnecessary code with Android where binary/installer size is paramount
• people stop copy-pasting code with "bad" dependencies (ie from toolkit to browser) around the place
• the docs and code are easier to follow and reason about by people outside of the teams working directly on Nimbus/MS/ASRouter

So I think we should at least get to some level of clarity here.

+1 on reducing complexity and moving actions having browser dependency in a module in asrouter folder that can register actions into the SpecialMessageActions only for Desktop Firefox at runtime. I will submit a patch and a tech doc capturing this direction.

This can be followed by removing targeting context Targeting.sys.mjs toolkit module dependency from ASRouterTargeting so that messaging-system toolkit module can be available everywhere if needed

Comment 10

[Shane Hughes [:aminomancer]]

In case I wasn't clear, I'm not proposing moving anything into toolkit. This bug is strictly about moving what is already in toolkit into browser (and finding out why it's in there in the first place). We could just move all of this into browser and reconsider it if there's ever a need to make this available to android. Right now, SpecialMessageActions is not used outside of browser, so it doesn't make sense for it to be in toolkit. If the messaging system were to be available elsewhere, we'd need to move a bunch of other files into toolkit as well.

(In reply to Punam Dahiya [:pdahiya] from comment #9)

This can be followed by removing targeting context Targeting.sys.mjs toolkit module dependency from ASRouterTargeting so that messaging-system toolkit module can be available everywhere if needed

Hmm... The Targeting/ASRouterTargeting dependencies are especially confusing. Targeting (toolkit) depends on ASRouterTargeting (browser), and vice versa. And a bunch of other toolkit modules depend on one or the other also. Unless we move the whole nimbus directory into browser, I think any targeting attribute that is depended on by toolkit modules (e.g. nimbus stuff) should be in Targeting rather than in ASRouterTargeting, and then Targeting should be able to stop importing ASRouterTargeting.

ni?barret - what do you think about moving toolkit/components/nimbus to browser/components/nimbus? Does anything outside of Firefox depend on it or are there plans to use it outside of Firefox?

Comment 11

[Punam Dahiya [:pdahiya]]

Attachment: Bug 1901354 - Move message actions having browser dependency out of toolkit

Comment 12

[Beth Rennie [:beth] (she/her)]

Nimbus cannot move out of toolkit. It is used by components in toolkit.

Comment 13

[:Gijs (out for now; he/him)]

ISTM that nimbus, jexl, and the concept of "targeting" and the related modules should all be in toolkit.

Inasmuch as there are desktop-browser-specific targeting bits, they should be using dependency injection, rather than Nimbus having to rely on importing ASRouterTargeting (also here ).

I don't have strong feelings on messaging-system, and it's never been clear to me what ASRouter really is/means - so I guess both could be in browser, as long as we use dependency injection to get the relevant bits "into" the toolkit default targeting etc. for nimbus.

Comment 14

[Shane Hughes [:aminomancer]]

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

I don't have strong feelings on messaging-system, and it's never been clear to me what ASRouter really is/means - so I guess both could be in browser, as long as we use dependency injection to get the relevant bits "into" the toolkit default targeting etc. for nimbus.

There's a brief overview on here. These messaging capabilities originally revolved around controlling new tab page content called Activity Stream. So ASRouter is short for Activity Stream Router (as in routing messages to the modules that render them). Code wise it's basically the core of the messaging system. I'm sure we'll get around to changing the name now that we've largely disentangled it from the newtab code.

Comment 15

[Punam Dahiya [:pdahiya]]

Filed Bug 1903898 to tackle remaining browser dependency from toolkit messaging-system module.