nsISmsService acts as glue between the backend (in our case the RIL worker) and the DOM API for sending. For receiving, we simply have to send an nsIDOMMozSmsMessage object as the subject of the "sms-received" observer notification. nsTelephonyWorker could probably take care of both, since it is in charge of sending RIL parcels. This would be very much analogous to telephony, where nsTelephonyWorker implements nsITelephone -- the glue between the backend and the DOM API.

Also, I think we should rename nsTelephonyWorker to RILWorkerBridge to reflect the actual purpose of this object. It's neither limited to just Telephony nor is it the actual worker -- it's an object that lets us talk to the worker.

This is a WIP for an nsISmsService for gonk that just wraps nsITelephone (to be renamed to nsIRadioInterfaceLayer (see bug 711671).

This is the plumbing for incoming SMS. Right now I'm trying to create the nsIDOMMozSmsMessage entirely in JS, but perhaps we can make the existing C++ implementation instantiable from JS... I don't care much either way tbh.

Comment on attachment 582625 [details] [diff] [review] Part 1 (WIP): nsISmsService for gonk, plumbing for outgoing SMS Review of attachment 582625 [details] [diff] [review]: ----------------------------------------------------------------- The original idea was to have SmsService being implemented in JS. Why did you do it in C++? f- because of that. ::: dom/sms/src/Makefile.in @@ +88,5 @@ > # Add VPATH to LOCAL_INCLUDES so we are going to include the correct backend > # subdirectory (and the ipc one). > LOCAL_INCLUDES += $(VPATH:%=-I%) > > +ifdef MOZ_B2G_RIL #{ Could you remove this #{ @@ +93,5 @@ > +LOCAL_INCLUDES += \ > + -I$(topsrcdir)/dom/telephony \ > + -I$(topsrcdir)/dom/system/b2g \ > + $(NULL) > +endif #} and #}

Comment on attachment 582626 [details] [diff] [review] Part 2 (WIP): plumbing for incoming SMS Review of attachment 582626 [details] [diff] [review]: ----------------------------------------------------------------- I would prefer to use the C++ implementation and find a way to create it and initialize it from JS. Having a JS implementation and a C++ implementation might increase the chances of error regarding the DOM behavior and it will also make things really hard for IPC: right now, IPC code is assuming nsIDOMMozSmsMessage are castable in SmsMessage and those objects have a mData variable which is sent through processes. Though, I never had to handle that kind of situations so I don't know how hard it might be.

CCing some DOM folks who might have ideas of how to handle the problem from comment 5.

Two implementations of something? Sounds error prone. Why couldn't you just add contract id for SmsMessage, and add some setters (if it doesn't have those already). Or perhaps there could be some service which creates nsIDOMMozSmsMessage object. The method which creates them could take all the necessary parameters for initialization. (Does something use C++ casting for nsIDOMMozSmsMessage objects? I sure hope nsIDOMMozSmsMessage is then marked as builtin interface.)

