729104 - B2G SMS: Make SmsDatabaseService stub a JS component

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

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.

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

This makes SmsServiceFactory get the SmsDatabaseService via XPCOM if we're on the RIL backend.

Comment 2

[Mounir Lamouri (: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.

Comment 3

[Philipp von Weitershausen [:philikon]]

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

Comment 4

[Mounir Lamouri (:mounir)]

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

Comment 5

[Philipp von Weitershausen [:philikon]]

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

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/75ea99f22bf7