1064231 - Module definition for RIL components

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

Bug 843452 suggested we move factory method for mobileconnection into nsLayoutModule.cpp directly. However, all other existing factory methods for SMS and Telephony are in their own factory classes, so bug 843452 becomes an exception and scatters RIL related lines in different places.

Besides, in order to deprecate RILContentHelper in bug 815526, moving other components, Cell Broadcast/Voicemail/ICC, to IPDL will also introduce more factory methods in nsLayoutModule.cpp if we keep the same method. In additioan, with bug 1038606 and its descendants, there will be different back-end for different architecture, and this follows more header files, components to be included into nsLayoutModule.cpp.

Therefore I suggest we create yet another module definition file for all RIL components as WiFi does. [1] Considering RIL may exist on different architectures, maybe we create a dom/system/ril/Modules.cpp for this.

How do you think?

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkWorker.cpp#285

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

In general I'd prefer to just use nsLayoutModule.cpp for all the factory methods if possible, but
if that makes code harder to maintain, other approaches are ok too.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #1)
> In general I'd prefer to just use nsLayoutModule.cpp for all the factory
> methods if possible, but
> if that makes code harder to maintain, other approaches are ok too.

How about we stay in nsLayoutModule.cpp, but move mobileconnection factory function into dom/mobileconnection as others do? This way we don't have to create dom/system/ril, no additional headers to be exported because of such folder separation, and all the related parts stay in their home folders as usual?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So what would go to dom/mobileconnection, and to which file there and why?

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: 0001-Bug-1064231-move-mobileconnection-factory-method-int.patch

1) Hide back-ends from nsLayoutModule. All the back-end info is only available inside MobileConnectionFactory.cpp.

2) Reuse NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR. MobileConnectionFactory is only responsible for instantiate a MobileConnectionService instance.

3) remove MOZ_B2G_RIL guards from nsLayoutModule. They are not supposed to be there anyway because of bug 947116.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8486372 [details] [diff] [review]
0001-Bug-1064231-move-mobileconnection-factory-method-int.patch

Not sure we really need the new files. We could just put MobileConnectionFactory to some existing file. Other than that, looks good.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

1) Unify instantiation process for RIL services. So far MobileConnection/MobileMessage/Telephony are affected. Others to come in bug 864484, bug 864489, and bug 833229.

2) Add NS_CreateFooService for affected components. FooFactory classes are obsoleted. The new factory functions are located in their major DOM component definition files. For example, NS_CreateTelephonyService is in dom/telephony/Telephony.cpp.

3) Don't really need singleton pattern for MobileConnectionIPCService because NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR has done the same.

4) SmsIPCService implements multiple mobile message service interfaces. Create one for all of them.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8489334 [details] [diff] [review]
patch

>+/* static */ already_AddRefed<SmsIPCService>
>+SmsIPCService::GetSingleton()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  if (!sSingleton)
>+    sSingleton = new SmsIPCService();
Always {} with if in C++ code, please

>+%{C++
>+#include "nsCOMPtr.h" // For already_AddRefed<T>.
>+%}
I _think_ you don't to include nsCOMPtr.h in order to return already_AddRefed.
template<class E> class already_AddRefed


>-#ifdef MOZ_B2G_RIL
>   { "profile-after-change", "MobileConnection Service", NS_MOBILE_CONNECTION_SERVICE_CONTRACTID },
>-#endif
Er, why do we want to remove that ifdef? Same also elsewhere. child process in e10s shouldn't be able to create
mobile connection service.

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Olli Pettay [:smaug] from comment #7)
> >-#ifdef MOZ_B2G_RIL
> >   { "profile-after-change", "MobileConnection Service", NS_MOBILE_CONNECTION_SERVICE_CONTRACTID },
> >-#endif
> Er, why do we want to remove that ifdef? Same also elsewhere. child process
> in e10s shouldn't be able to create
> mobile connection service.

Bug 947116. MOZ_B2G_RIL has to go. We should probe backend existence in the future. So, child process should always try to open IPC channel and let parent process decide whether it's available or not.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

Addressed only the first two nits in comment 7.