Thanks for the feedback, everybody. (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4) > Comment on attachment 582625 [details] [diff] [review] > The original idea was to have SmsService being implemented in JS. Why did > you do it in C++? Yeah, I originally wanted to do it in JS, too. But as far as I could tell, that would've gotten pretty complicated with the XPCOM component registration, particularly given how SmsServiceFactory does the conditional registration depending on IPC scenarios. So I could have nsTelephonyWorker register itself as an SmsService, but because we want want the creation of that object to managed by RadioManager, not anybody else, we can't really do that. So we'd need a separate object than implements nsISmsService and then defers to nsTelephonyWorker. That could be in JS, or could just be in C++ because that's simpler at this point. Do you agree? (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5) > I would prefer to use the C++ implementation and find a way to create it and > initialize it from JS. Yes, as I mentioned on IRC, I would prefer that too. I had to board a plane quickly after attaching this patch, otherwise I would have gone into more detail in the description ;). As I said, this is a quickly hacked up WIP. I will look into giving nsDOMMozSmsMessage a constructor that we can call from XPCOM land. (In reply to Olli Pettay [:smaug] from comment #7) > Two implementations of something? Sounds error prone. Well, one of the original ideas of XPCOM was pluggability right? Particularly in this case, it's all about supporting the same API for different backends, so *somewhere* we've got to have separate implementations of something. So in general, I wouldn't agree with your statement, but in this specific case (mostly due to IPC reasons), we should definitely reuse the nsDOMMozSmsMessage > (Does something use C++ casting for nsIDOMMozSmsMessage objects? > I sure hope nsIDOMMozSmsMessage is then marked as builtin interface.) Yeah, that would definitely make the intent clearer.

This makes SmsMessage instantiable from JS. The IPC plumbing kinda expects that all nsIDOMMozSmsMessage objects are in fact the C++ implementation, because it grabs the SmsMessageData object and sends that over the pipe. So I've also marked nsIDOMMozSmsMessage as [builtinclass]. The "setData" method is in a separate nsISmsMessage interface. Sadly this has to be exposed in the class info (so it's exposed to content, too), because of how the DOM scriptable flags are set (see bug 712199 for details). To make sure content can't manipulate SmsMessages, we check that the caller does in fact have chrome privileges. (This should need a test, which I haven't added yet. I was hoping that mounir's patches which haven't landed yet would contain tests for SmsMessage, so that I could just extend those.)

