Enhance SystemWorkerManager to enable runtime selection of the nsIRadioInterfaceLayer implementation based on something like a system property or config file.
I *think* you should get this for free from XPCOM by simply registering a different implementation for the same CID (class ID). It also depends a bit on what you mean by "runtime". Can you elaborate a bit on the use case? Swapping out all of nsIRadioInterfaceLayer seems like a big hammer. Maybe there's a more fine-grained solution?
By runtime I mean "via something external to the gecko binaries" (ie, an extension). The intent is to not get in the way of gecko updates but enable an external nsIRadioInterfaceLayer implementation. I'm so not an XPCOM expert but seems like an extension registering as implementing NS_RADIOINTERFACELAYER_CID would result in undefined behavior -- maybe XPCOM picks up the extension's implementation while enumerating classes but maybe not.
I just tried adding {2d831c8d-6017-435b-a80ce5d422810cea} (NS_RADIOINTERFACELAYER_CID) to the extension's chrome.manifest and XPCOM still picked up the in-gecko implementation. Not surprising at all but just wanted to confirm.
Created attachment 656315 [details] [diff] [review] attempt #1 Alternate nsIRadioInterfaceLayer implementation can be selected by setting the "ro.moz.ril.contractid" property. https://tbpl.mozilla.org/?tree=Try&rev=cb97c688596e
A CID ("class ID") refers to a particular implementation class so you can never override a previously registered CID. You want to override the "contract ID" instead.
A contract ID is not currently defined for nsIRadioInterfaceLayer, just a CID [1]. But if we were to define a contract ID for this interface, @mozilla.org/ril/radiointerfacelayer;1 or something, an extension could override it via its chrome.manifest? Seems like this would have the same ambiguity problem during XPCOM component enumeration (but again, I am *not* an XPCOM expert). [1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/SystemWorkerManager.cpp#327
Yep, you'll need to register a contract ID for the RIL, then switch the createInstance call in SystemWorkerManager.cpp to use the contract ID instead of a CID. Then an extension can override the contract ID and will get created instead. The only issue will be that the extension's contract ID override happens slightly after startup (if we call createInstance before extension registration then this won't work). I think it should be ok since we initialize the SystemWorkerManager (and then the RIL) at "profile-after-change" ( http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1232 ). Not sure what you mean by the "ambiguity problem during XPCOM component enumeration"... Think of the component manager as a big hash table mapping contract IDs to class factories (that's really all it is). By overriding the contract ID you're just replacing the old factory with a different factory.
Awesome, that sounds much nicer. I'll give that a go.
Created attachment 656734 [details] [diff] [review] attempt #2 -- using a contract ID This is much nicer now with the system property junk removed. Maybe the contact ID name as a little weak: "@mozilla.org/ril;1". Not attached to it by any means so suggestions welcome!
Comment on attachment 656734 [details] [diff] [review] attempt #2 -- using a contract ID This looks great! Though, I wonder why you need to switch nsIWorkerHolder to nsISupports here?
(In reply to ben turner [:bent] from comment #10) > This looks great! Though, I wonder why you need to switch nsIWorkerHolder to > nsISupports here? Presumably so that alternate implementations of nsIRadioInterfaceLayer don't have to also implement nsIWorkerHolder, e.g. if they don't want to use ChromeWorkers.
Yep, that's correct. Can I ask you or bent for a r? I'll push this to try later today, but I don't expect any issues given no significant change since the last try run.
Hm, then you would want it to be nsIRadioInterfaceLayer, not nsISupports.
The other thing is we'll have to be careful with this change. Currently consumers of nsIRadioInterfaceLayer have to get the system worker manager service and then call do_GetInterface on it. If we actually have a separate component that we want to use then we should probably change the createInstance call to getService, otherwise other consumers could just use the ril contractid to create multiple instances of the new component.
Created attachment 656948 [details] [diff] [review] attempt #3 -- use nsIRadioInterfaceLayer not nsISupports. also rebased to address patch rot
Good point about using nsIRadioInterfaceLayer, thanks. (In reply to ben turner [:bent] from comment #14) > If we actually have a separate > component that we want to use then we should probably change the > createInstance call to getService, otherwise other consumers could just use > the ril contractid to create multiple instances of the new component. I'm not thinking of any changes to the consumers, they would continue going through swm. Although I see how declaring a contractid makes it a little more likely that somebody in the future would attempt to CI the ril directly and wonder why it didn't work as expected. Is that a risk we can accept for now (maybe until the 1st or 2nd person stubs their toe)?
The reason for the current setup is that SWM is in charge of the RIL. In particular, it tries to ensure nobody else creates the RIL first, so that the SWM gets the chance to attach its things to the worker in time. So yes, I agree about not changing consumers. I would be ok with a big fat comment in the manifest to let people know to not use the contract id to acquire the RIL (instead tell them how else to do it).
How about this for the comment? I'll upload a new patch once we get the wording right: # RadioInterfaceLayer.js # # IMPORTANT: # Consumers of nsIRadioInterfaceLayer should invoke # nsIInterfaceRequestor::GetInstance() as implemented by # "@mozilla.org/telephony/system-worker-manager;1" to # obtain an instance. # # DO NOT use do_CreateInstance()/do_GetService() to directly # instantiate "@mozilla.org/ril;1". #
(In reply to Michael Vines [:m1] from comment #18) > # IMPORTANT: > # Consumers of nsIRadioInterfaceLayer should invoke > # nsIInterfaceRequestor::GetInstance() as implemented by itym nsIInterfaceRequestor::GetInterface(). sgtm otherwise
Created attachment 657021 [details] [diff] [review] attempt #4 -- += contract ID usage disclaimer
Try run: https://tbpl.mozilla.org/?tree=Try&rev=65a80fc99720 (Looks like I ended up pushing two unrelated inbound patches to try in addition to this patch as well. A little unhygienic but builds are looking green so far regardless)
Comment on attachment 657021 [details] [diff] [review] attempt #4 -- += contract ID usage disclaimer Review of attachment 657021 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/SystemWorkerManager.cpp @@ +472,1 @@ > } I am not qualified to rubberstamp this refcounting black magic. mrbkap, is this ok? lgtm otherwise.
Comment on attachment 657021 [details] [diff] [review] attempt #4 -- += contract ID usage disclaimer Review of attachment 657021 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/SystemWorkerManager.cpp @@ +472,1 @@ > } Yeah, this is fine. @@ +498,3 @@ > > + nsCOMPtr<nsIWorkerHolder> worker = do_QueryInterface(ril); > + if (worker != 0) { Please don't compare with 0, just |if (worker)|.
Created attachment 657327 [details] [diff] [review] attempt #5 -- address trivial style issue Trivial change from previous patch to address |if (worker)| coding style nit.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb57e4e09bb
https://hg.mozilla.org/mozilla-central/rev/bfb57e4e09bb
or