Try: https://tbpl.mozilla.org/?tree=Try&rev=2d6b1d85ed5a

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Thank you for understanding.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v3

Fix build failures on Windows & Mac.
s/class already_AddRefed/struct already_AddRefed/

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Try on failed platforms: https://tbpl.mozilla.org/?tree=Try&rev=1e612b7bf49b

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Flash Gecko & Gaia into Flame v123 ok. Waiting for try results.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/69870df1c72f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69870df1c72f

Comment 16

[Carsten Book [:Tomcat]]

backed this out since this might have caused ongoing frequent cppunit tests like https://tbpl.mozilla.org/php/getParsedLog.php?id=48267235&tree=B2g-Inbound

also i guess the uuid change still have to land

where still is mentioned 15:30:26     INFO -  JavaScript strict warning: jar:file:///system/b2g/omni.ja!/components/MobileConnectionService.js, line 340: ReferenceError: reference to undefined property aDestInfo[key]
15:30:26     INFO -  JavaScript strict warning: , line 0: ReferenceError: reference to undefined property "QueryInterface"
15:30:26     INFO -  [217] WARNING: Trying to SendCommand() without a SLC: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 1279
15:30:26     INFO -  [217] WARNING: Trying to SendCommand() without a SLC: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 1279
15:30:27     INFO -  [217] WARNING: NS_ENSURE_TRUE(iccInfo) failed: file ../../../gecko/dom/bluetooth/bluez/BluetoothHfpManager.cpp, line 688
15:30:34     INFO -  JavaScript strict warning: , line 0: ReferenceError: reference to undefined property "QueryInterface"

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a9a2db3660cb

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a9a2db3660cb

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

I am getting this erro in Firefox for Android:
I/GeckoConsole(17166): While creating services from category 'profile-after-change', could not create service for entry 'MobileConnection Service', contract ID '@mozilla.org/mobileconnection/mobileconnectionservice;1'

Why does this patch remove the preprocessing for services that are B2G only?

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> I am getting this erro in Firefox for Android:
> I/GeckoConsole(17166): While creating services from category
> 'profile-after-change', could not create service for entry 'MobileConnection
> Service', contract ID
> '@mozilla.org/mobileconnection/mobileconnectionservice;1'
> 
> Why does this patch remove the preprocessing for services that are B2G only?

Comment 8. But MobileConnectionService still don't have to be instantiated while 'profile-after-change'. That's a know issue since bug 843452.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> (In reply to Mark Finkle (:mfinkle) from comment #19)
> > I am getting this erro in Firefox for Android:
> > I/GeckoConsole(17166): While creating services from category
> > 'profile-after-change', could not create service for entry 'MobileConnection
> > Service', contract ID
> > '@mozilla.org/mobileconnection/mobileconnectionservice;1'
> > 
> > Why does this patch remove the preprocessing for services that are B2G only?
> 
> Comment 8. But MobileConnectionService still don't have to be instantiated
> while 'profile-after-change'. That's a know issue since bug 843452.

Does comment 8 address the current error happening in Fennec, which does not use e10s? I assume bug 1072275 was filed to stop trying to create a service (MobileConnection) that does not exist in Fennec and Firefox?

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Does comment 8 address the current error happening in Fennec, which does not
> use e10s? I assume bug 1072275 was filed to stop trying to create a service
> (MobileConnection) that does not exist in Fennec and Firefox?

It was filed to remove MobileConnectionService from 'profile-after-change'. That means we don't try to create a MobileConnectionService when start up. And we don't create MobileConnectionService after that because there is no back end that notifies MobileConnectionService and DOM API is not enabled on unsupported platforms.

In summary, MobileConnectionService will not be instantiated on platforms other than B2G. So, yes.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

To be completed, I once had a patch to guard all RIL lines with MOZ_B2G_RIL, but then I was told by DOM peers that Mozilla don't want so much preprocessor guards and prefer the removal of them. Long after that, Andreas gave a comment on b2g mailing list and hence came bug 1072275. If any one has a different opinion, please comment on bug 1072275. I will listen, or 'read' actually.