Module definition for RIL components

RESOLVED FIXED in mozilla35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
mozilla35
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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
(Assignee)

Updated

4 years ago
Flags: needinfo?(khuey)
Flags: needinfo?(bugs)
(Assignee)

Updated

4 years ago
Depends on: 864485
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.
No longer depends on: 864485
Flags: needinfo?(bugs)
(Assignee)

Comment 2

4 years ago
(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?
Flags: needinfo?(bugs)
(Assignee)

Updated

4 years ago
Assignee: nobody → vyang
So what would go to dom/mobileconnection, and to which file there and why?
Flags: needinfo?(bugs)
(Assignee)

Comment 4

4 years ago
Created attachment 8486372 [details] [diff] [review]
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 6

4 years ago
Created attachment 8489334 [details] [diff] [review]
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.
Attachment #8486372 - Attachment is obsolete: true
Attachment #8489334 - Flags: review?(bugs)
Flags: needinfo?(khuey)
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.
Attachment #8489334 - Flags: review?(bugs) → review-
(Assignee)

Comment 8

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
Created attachment 8489827 [details] [diff] [review]
patch : v2

Addressed only the first two nits in comment 7.

Try: https://tbpl.mozilla.org/?tree=Try&rev=2d6b1d85ed5a
Attachment #8489334 - Attachment is obsolete: true
Attachment #8489827 - Flags: review?(bugs)
Attachment #8489827 - Flags: review?(bugs) → review+
(Assignee)

Comment 10

4 years ago
Thank you for understanding.
(Assignee)

Comment 11

4 years ago
Created attachment 8489904 [details] [diff] [review]
patch : v3

Fix build failures on Windows & Mac.
s/class already_AddRefed/struct already_AddRefed/
Attachment #8489904 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #8489827 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Flash Gecko & Gaia into Flame v123 ok. Waiting for try results.
https://hg.mozilla.org/mozilla-central/rev/69870df1c72f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a9a2db3660cb
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 20

4 years ago
(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.
(Assignee)

Updated

4 years ago
Blocks: 1072275
(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?
(Assignee)

Comment 22

4 years ago
(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.
(Assignee)

Comment 23

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.