This implements the nsISmsService as a shallow wrapper around our RIL (currently called nsITelephone, but that may change (bug 711671).

This now uses the SmsMessage constructor from part 0 instead of a custom JS implementation of nsIDOMMozSmsMessage. This does not complete SMS support for B2G, of course. I'd like to attack anything else, including the TODO comments, but especially the database stuff, in follow-up bugs.

Comment on attachment 583317 [details] [diff] [review] Part 0 (v1): Make SmsMessage instantiable from JS Review of attachment 583317 [details] [diff] [review]: ----------------------------------------------------------------- Maybe you could add the test to dom/sms/tests/ ? BTW, if there is no SmsMessage tests currently that's because it's not possible to create those objects from the DOM and I only wrote mochitests (so with content privileges). ::: dom/base/nsDOMClassInfo.cpp @@ +3934,1 @@ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozSmsMessage) I would prefer the other way (keeping the interfaces that are more relevant on top). ::: dom/sms/interfaces/nsIDOMSmsMessage.idl @@ +54,5 @@ > +[scriptable, builtinclass, uuid(48e4e252-253b-429f-adbc-be5068d8316f)] > +interface nsISmsMessage : nsIDOMMozSmsMessage > +{ > + [implicit_jscontext] > + void setData(in long id, If |setData| fully visible from the content, I would just add this to nsIDOMMozSmsMessage... @@ +65,5 @@ > + > +%{C++ > +// b70e870c-3ccb-4c9f-86e2-c15e6c8b017c > +#define NS_SMSMESSAGE_CID {0x0ebaef90, 0xa485, 0x44c4, {0x98, 0x1d, 0xf4, 0xd4, 0x3e, 0x91, 0x8f, 0xc7}} > +#define SMSMESSAGE_CONTRACTID "@mozilla.org/sms/smsmessage;1" Why is that needed? ::: dom/sms/src/SmsMessage.cpp @@ +90,5 @@ > + NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE); > + > + DeliveryState delivery; > + if (aDelivery.Equals(NS_LITERAL_STRING("received"))) { > + delivery = eDeliveryState_Received; You should just set mData.delivery() instead of creating this DeliveryState object. Also, I would have used a switch here it seems appropriate with an enum. @@ +97,5 @@ > + } else { > + return NS_ERROR_INVALID_ARG; > + } > + > + // We support both a Date object and a millisecond timestamp as a number. Why? If that comes from JS, we should only take a Date object. @@ +98,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + // We support both a Date object and a millisecond timestamp as a number. > + PRUint64 ts; Just set mData.timestamp() instead of using this variable. @@ +99,5 @@ > + } > + > + // We support both a Date object and a millisecond timestamp as a number. > + PRUint64 ts; > + if (JSVAL_IS_OBJECT(aTimestamp)) { I think nowadays you should use stuff like: aTimestamp.isObject() What follow should be changed too. @@ +115,5 @@ > + } > + > + SmsMessageData data(aId, delivery, nsString(aSender), nsString(aReceiver), > + nsString(aBody), ts); > + mData = data; mData.id() = ...; mData.sender() = ...; mData.receiver() = ...; ::: dom/sms/src/SmsMessage.h @@ +55,3 @@ > > + // Constructor > + SmsMessage(); Why do you need to make the ctor public? Is that explained in a following patch? (and "// Constructor" isn't useful)

Comment on attachment 583317 [details] [diff] [review] Part 0 (v1): Make SmsMessage instantiable from JS Review of attachment 583317 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/src/SmsMessage.cpp @@ +90,5 @@ > + NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE); > + > + DeliveryState delivery; > + if (aDelivery.Equals(NS_LITERAL_STRING("received"))) { > + delivery = eDeliveryState_Received; Ousp, those are strings... Though, you should use DELIVERY_RECEIVED and DELIVERY_SENT from dom/sms/src/Constants.h

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12) > Maybe you could add the test to dom/sms/tests/ ? Sure, I don't care where it goes (though I find the inconsistency in dom/ a bit lame). But then we need to create tests/browser and tests/unit. > ::: dom/sms/interfaces/nsIDOMSmsMessage.idl > @@ +54,5 @@ > > +[scriptable, builtinclass, uuid(48e4e252-253b-429f-adbc-be5068d8316f)] > > +interface nsISmsMessage : nsIDOMMozSmsMessage > > +{ > > + [implicit_jscontext] > > + void setData(in long id, > > If |setData| fully visible from the content, I would just add this to > nsIDOMMozSmsMessage... Well, this is a matter of taste I guess. I don't really care so much, but I do think there's value in keeping nsIDOMMozSmsMessage pure. This also seemed to be what bent and others were preferring when I talked to them about this stuff. > > +%{C++ > > +// b70e870c-3ccb-4c9f-86e2-c15e6c8b017c > > +#define NS_SMSMESSAGE_CID {0x0ebaef90, 0xa485, 0x44c4, {0x98, 0x1d, 0xf4, 0xd4, 0x3e, 0x91, 0x8f, 0xc7}} > > +#define SMSMESSAGE_CONTRACTID "@mozilla.org/sms/smsmessage;1" > > Why is that needed? See layout/build/nsLayoutModule.cpp > ::: dom/sms/src/SmsMessage.cpp > @@ +90,5 @@ > > + NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE); > > + > > + DeliveryState delivery; > > + if (aDelivery.Equals(NS_LITERAL_STRING("received"))) { > > + delivery = eDeliveryState_Received; > > You should just set mData.delivery() instead of creating this DeliveryState > object. Also, I would have used a switch here it seems appropriate with an > enum. Pretty sure I can't use a switch statement for strings ;). So I'm a bit confused how I can just assign to mData.delivery(). Does it return a pointer? so *mData.delivery() = eDeliveryState_Received; ? Your examples don't dereference the pointer, so I'm guessing something else is going on (as you can see, C++ is not my forte.) > > @@ +97,5 @@ > > + } else { > > + return NS_ERROR_INVALID_ARG; > > + } > > + > > + // We support both a Date object and a millisecond timestamp as a number. > > Why? If that comes from JS, we should only take a Date object. Except the RIL (nsTelephonyWorker) just has a timestamp number because it gets it from the RIL worker via postMessage (and that doesn't do Date objects). It seems silly having to create a Date object in the RIL just for it to be deconstructed again to a timestamp. (<insert Andreas here about object allocations>) > @@ +99,5 @@ > > + } > > + > > + // We support both a Date object and a millisecond timestamp as a number. > > + PRUint64 ts; > > + if (JSVAL_IS_OBJECT(aTimestamp)) { > > I think nowadays you should use stuff like: > aTimestamp.isObject() Um, ok. Is that preferred? Who can I ask about this? Maybe smaug will tell me in his review :) > ::: dom/sms/src/SmsMessage.h > @@ +55,3 @@ > > > > + // Constructor > > + SmsMessage(); > > Why do you need to make the ctor public? NS_GENERIC_FACTORY_CONSTRUCTOR needs it to be, it seems. > Is that explained in a following patch? (and "// Constructor" isn't useful) Uh yeah, I don't even remember typing that comment. It shall be gone!

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #13) > Though, you should use DELIVERY_RECEIVED and DELIVERY_SENT from > dom/sms/src/Constants.h Good point! Will do, thanks!

