Closed Bug 729104 Opened 9 years ago Closed 9 years ago

B2G SMS: Make SmsDatabaseService stub a JS component

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file)

In bug 712809 we're implementing nsISmsDatabaseService in JS for the RIL/Gonk backend. As a first step towards that, we should move the stub implementation that we have right now to a JS one, with all the attached boilerplate.
Attached patch v1Splinter Review
This makes SmsServiceFactory get the SmsDatabaseService via XPCOM if we're on the RIL backend.
Assignee: nobody → philipp
Attachment #599162 - Flags: review?(mounir)
Comment on attachment 599162 [details] [diff] [review]
v1

Review of attachment 599162 [details] [diff] [review]:
-----------------------------------------------------------------

This could go as-is but I wonder how hard would that be to have a C++ bridge. If it's not that annoying, it might worth it to prevent code being too special-cased.

::: browser/installer/package-manifest.in
@@ +376,5 @@
>  #ifdef MOZ_B2G_RIL
>  @BINPATH@/components/RadioInterfaceLayer.manifest
>  @BINPATH@/components/RadioInterfaceLayer.js
> +@BINPATH@/components/SmsDatabaseService.manifest
> +@BINPATH@/components/SmsDatabaseService.js

You are adding those to browser/ and b2g/ but not to mobile/. Why?

::: dom/sms/src/Makefile.in
@@ +103,5 @@
>    $(NULL)
> +
> +EXTRA_COMPONENTS = \
> +  ril/SmsDatabaseService.js \
> +  ril/SmsDatabaseService.manifest \

FYI, you don't need to prefix with ril/ but it's okay if you prefer to.

::: dom/sms/src/SmsServicesFactory.cpp
@@ +78,2 @@
>      smsDBService = new SmsDatabaseService();
> +#endif

Hmm, I wonder if a C++ dbridge wouldn't be useful at least to prevent that kind of special casing.
Attachment #599162 - Flags: review?(mounir) → review+
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> This could go as-is but I wonder how hard would that be to have a C++
> bridge. If it's not that annoying, it might worth it to prevent code being
> too special-cased.

What do you mean by C++ bridge? A small C++ object that defers all calls to the JS XPCOM object, like in ril/SmsService.cpp?

> ::: browser/installer/package-manifest.in
> @@ +376,5 @@
> >  #ifdef MOZ_B2G_RIL
> >  @BINPATH@/components/RadioInterfaceLayer.manifest
> >  @BINPATH@/components/RadioInterfaceLayer.js
> > +@BINPATH@/components/SmsDatabaseService.manifest
> > +@BINPATH@/components/SmsDatabaseService.js
> 
> You are adding those to browser/ and b2g/ but not to mobile/. Why?

Because it's Gonk only. The only reason we're adding to browser/ at all is when we build Firefox with --enable-b2g-ril to run the RIL on the desktop for debugging/testing purposes.
(In reply to Philipp von Weitershausen [:philikon] from comment #3)
> (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)
> > This could go as-is but I wonder how hard would that be to have a C++
> > bridge. If it's not that annoying, it might worth it to prevent code being
> > too special-cased.
> 
> What do you mean by C++ bridge? A small C++ object that defers all calls to
> the JS XPCOM object, like in ril/SmsService.cpp?

Yes. You don't *have* to. Seems nicer because it moves all the special casing to a code that is only RIL only instead of a platform-agnostic one.

> > ::: browser/installer/package-manifest.in
> > @@ +376,5 @@
> > >  #ifdef MOZ_B2G_RIL
> > >  @BINPATH@/components/RadioInterfaceLayer.manifest
> > >  @BINPATH@/components/RadioInterfaceLayer.js
> > > +@BINPATH@/components/SmsDatabaseService.manifest
> > > +@BINPATH@/components/SmsDatabaseService.js
> > 
> > You are adding those to browser/ and b2g/ but not to mobile/. Why?
> 
> Because it's Gonk only. The only reason we're adding to browser/ at all is
> when we build Firefox with --enable-b2g-ril to run the RIL on the desktop
> for debugging/testing purposes.

Ok.
I ran out of time today to refactor this to be nicer, like you suggested. Will have to do this in a follow up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75ea99f22bf7
https://hg.mozilla.org/mozilla-central/rev/75ea99f22bf7
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.