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

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.

(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?

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

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 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.

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 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.

(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.

Addressed only the first two nits in comment 7. Try: https://tbpl.mozilla.org/?tree=Try&rev=2d6b1d85ed5a

Thank you for understanding.

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

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

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

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"

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?

(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.

(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?

(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.

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.