Comment on attachment 583319 [details] [diff] [review] Part 1 (v1): nsISmsService for the RIL, plumbing for outgoing SMS Review of attachment 583319 [details] [diff] [review]: ----------------------------------------------------------------- I'm still sad that SmsService is implemented in C++. I don't see how the registration makes thing harder but I also don't know anything about how to deal with RadioManager and nsITelephonyWorker so I will just admit this is the easiest solution... ::: dom/sms/src/ril/SmsService.cpp @@ +48,5 @@ > + > +SmsService::SmsService() > +{ > + nsCOMPtr<nsITelephone> inst = RadioManager::GetTelephone(); > + if (NULL != inst) { NULL -> nsnull But didn't you meant the opposite? I might be a bit tired but for what I read, this is equivalent to mRil = nsnull... I think you want: if (!inst) { return; } mRil = inst; Actually, you could also just do: mRil = RadioManager::GetTelephone(); ::: dom/sms/src/ril/SmsService.h @@ +18,5 @@ > + * Portions created by the Initial Developer are Copyright (C) 2011 > + * the Initial Developer. All Rights Reserved. > + * > + * Contributor(s): > + * Mounir Lamouri <mounir.lamouri@mozilla.com> (Original Author) I have nothing to do with that file :)

Comment on attachment 583321 [details] [diff] [review] Part 2 (v1): plumbing for incoming SMS Review of attachment 583321 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/nsTelephonyWorker.js @@ +228,5 @@ > }, > > + handleSmsReceived: function handleSmsReceived(message) { > + //TODO: put the sms into a database, assign it a proper id, yada yada > + let sms = newSmsMessage(null, 0 or -1 seems a better value. Though, null might be changed in 0 automatically given that it's expecting an integer.

We discussed this on IRC last night and smaug brilliantly suggested that instead of adding a method to SmsMessage that we don't really want to expose to content, we just expose the functionality on some other XPCOM component. SmsService seemed like the best solution, even though every backend has to re-implement this function. To that end, we decided to make a central helper, so that the burden of each backend is minimal. This now implements all that, extending nsISmsService and all existing implementations, and providing the helper as a static method on SmsMessage.

Address mounir's feedback. Thanks, mounir!

Same as v1, just uses the new way of instantiating SmsMessages as implemented in part 0.

Comment on attachment 583644 [details] [diff] [review] Part 0 (v2): Create SmsMessages from JS >+SmsMessage::Create(PRInt32 aId, >+ const nsAString& aDelivery, >+ const nsAString& aSender, >+ const nsAString& aReceiver, >+ const nsAString& aBody, >+ const jsval& aTimestamp, >+ JSContext* cx, aCx >+ nsIDOMMozSmsMessage** aMessage) >+{ >+ *aMessage = nsnull; >+ >+ SmsMessageData data; >+ data.id() = aId; >+ data.sender() = nsString(aSender); >+ data.receiver() = nsString(aReceiver); >+ data.body() = nsString(aBody); Could you add a comment that you're assigning to a reference. >+SmsService::CreateSmsMessage(PRInt32 aId, >+ const nsAString& aDelivery, >+ const nsAString& aSender, >+ const nsAString& aReceiver, >+ const nsAString& aBody, >+ const jsval& aTimestamp, >+ JSContext* cx, aCx >+SmsService::CreateSmsMessage(PRInt32 aId, >+ const nsAString& aDelivery, >+ const nsAString& aSender, >+ const nsAString& aReceiver, >+ const nsAString& aBody, >+ const jsval& aTimestamp, >+ JSContext* cx, aCx >+SmsIPCService::CreateSmsMessage(PRInt32 aId, >+ const nsAString& aDelivery, >+ const nsAString& aSender, >+ const nsAString& aReceiver, >+ const nsAString& aBody, >+ const jsval& aTimestamp, >+ JSContext* cx, ditto

Comment on attachment 583645 [details] [diff] [review] Part 1 (v2): nsISmsService for the RIL, plumbing for outgoing SMS >+SmsService::CreateSmsMessage(PRInt32 aId, >+ const nsAString& aDelivery, >+ const nsAString& aSender, >+ const nsAString& aReceiver, >+ const nsAString& aBody, >+ const jsval& aTimestamp, >+ JSContext* cx, aCx >-[scriptable, uuid(f6baa721-665e-403e-8a98-acaa0d8bf267)] >-interface nsITelephone : nsISupports { >+[scriptable, uuid(5be6e41d-3aee-4f5c-8284-95cf529dd6fe)] >+interface nsITelephone : nsISupports >+{ >+ readonly attribute jsval currentState; >+ void registerCallback(in nsITelephoneCallback callback); >+ void unregisterCallback(in nsITelephoneCallback callback); >+ getNumberOfMessagesForText: function getNumberOfMessagesForText(text) { >+ //TODO: this assumes 7bit encoding, which is incorrect. Need to look >+ // for characters not supported by 7bit alphabets and then calculate >+ // length in UCS2 encoding. Followup bug filed? Mounir should review this stuff.

Comment on attachment 583646 [details] [diff] [review] Part 2 (v2): Plumbing for incoming SMS Again, mounir should review this too.

(In reply to Olli Pettay [:smaug] from comment #22) > >+ getNumberOfMessagesForText: function getNumberOfMessagesForText(text) { > >+ //TODO: this assumes 7bit encoding, which is incorrect. Need to look > >+ // for characters not supported by 7bit alphabets and then calculate > >+ // length in UCS2 encoding. > Followup bug filed? Just did: bug 712804 > Mounir should review this stuff. He did review the earlier version (see comment 16 and 17)

Comment on attachment 583644 [details] [diff] [review] Part 0 (v2): Create SmsMessages from JS Review of attachment 583644 [details] [diff] [review]: ----------------------------------------------------------------- Having to define CreateMessage in all SmsService implementation is kind of sad but we will live with it for the moment. r=me with comments below fixed. ::: dom/sms/interfaces/nsISmsService.idl @@ +35,5 @@ > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsISupports.idl" > +interface nsIDOMMozSmsMessage; New line before this please :) @@ +48,5 @@ > { > boolean hasSupport(); > unsigned short getNumberOfMessagesForText(in DOMString text); > void send(in DOMString number, in DOMString message); > + [implicit_jscontext] ditto ::: dom/sms/src/SmsMessage.cpp @@ +37,5 @@ > * ***** END LICENSE BLOCK ***** */ > > #include "SmsMessage.h" > #include "nsIDOMClassInfo.h" > +#include "jsdate.h" // For js_DateGetMsecSinceEpoch Please keep #include "jsapi.h". @@ +60,5 @@ > : mData(aData) > { > } > > +nsresult /* static */ nsresult @@ +67,5 @@ > + const nsAString& aSender, > + const nsAString& aReceiver, > + const nsAString& aBody, > + const jsval& aTimestamp, > + JSContext* cx, aCx, as Olli said. @@ +87,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + // We support both a Date object and a millisecond timestamp as a number. > + if (JSVAL_IS_OBJECT(aTimestamp)) { aTimestamp.isObject() @@ +89,5 @@ > + > + // We support both a Date object and a millisecond timestamp as a number. > + if (JSVAL_IS_OBJECT(aTimestamp)) { > + JSObject* obj = JSVAL_TO_OBJECT(aTimestamp); > + NS_ENSURE_ARG(obj); JSObject& obj = aTimestamp.toObject(); @@ +90,5 @@ > + // We support both a Date object and a millisecond timestamp as a number. > + if (JSVAL_IS_OBJECT(aTimestamp)) { > + JSObject* obj = JSVAL_TO_OBJECT(aTimestamp); > + NS_ENSURE_ARG(obj); > + NS_ENSURE_ARG(JS_ObjectIsDate(cx, obj)); Please use NS_ENSURE_TRUE(..., NS_ERROR_INVALID_ARG) @@ +93,5 @@ > + NS_ENSURE_ARG(obj); > + NS_ENSURE_ARG(JS_ObjectIsDate(cx, obj)); > + data.timestamp() = js_DateGetMsecSinceEpoch(cx, obj); > + } else { > + NS_ENSURE_ARG(JSVAL_IS_NUMBER(aTimestamp)); NS_ENSURE_TRUE() or even: if (!...) { return ...; } and I think you want: aTimestamp.isNumber() @@ +98,5 @@ > + jsdouble number; > + JSBool rc = JS_ValueToNumber(cx, aTimestamp, &number); > + NS_ENSURE_TRUE(rc, NS_ERROR_UNEXPECTED); > + data.timestamp() = PRUint64(number); > + NS_ENSURE_ARG(data.timestamp() == number); The last 5 lines should be: jsdouble number = aTimestamp.toNumber(); NS_ENSURE_TRUE(static_cast<PRUint64>(number) == number, NS_ERROR_INVALID_ARG); data.timestamp() = static_cast<PRUint64>(number); ::: dom/sms/src/SmsMessage.h @@ +40,5 @@ > > #include "mozilla/dom/sms/PSms.h" > #include "nsIDOMSmsMessage.h" > #include "nsString.h" > +#include "jsapi.h" Please include jspubtd.h instead. @@ +59,5 @@ > + const nsAString& aDelivery, > + const nsAString& aSender, > + const nsAString& aReceiver, > + const nsAString& aBody, > + const jsval& aTimestamp, Use JS::Value instead of jsval. It's weird but that prevents including jsapi.h in the header. You don't have to change the cpp file. ::: dom/tests/unit/xpcshell.ini @@ +7,5 @@ > [test_domstorage_aboutpages.js] > [test_geolocation_provider.js] > # Bug 684962: test hangs consistently on Android > skip-if = os == "android" > +[test_smsservice_createsmsmessage.js] Could you move this test to dom/sms/tests ?

Comment on attachment 583645 [details] [diff] [review] Part 1 (v2): nsISmsService for the RIL, plumbing for outgoing SMS Review of attachment 583645 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/telephony/nsITelephone.idl @@ +71,5 @@ > + /** > + * SMS-related functionality. > + */ > + unsigned short getNumberOfMessagesForText(in DOMString text); > + void sendSMS(in DOMString number, in DOMString message); Why are you adding those methods to this API? Is this API "public" like part of WebTelephone APIs?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #26) > ::: dom/telephony/nsITelephone.idl > @@ +71,5 @@ > > + /** > > + * SMS-related functionality. > > + */ > > + unsigned short getNumberOfMessagesForText(in DOMString text); > > + void sendSMS(in DOMString number, in DOMString message); > > Why are you adding those methods to this API? Is this API "public" like part > of WebTelephone APIs? nsITelephone (it's a bit of a misnomer, we want to rename it to nsIRadioInterfaceLayer, see bug 711671) represents everything the radio can do in an XPCOM API. The WebTelephony and WebSMS DOM glue then talk to nsITelephone to do the actual work, and they can register themselves to listen for events coming from the radio. So, in a way, yes, it's the gecko-public API of the RIL.

smaug's and mounir's review comments addressed.

smaug's review comments addressed. This is good to go now, just needs the appropriate RIL pieces to come through.

https://hg.mozilla.org/integration/mozilla-inbound/rev/49b65b70d75f https://hg.mozilla.org/integration/mozilla-inbound/rev/35a734c5f8c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/02095c35b292

https://hg.mozilla.org/mozilla-central/rev/49b65b70d75f https://hg.mozilla.org/mozilla-central/rev/35a734c5f8c3 https://hg.mozilla.org/mozilla-central/rev/02095c35b292