Closed
Bug 786468
Opened 13 years ago
Closed 13 years ago
Support alternate nsIRadioInterfaceLayer implementations
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: m1, Assigned: m1)
Details
Attachments
(1 file, 4 obsolete files)
4.83 KB,
patch
|
Details | Diff | Splinter Review |
Enhance SystemWorkerManager to enable runtime selection of the nsIRadioInterfaceLayer implementation based on something like a system property or config file.
Comment 1•13 years ago
|
||
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?
Assignee | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
Alternate nsIRadioInterfaceLayer implementation can be selected by setting the "ro.moz.ril.contractid" property.
https://tbpl.mozilla.org/?tree=Try&rev=cb97c688596e
Attachment #656315 -
Flags: review?(philipp)
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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 8•13 years ago
|
||
Awesome, that sounds much nicer. I'll give that a go.
Assignee | ||
Updated•13 years ago
|
Attachment #656315 -
Attachment is obsolete: true
Attachment #656315 -
Flags: review?(philipp)
Assignee | ||
Comment 9•13 years ago
|
||
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?
Comment 11•13 years ago
|
||
(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.
Assignee | ||
Comment 12•13 years ago
|
||
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.
Assignee | ||
Comment 15•13 years ago
|
||
also rebased to address patch rot
Attachment #656734 -
Attachment is obsolete: true
Assignee | ||
Comment 16•13 years ago
|
||
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)?
Comment 17•13 years ago
|
||
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).
Assignee | ||
Comment 18•13 years ago
|
||
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".
#
Comment 19•13 years ago
|
||
(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
Assignee | ||
Comment 20•13 years ago
|
||
Attachment #656948 -
Attachment is obsolete: true
Assignee | ||
Comment 21•13 years ago
|
||
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)
Assignee | ||
Updated•13 years ago
|
Attachment #657021 -
Flags: review?(philipp)
Comment 22•13 years ago
|
||
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.
Attachment #657021 -
Flags: review?(philipp)
Attachment #657021 -
Flags: review?(mrbkap)
Attachment #657021 -
Flags: review+
Comment 23•13 years ago
|
||
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)|.
Attachment #657021 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 24•13 years ago
|
||
Trivial change from previous patch to address |if (worker)| coding style nit.
Attachment #657021 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 25•13 years ago
|
||
Component: General → DOM: Device Interfaces
Keywords: checkin-needed
Product: Boot2Gecko → Core
Target Milestone: --- → mozilla18
Comment 26•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•