Goals - allow content to send SMS messages - allow content to receive SMS messages This overlaps with sms: URIs and the W3C Messaging work (http://www.w3.org/TR/messaging-api/). Neither is enough because we need to receive SMS's. Personally, I don't see much use in generalizing from SMS to Messaging (which is so general to be almost meaningless), but it might be worth reviewing the w3c's rationale for doing so.

Those patches let a web page send a SMS and get informed when a SMS is received. We still need to add a few functionalities, like (not exhaustive list): - return a SmsRequest object when |send| is called. This object would receive a 'success' or 'error' event depending on the SMS being sent or not; - 'smssent' event; - 'smsdelivered' event; - create a SmsMessage object that would be used for events; - API to read current SMS's.

So, now, when 'smsreceived' event is sent, |event.message| will contain a SmsMessage object with |.sender| being the number who sent the SMS and |.body| being the text message. Basically, that means, on Android, with the current API, you can simulate the stock app notification using smsreceived and DesktopNotification :) What's next: 1. Add two other events: smssent and smsdelivered; 2. SmsRequest object returned when |send| is called (maybe suggest?); 3. API to read current SMS's; x. Finalizing the API's...

Huzzah! Awesome work :).

This event is sent when a SMS is sent. If the SMS is a multi-part SMS (more than 160 characters) an event is sent when all parts are actually sent. The error handling (SMS sending failing) will be handled later. The idea is that smssent is a 'public' event while sending error will only send an event to the SmsRequest object that is going to be returned by send().

After a fight with Android API's, I've been able to find a way to not have a new PendingIntents erasing the previous one when they should not. I really don't understand why the API behave that way: if you create a PendingIntent with an Intent, it will try to find an intent with a common set of properties (like action) in a "pool". Also, I tried limiting the intent to GeckoSmsManager but it didn't work. Though, we don't care that much.

Now, you can do things like: var r = navigator.mozSms.send("0000000", "message"); r.onSuccess = function() { alert(r.message + " delivered!"); } r.onError = function() { alert("error :("); } This patch doesn't implement different error codes.

delete method can take a 'long' representing the message UUID or a SmsMessage object. Taking a long prevent the applications from storing the message objects to delete them. Taking a message object make sure the obvious signature exist. The method throws when it is failing. I think it's better than using a SmsRequest because no-one might be really interesting in knowing if the method failed (it should not fail, really).

Example usage of send/recv. I'm getting a crash on the send notification, investigating now.

Also: this is working great. Hot!

Hope this is useful.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #18) > Created attachment 559360 [details] > Example page > > Example usage of send/recv. I'm getting a crash on the send notification, > investigating now. Indeed, there is a crash here as I said during the last WebAPI call. My plan was to investigate it after finishing to implement the get method. Though, I guess you might have fixed it. Thanks a lot :) By the way, these patches might be hard to apply. Did you use my user repo? If someone else wants to try those patches, my user repo URL is: https://hg.mozilla.org/users/mlamouri_mozilla.com/webapi-patchqueue/ Here, you will find the updated patches that should apply easily with each other. Though, they might not apply really easily on an updated trunk.

They apply on top of rev 482742e6fff7. (BTW, when you rebase, it's not a bad idea to note the new base rev in the commit message.)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22) > They apply on top of rev 482742e6fff7. (BTW, when you rebase, it's not a > bad idea to note the new base rev in the commit message.) TBH, I wasn't expecting someone to use my user repo someday :) I will try to do that now.

No worries. I've found it useful in the past just for myself, even when others weren't using my user repo, when I switch around between different machines or repos.

This is actually the main reason why I have user repositories for my patches. The fact that anyone can get my work to test it is actually I side effect I'm very happy with :) Anyway, this is a digression :)

Mounir/Andreas: I'm running into a problem when I run this line var r = navigator.mozSms.send(num, msg); in the "htmlrunner" [1]. I get TypeError: navigator.mozSms is undefined This works in Firefox-on-android in both in- and out-of-process tabs, and for both http:// and file:// URIs. After fighting with git/hg interaction for a while, I'm pretty sure the rollup patch is applied correctly. I also tried - using a <browser> instead of <iframe> in the htmlrunner XUL - using remote=true/false in both (in- and out-of-process) to no avail. I set up some logging in nsGlobalWindow.cpp for the navigator and mozSms attrs, and I see I/GeckoSMS( 2824): nsGlobalWindow::GetNavigator I/GeckoSMS( 2824): nsGlobalWindow::GetNavigator I/GeckoSMS( 2824): createdNavigator I/GeckoSMS( 2824): OK! E/SurfaceFlinger( 2722): GL error 0x0506 that is, we don't even get to GetMozSms() (I guess if that returned an nsnull outparam, that would appear to JS as null?). I finally got gdb working in (content processes) with b2g (UI process crashes gdb). I poked around for a bit, but I'm not sure where to be looking. Any leads? [1] https://github.com/cgjones/mozilla-central/blob/master/b2g/chrome/content/browser.xul

Fabrice straightened me out wrt comment 26. Working in b2g now!

Mounir: BTW, I wasn't able to get the SmsRequest callbacks to work with the code in attachment 559360 [details]; they never fired. I might be doing something wrong, but just a heads up (I copy-n-pasted from comment 14 ;).

Mounir, what work is needed before we can land these patches on m-c?

(In reply to Chris Jones [:cjones] [:warhammer] from comment #29) > Mounir, what work is needed before we can land these patches on m-c? The patches by themselves should be cleaned, fixed (there are still some crashes) and some new features should be added (like SmsMessage.unread). Nothing major. However, we need some tests and we need to figure out how we can handle that for the Android code.

need full review

One issue I remember coming up is whether Gecko should explicitly save text messages into the system database on android. I think on android, the right thing to do is let the OS save the messages. We'll want a followup bug where we implement this for gonk, and there we'll want two parts: a low-level layer to send and receive RIL messages related to SMS, and another piece that stores the messages in an IndexedDB, probably. (Later to be Sync'd, maybe.)

This is actually a tricky issue. On Android, there is an intent broadcasted when a message is received. It seems that the stock sms app uses this intent to save the sms in its internal DB. If we use this intent to read the saved values. we are facing a race condition. If we wait for x milliseconds before doing so, we are still facing a race condition. I will try to see if there is a simple way to fix that (like having a callback when the DB is changed). Note that we could alternatively not try to look at the saved message but we would miss some information (or have wrong ones) like id or timestamp. In another hand, I don't believe we really want to have WebSMS enabled in a shipped browser for the moment because it would require some privileges that might freak out users. We should probably discuss that with the mobile team. With that in mind, we could just not read the DB and have wrong information when receiving a message on Android.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #33) > This is actually a tricky issue. On Android, there is an intent broadcasted > when a message is received. It seems that the stock sms app uses this intent > to save the sms in its internal DB. If we use this intent to read the saved > values. we are facing a race condition. If we wait for x milliseconds before > doing so, we are still facing a race condition. I will try to see if there > is a simple way to fix that (like having a callback when the DB is changed). > Note that we could alternatively not try to look at the saved message but we > would miss some information (or have wrong ones) like id or timestamp. At which point do sent and received SMS's get assigned IDs? Is it when they're saved in the OS database, or sometime before? I wonder if we could temporarily keep around the messages in a "limbo" until we see them come back from a DB query, and automatically add the limbo messages to the DB query results returned to script until we see them from the system DB. That would require being able to uniquely identify the sent/received messages though.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #34) > At which point do sent and received SMS's get assigned IDs? Is it when > they're saved in the OS database, or sometime before? We are using the id in the database. > I wonder if we could temporarily keep around the messages in a "limbo" until > we see them come back from a DB query, and automatically add the limbo > messages to the DB query results returned to script until we see them from > the system DB. That would require being able to uniquely identify the > sent/received messages though. Unfortunately, we can't do that with a 100% certainty that we will got the correct message because we only have the sender and the body that are sure information and we can't guarantee that someone didn't sent you twice the same message. We can try to put a lower range for the date but again, we can't guarantee that someone didn't sent you twice the same message in a very short interval of time. Again, given that we might never ship WebSMS for Android, something that works 99% of the time on Android would be fine by me.

What is blocking this from landing? Can we please land this Monday?

(In reply to Andreas Gal :gal from comment #36) > What is blocking this from landing? I've been working on Battery API but most of it has landed now and the last patches are waiting for reviews. I was trying to get Battery API done for Firefox 10. I did what I had to, now WebSMS is my top priority. > Can we please land this Monday? I'm now working on WebSMS and trying to use a better (and final) code design. Patches will come soon. Do you want those WIP patches to land ASAP or do you prefer to wait for the new patches? I don't know how long it's gonna take though. I would have say around a week but with MozCamp Europe coming (and a few days off next week), it might take longer. Also, note that landing the WIP patches is going to require some time: re-basing them to tip and put a big --enable-websms around them. Ideally, I would prefer not to do that given that it would last in the tree for a very short period of time.

Aright let's wait for your new patch but let's really drive that in. We are critically dependent on this.

As said in the code comments, the *temporary* security model is going to have two parts: 1. dom.sms.enabled preference that is going to be set to false by default. This is going to stay even when we will have a final security model to simply disable the feature. 2. dom.sms.whitelist preference that is going to be set to the empty string by default. This is a comma separated list of prepath (mostly hostnames) that are allowed to use the WebSMS api.

Jonas, I'm asking a sr here because our draft spec say we should return a unsigned short but I wonder if we should really care about the type and simply return an unsigned long like most specifications seem to do.

Comment on attachment 572876 [details] [diff] [review] Part G - Add PSms IPDL subprotocol You can combine the *Child/Parent.h headers into a single RemoteSms.h or something, if you don't really need to include them separately. But I'll leave that to your taste. Nit: we've been trying to avoid namespaces deeper than 2 elements ("mozilla::foo"), so unless you have a specific requirement for a mozilla::dom::sms namespace, please stick with mozilla::dom.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #47) > Nit: we've been trying to avoid namespaces deeper than 2 elements > ("mozilla::foo"), so unless you have a specific requirement for a > mozilla::dom::sms namespace, please stick with mozilla::dom. workers, indexeddb, and telephony all are subnamespaces of mozilla::dom.

Case in point :). What did all those extra characters buy you?

Comment on attachment 572881 [details] [diff] [review] Part I - Implement mozSms.getNumberOfMessagesForText >diff --git a/dom/sms/src/android/SmsService.cpp b/dom/sms/src/android/SmsService.cpp >+/* static */ PSmsChild* >+SmsService::GetSmsChild() >+{ >+ if (!sSmsChild) { >+ sSmsChild = ContentChild::GetSingleton()->SendPSmsConstructor(); >+ } >+ return sSmsChild; >+} >+ The IPC implementation shouldn't be in android-specific code. We're going to need to share this among all platforms with SMS capability. Instead, as painful as it may be, I think you'll want to have an "IPC SMS service" that's created magically in content processes when you GetService(). > parent: >+ sync GetNumberOfMessagesForText(nsString aText) >+ returns (PRUint16 aNumber); This is a question of the DOM API, but shouldn't this be asynchronous? I would have thought this required a DB query, so disk IO.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #50) > The IPC implementation shouldn't be in android-specific code. We're > going to need to share this among all platforms with SMS capability. > Instead, as painful as it may be, I think you'll want to have an "IPC > SMS service" that's created magically in content processes when you > GetService(). Moreover, we want to have a native implementation for gonk, right? So it might make more sense to implement this as an XPCOM service and make navigator.mozSms defer to that. AFAICT from patch B and C, this does not allow for a JS implementation right now. (There's a lot of patches here... It seems like some of the WIPs can be obsoleted, no?) > > parent: > >+ sync GetNumberOfMessagesForText(nsString aText) > >+ returns (PRUint16 aNumber); > > This is a question of the DOM API, but shouldn't this be asynchronous? > I would have thought this required a DB query, so disk IO. Yes, from an implementor's point of view, this should be async. In fact, I can't think of a single method revolving around SMS that shouldn't be async. On a general note, where's the navigator.sms API being spec'ed out? The request model suggested by cjones's example page is akin to IndexedDB, but both the W3C's and WAC's API drafts seem to be going with directly passing in success and failure callbacks.

(In reply to Philipp von Weitershausen [:philikon] from comment #51) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #50) > > The IPC implementation shouldn't be in android-specific code. We're > > going to need to share this among all platforms with SMS capability. > > Instead, as painful as it may be, I think you'll want to have an "IPC > > SMS service" that's created magically in content processes when you > > GetService(). > > Moreover, we want to have a native implementation for gonk, right? So it > might make more sense to implement this as an XPCOM service and make > navigator.mozSms defer to that. AFAICT from patch B and C, this does not > allow for a JS implementation right now. > Yep, that's the idea. B and C add an nsISmsManager that uses an nsISmsService, of which will have multiple implementations (including the native-JS one).

(In reply to Chris Jones [:cjones] [:warhammer] from comment #50) > > parent: > >+ sync GetNumberOfMessagesForText(nsString aText) > >+ returns (PRUint16 aNumber); > > This is a question of the DOM API, but shouldn't this be asynchronous? > I would have thought this required a DB query, so disk IO. There is no DB query here. The method returns how many messages would be needed to send a specific text. That's likely something like: |upper(aText.length() / 160)| but given that it's unlikely exactly that, I've decided to use the Android implementation. We could directly implement this in SmsManager later (when we will have a b2g-specific backend). (In reply to Philipp von Weitershausen [:philikon] from comment #51) > Moreover, we want to have a native implementation for gonk, right? So it > might make more sense to implement this as an XPCOM service and make > navigator.mozSms defer to that. AFAICT from patch B and C, this does not > allow for a JS implementation right now. The idea is to have the b2g implementation implementing SmsService. SmsManager should be left as is because it will handle common stuff. > (There's a lot of patches here... It seems like some of the WIPs can be > obsoleted, no?) Yes, will do that. > On a general note, where's the navigator.sms API being spec'ed out? The > request model suggested by cjones's example page is akin to IndexedDB, but > both the W3C's and WAC's API drafts seem to be going with directly passing > in success and failure callbacks. It's in the bug URL now. We are using an API similar to IndexedDB but getNumberOfMessagesForText doesn't have to be async.

This should be doing what you were asking. The only disadvantages is that asking for support is now an IPC call. It's the cleanest way to do that without #ifdef I think. I would tend to say it's no big deal but if someone has a simple and clean idea, I'm listening.

If you are interested in trying the prototype/poc/draft implementation, have a look at this patch queue: https://hg.mozilla.org/users/mlamouri_mozilla.com/webapi-patchqueue/

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #53) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #50) > > > parent: > > >+ sync GetNumberOfMessagesForText(nsString aText) > > >+ returns (PRUint16 aNumber); > > > > This is a question of the DOM API, but shouldn't this be asynchronous? > > I would have thought this required a DB query, so disk IO. > > There is no DB query here. The method returns how many messages would be > needed to send a specific text. That's likely something like: > |upper(aText.length() / 160)| but given that it's unlikely exactly that, > I've decided to use the Android implementation. We could directly implement > this in SmsManager later (when we will have a b2g-specific backend). > Yes, that might be better. It's OK for now.

Comment on attachment 573225 [details] [diff] [review] Part H - Add a IPC service to handle IPC calls Looks good. Same comment applies as before: you can group more of the files needed for the cross-process support into fewer, but if you're more comfortable with multiple separate files, OK.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #58) > Comment on attachment 573225 [details] [diff] [review] [diff] [details] [review] > Part H - Add a IPC service to handle IPC calls > > Looks good. Same comment applies as before: you can group more of the files > needed for the cross-process support into fewer, but if you're more > comfortable with multiple separate files, OK. I don't really like having one header declaring two classes and two cpp files for the implementation. I also prefer to avoid having one header and one cpp file for two classes. And I hate big files :)

Chris, do you think someone should review the files you superreviewed?

They're simple enough boilerplate that I'm comfortable with just my sr. But I'm not a DOM peer, so you might want to get a second opinion from one.

Comment on attachment 573306 [details] [diff] [review] Part J - Basic implementation of mozSms.send() >diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java >+ public static void send(String aNumber, String aMessage) { >+ /* >+ * TODO: >+ * This is a basic send method that doesn't handle errors, doesn't listen to >+ * sent and received messages. It's only calling the sent method. "send"

I did remove SmsMessage.unread from our proposal because it makes SmsMessage dynamic [1] which is making the implementation way harder. We will see in a second pass if we want to implement that. [1] if two apps get an SmsMessage with unread = false, one sets it to true, the other one should get an update.

This is not implementing the js ctor. It's probably not really hard to add that but it will affect all events AFAIUI so let's do that in a follow-up.

... I hate having so many patches in the same bug :(

Comment on attachment 573609 [details] [diff] [review] Part L - Implement SmsEvent >@@ -899,11 +899,13 @@ nsEventDispatcher::CreateEvent(nsPresCon > return NS_NewDOMCloseEvent(aDOMEvent, aPresContext, nsnull); > if (aEventType.LowerCaseEqualsLiteral("touchevent") && > nsDOMTouchEvent::PrefEnabled()) > return NS_NewDOMTouchEvent(aDOMEvent, aPresContext, nsnull); > if (aEventType.LowerCaseEqualsLiteral("hashchangeevent")) > return NS_NewDOMHashChangeEvent(aDOMEvent, aPresContext, nsnull); > if (aEventType.LowerCaseEqualsLiteral("customevent")) > return NS_NewDOMCustomEvent(aDOMEvent, aPresContext, nsnull); >+ if (aEventType.LowerCaseEqualsLiteral("smsevent")) >+ return NS_NewDOMSmsEvent(aDOMEvent, aPresContext, nsnull); mozsmsevent >+[scriptable, uuid(34dda4c3-4683-4323-9ee3-2a7bfef7df3b)] >+interface nsIDOMSmsEvent : nsIDOMEvent nsIDOMMozSmsEvent or something like that >+class SmsEvent : public nsIDOMSmsEvent >+ , public nsDOMEvent , should be in the previous line, IMO.

Comment on attachment 573608 [details] [diff] [review] Part K - Implement SmsMessage >+[scriptable, function, uuid(0a0037ba-585e-41f4-b0a5-1d0224353105)] >+interface nsIDOMSmsMessage : nsISupports nsIDOMMozSmsMessage or some such >+#include "jsdate.h" // For js_NewDateObjectMsec have you asked JS people if this is ok?

Comment on attachment 573612 [details] [diff] [review] Part M - Receiving SMS, IPC handling >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp > PSmsParent* > ContentParent::AllocPSms() > { >- return new SmsParent(); >+ SmsParent* smsParent = new SmsParent(); >+ smsParent->Init(); Since the Init() method is infallible, is there any point in having it separate from the ctor? >diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp >+IPCSmsMessage >+SmsMessage::ToIPCSmsMessage() const >+{ >+ return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender, >+ mReceiver, mBody, mTimestamp); >+} Instead of duplicating the definition of the IPDL struct for SmsMessage, it probably makes sense for SmsMessage to be a simple XPCOM wrapper around the struct defined in IPDL, plus whatever additional non-data state it needs. That keeps the definition in one place. >diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl >--- a/dom/sms/src/ipc/PSms.ipdl >+++ b/dom/sms/src/ipc/PSms.ipdl >@@ -38,19 +38,31 @@ > * ***** END LICENSE BLOCK ***** */ > > include protocol PContent; > > namespace mozilla { > namespace dom { > namespace sms { > >+struct IPCSmsMessage { I don't like this name but I understand the conflict with SmsMessage. Maybe SmsMessageData? >diff --git a/dom/sms/src/ipc/SmsChild.cpp b/dom/sms/src/ipc/SmsChild.cpp >+bool >+SmsChild::RecvNotifyReceivedMessage(const IPCSmsMessage& aMessage) >+{ >+ nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); |using namespace services;| or |services::GetObserverService()| for consistency with other uses, plz. >diff --git a/dom/sms/src/ipc/SmsParent.cpp b/dom/sms/src/ipc/SmsParent.cpp >--- a/dom/sms/src/ipc/SmsParent.cpp >+++ b/dom/sms/src/ipc/SmsParent.cpp >@@ -32,21 +32,69 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "SmsParent.h" > #include "nsISmsService.h" >+#include "nsIObserverService.h" >+#include "mozilla/Services.h" >+#include "Constants.h" >+#include "nsIDOMSmsMessage.h" >+#include "mozilla/unused.h" >+#include "SmsMessage.h" > > namespace mozilla { > namespace dom { > namespace sms { > >+NS_IMPL_THREADSAFE_ISUPPORTS1(SmsParent, nsIObserver) >+ Why does this need to be thread safe? That's pretty dangerous. Actors can only be used on thread they're created on.

Comment on attachment 573610 [details] [diff] [review] Part M - Receiving SMS, dom code >+[scriptable, function, uuid(807d593c-09cb-4aa3-afa5-aa0a671bd0d3)] >+interface nsIDOMSmsManager : nsIDOMEventTarget This should be nsIDOMMoz* or something similar >+void >+SmsManager::Shutdown() ... >+ // Those vars come from nsDOMEventTargetHelper. >+ mOwner = nsnull; >+ mScriptContext = nsnull; Why these? After that event handling could behave unexpectedly

I tried to find a simple way to know the current phone number but the Android SDK method doing that is a SIM card field that isn't always filed by operator (mine being one of those). There are other ways to find that number but they are quite complex so I will just ignore that field for the moment. I'm also not trying to find the real id of the message. It's not needed for a simple implementation and usage. I will do that in a follow-up I think (for the reasons given in the patch).

Comment on attachment 573682 [details] [diff] [review] Part O - Receiving SMS, Android backend >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java >+ IntentFilter smsFilter = new IntentFilter(); >+ smsFilter.addAction("android.provider.Telephony.SMS_RECEIVED"); >+ mSmsReceiver = new GeckoSmsManager(); >+ registerReceiver(mSmsReceiver, smsFilter); >+ Is there a reason you don't want to register/unregister based on when there are SMS listeners? I would prefer gecko not see all my SMSes until something I explicitly gave permission to registers to listen for them. >diff --git a/widget/src/android/AndroidJNI.cpp b/widget/src/android/AndroidJNI.cpp >+NS_EXPORT void JNICALL >+Java_org_mozilla_gecko_GeckoAppShell_notifySmsReceived(JNIEnv* jenv, jclass, >+ jstring aSender, >+ jstring aBody, >+ jlong aTimestamp) >+{ >+ if (!nsAppShell::gAppShell) { >+ return; >+ } >+ >+ nsCOMPtr<nsIDOMSmsMessage> message = >+ new SmsMessage(0, SmsMessage::eDeliveryState_Received, nsJNIString(aSender, jenv), >+ EmptyString(), nsJNIString(aBody, jenv), aTimestamp); >+ >+ nsAppShell::gAppShell->NotifyObservers(message, kSmsReceivedObserverTopic, nsnull); >+} Making nsIDOMSmsMessage thread safe makes me rather uncomfortable. AFAICT, you're doing it so that you can use this NotifyObservers() helper here and save a few lines of code. I'd rather not open that can of worms. I phoned a friend (gal) and he didn't like it either. Instead, let's use the SmsMessageData struct in PSms here: post a task to the gecko thread that includes the received SmsMessageData, and then NotifyObservers() from there. You can remove the NS_IMPL_THREADSAFE on SmsMessage then.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #71) > > PSmsParent* > > ContentParent::AllocPSms() > > { > >- return new SmsParent(); > >+ SmsParent* smsParent = new SmsParent(); > >+ smsParent->Init(); > > Since the Init() method is infallible, is there any point in having it > separate from the ctor? I prefer to have Init/Shutdown couples instead of ctor/Shutdown so I prefer to keep the Init method. > >diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp > > >+IPCSmsMessage > >+SmsMessage::ToIPCSmsMessage() const > >+{ > >+ return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender, > >+ mReceiver, mBody, mTimestamp); > >+} > > Instead of duplicating the definition of the IPDL struct for > SmsMessage, it probably makes sense for SmsMessage to be a simple > XPCOM wrapper around the struct defined in IPDL, plus whatever > additional non-data state it needs. That keeps the definition in one > place. I agree that keeping the definition in one place would be better but I really don't like keeping the definition in PSms.ipdl. The only reason to do that is to prevent writing the IPC struct reading/writing code and have it auto-generated. I agree it's practically a good reason but it seems to be a bad design IMO. I'm using this IPCSmsMessage as a compromise between writing the code to pass objects between processes and having SmsMessage data owned by PSMs.ipdl.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #74) > >diff --git a/embedding/android/GeckoApp.java b/embedding/android/GeckoApp.java > > >+ IntentFilter smsFilter = new IntentFilter(); > >+ smsFilter.addAction("android.provider.Telephony.SMS_RECEIVED"); > >+ mSmsReceiver = new GeckoSmsManager(); > >+ registerReceiver(mSmsReceiver, smsFilter); > >+ > > Is there a reason you don't want to register/unregister based on > when there are SMS listeners? I would prefer gecko not see all > my SMSes until something I explicitly gave permission to > registers to listen for them. In my opinion, doing that is going to make the code more complex: we are going to add addObserver and removeObserver methods and we will have to count observers to know when listening to SMS is needed. Perf-wise, doing that is not useful, and, on a B2G device (it's yet not sure this will be enabled in official Firefox Mobile release), there will be no reason to not listen to SMS given that we can assume there will be always one SMS observer. In addition, permissions are asked at the installation time on Android so users will not see any difference between Gecko always listening or listening when needed. I think we could consider this as a follow-up if it happens that the pros out-weight the cons but for the moment, that seems to be the opposite. > Making nsIDOMSmsMessage thread safe makes me rather > uncomfortable. AFAICT, you're doing it so that you can use this > NotifyObservers() helper here and save a few lines of code. I'd > rather not open that can of worms. I phoned a friend (gal) and > he didn't like it either. Instead, let's use the SmsMessageData > struct in PSms here: post a task to the gecko thread that > includes the received SmsMessageData, and then NotifyObservers() > from there. You can remove the NS_IMPL_THREADSAFE on SmsMessage > then. I asked Olli and he was comfortable with that. However, I don't really mind so I changed it to non-thread safe.

Comment on attachment 572870 [details] [diff] [review] Part B - Add .mozSms to navigator >+//***************************************************************************** >+// Navigator::nsIDOMNavigatorSms >+//***************************************************************************** >+ >+NS_IMETHODIMP >+Navigator::GetMozSms(nsIDOMSmsManager** aSmsManager) >+{ Should be nsIDOMMozSmsManager >+++ b/dom/base/nsDOMClassInfo.cpp >@@ -2288,16 +2288,17 @@ nsDOMClassInfo::Init() > DOM_CLASSINFO_MAP_BEGIN(Navigator, nsIDOMNavigator) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigator) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorGeolocation) > DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsIDOMNavigatorDesktopNotification, > Navigator::HasDesktopNotificationSupport()) > DOM_CLASSINFO_MAP_ENTRY(nsIDOMClientInformation) > DOM_CLASSINFO_MAP_CONDITIONAL_ENTRY(nsIDOMNavigatorBattery, > battery::BatteryManager::HasSupport()) >+ DOM_CLASSINFO_MAP_ENTRY(nsIDOMNavigatorSms) conditional? Or maybe that is added in some followup patch. >+[scriptable, function, uuid(562eade6-6dee-4c0a-bee9-bfc7f2bcadbe)] >+interface nsIDOMSmsManager : nsISupports nsIDOMMozSmsManager r+ with Moz prefix and assuming conditional is added in some patch.

Comment on attachment 572871 [details] [diff] [review] Part C - SmsManager stub You really like splitting patches to tiny parts.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #75) > > >diff --git a/dom/sms/src/SmsMessage.cpp b/dom/sms/src/SmsMessage.cpp > > > > >+IPCSmsMessage > > >+SmsMessage::ToIPCSmsMessage() const > > >+{ > > >+ return IPCSmsMessage(mId, mDelivery == eDeliveryState_Received, mSender, > > >+ mReceiver, mBody, mTimestamp); > > >+} > > > > Instead of duplicating the definition of the IPDL struct for > > SmsMessage, it probably makes sense for SmsMessage to be a simple > > XPCOM wrapper around the struct defined in IPDL, plus whatever > > additional non-data state it needs. That keeps the definition in one > > place. > > I agree that keeping the definition in one place would be better but I > really don't like keeping the definition in PSms.ipdl. The only reason to do > that is to prevent writing the IPC struct reading/writing code and have it > auto-generated. I agree it's practically a good reason but it seems to be a > bad design IMO. I don't quite follow this. Defining the structure in multiple places is a maintenance hell. I feel strongly about that; please don't do it. (Imagine what it would take to add a new field ...) The .ipdl happens to be the best place to do this because you need to send the sms info in IPC messages, and your definition will be auto-serialized. That's convenient, guarantees optimal performance, and avoids security issues. IPDL's datatypes are also more powerful than what can sanely be written in C++. > I'm using this IPCSmsMessage as a compromise between writing the code to > pass objects between processes and having SmsMessage data owned by PSMs.ipdl. I don't understand what you're compromising between. nsSmsMessage is in dom/sms, right, just like PSms?

Comment on attachment 572872 [details] [diff] [review] Part D - Temporary security model >+bool >+Navigator::IsSmsAllowed() const >+{ >+ static const bool defaultSmsPermission = false; >+ >+ // First of all, the general pref has to be turned on. >+ if (!Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)) { >+ return false; >+ } Can you pass const bool to GetBool? That is a bit surprising.

Comment on attachment 572876 [details] [diff] [review] Part G - Add PSms IPDL subprotocol Drop NS_OVERRIDE.

Comment on attachment 573294 [details] [diff] [review] Part I - Implement mozSms.getNumberOfMessagesForText >+SmsParent::RecvGetNumberOfMessagesForText(const nsString& aText, PRUint16* aResult) >+{ >+ nsCOMPtr<nsISmsService> smsService = do_GetService(SMSSERVICE_CONTRACTID); >+ NS_ENSURE_TRUE(smsService, true); Could you set *aResult = 0; before getting anything from the service. >+public class GeckoSmsManager >+{ >+ public static int getNumberOfMessagesForText(String aText) { >+ return SmsManager.getDefault().divideMessage(aText).size(); >+ } >+} I don't know the coding style for our Java code. In C++ { would be in the next line. >+AndroidBridge::GetNumberOfMessagesForText(const nsAString& aText) >+{ >+ ALOG_BRIDGE("AndroidBridge::GetNumberOfMessagesForText"); >+ >+ AutoLocalJNIFrame jniFrame; >+ jstring jText = GetJNIForThread()->NewString(PromiseFlatString(aText).get(), aText.Length()); >+ return GetJNIForThread()->CallStaticIntMethod(mGeckoAppShellClass, jNumberOfMessages, jText); >+} >+ I have no idea about JNI. Someone else should review this.

Comment on attachment 573294 [details] [diff] [review] Part I - Implement mozSms.getNumberOfMessagesForText Chris, could you review the Java/Android parts?

(In reply to Olli Pettay [:smaug] from comment #77) > Comment on attachment 572870 [details] [diff] [review] [diff] [details] [review] > Part B - Add .mozSms to navigator > [...] > r+ with Moz prefix and assuming conditional is added in some patch. If the pref is off or the site not whitelisted, mozSms returns null. I could use the dom.sms.enabled pref to conditionally add nsIDOMMozNavigatorSms to the Navigator object but it wouldn't be that much helpful in my opinion and would split the security model in different places. If you think we should change that, please open a follow-up. (In reply to Olli Pettay [:smaug] from comment #80) > Comment on attachment 572872 [details] [diff] [review] [diff] [details] [review] > Part D - Temporary security model > > > >+bool > >+Navigator::IsSmsAllowed() const > >+{ > >+ static const bool defaultSmsPermission = false; > >+ > >+ // First of all, the general pref has to be turned on. > >+ if (!Preferences::GetBool("dom.sms.enabled", defaultSmsPermission)) { > >+ return false; > >+ } > Can you pass const bool to GetBool? That is a bit surprising. The second parameter is the value used as the default value if the pref isn't present. It's not an out-param. (In reply to Olli Pettay [:smaug] from comment #70) > Comment on attachment 573608 [details] [diff] [review] [diff] [details] [review] > Part K - Implement SmsMessage > [...] > >+#include "jsdate.h" // For js_NewDateObjectMsec > have you asked JS people if this is ok? Why wouldn't that be okay? If that what you asked, I can always ask for a review.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #84) > If the pref is off or the site not whitelisted, mozSms returns null. Ah, right. that is ok. > > The second parameter is the value used as the default value if the pref > isn't present. It's not an out-param. indeed. > > >+#include "jsdate.h" // For js_NewDateObjectMsec > > have you asked JS people if this is ok? > > Why wouldn't that be okay? If that what you asked, I can always ask for a > review. I thought only jsapi.h should be used when just possible.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #79) > > I'm using this IPCSmsMessage as a compromise between writing the code to > > pass objects between processes and having SmsMessage data owned by PSMs.ipdl. > > I don't understand what you're compromising between. nsSmsMessage is in > dom/sms, right, just like PSms? Yes. But IPDL is related to IPC and SmsMessage is not directly related to that. Making SmsMessage inheriting from something declared in an IPDL file seems weird because of that. IMO, we should be able to remove all IPC related code and still be able to build Gecko. If you are strongly against any other solution, I will just do as recommended.

I wasn't suggesting inheriting from the message-data struct, only using it internally for data storage. ("Has-a", not "is-a".) I'm somewhat confused by the "removing IPC code" point; it's a fundamental technological building block, like xpconnect or spidermonkey or even C++. If we want to change a building block, code will be affected. I'm strongly against unnecessarily duplicating these data definitions, and writing unnecessary code. :)

Comment on attachment 576173 [details] [diff] [review] Part N - Receiving SMS, IPC handling >diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl >+struct SmsMessageData { >+ PRInt32 id; >+ // Whether the message has been received or sent (using constants in Constants.h). >+ // TODO: this should be an enum but IPDL doesn't know about enum. >+ PRInt8 delivery; Sure it does! // in .cpp enum Foo { /*...*/ }; // in .ipdl using Foo; //... async Message(Foo foo); You have to write a serializer for it, but that's simple. See the serializer for mozilla::PixelFormat in ipc/glue/IPCMessageUtils.h. This all looks fine except for the removal of enum DeliveryState.

The current patches can be tested in the Android build you can find here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mlamouri@mozilla.com-2b78b99255cd/ You can currently send a message with: navigator.mozSms.send(NUMBER, MESSAGE); and be informed when a new message comes with: navigator.mozSms.addEventListener("received", function(e) { alert(e.message); }); (you can also use mozSms.onReceived)

This is creating a EnumSerializer template class which I'm using for DeliveryState. I'm didn't change PixelFormat yet, I will open a bug for that. There is also a small difference between the template and what PixelFormat does: the highValue represents the highest value, not the value highest not allowed value.

Forgot to add one file...

Just downloaded the aforementioned try-build and I'm unable to get the sms to send (haven't tried receiving yet). On loading the following test page, sms should be sent (as I understand it) but presently isn't. Note that I tried a different number string also dropping the +1 so 2163xxxxxx <html> <head> <script type="text/javascript"> text = "hello sms!"; navigator.mozSms.send(+12163xxxxxx, text); </script> </head> <body> <p>Sends sms on load </p> </body> </html> page is hosted here: view-source:http://people.mozilla.com/~jhammink/webapi_test_pages/SMSv4.html

Sorry, I forgot to say you need to enable WebSMS with this pref: dom.sms.enabled (have to be set to true) And you have to whitelist the prepath [1] that are allowed to send SMS with this pref: dom.sms.whitelist (prepaths are comma-separated) [1] prepath is equivalent to protocol + user + password + domain. In your case: "http://people.mozilla.com" (I'm not sure if a final '/' is needed off-hand)

Comment on attachment 576468 [details] [diff] [review] Part N - Receiving SMS, IPC handling > #endif // mozilla_dom_sms_SmsMessage_h >diff --git a/dom/sms/src/Types.h b/dom/sms/src/Types.h >+namespace IPC { >+ >+using namespace mozilla::dom::sms; >+ This will import symbols from mozilla::dom::sms (sigh ... ;) into the IPC namespace for all headers that end up including Types.h, which isn't what you want to do. As nasty and verbose as it is, please use full qualification for the ParamTraits specialization below. >diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h >+/** >+ * Generic enum serializer. >+ * E is the enum type. >+ * lowValue is the lowest allowed value of the enum. >+ * highValue is the highest allowed value of the enum. >+ */ >+template <typename E, E lowValue, E highValue> Using the highest allowed value as a guard doesn't work, because it introduces a long invisible dependency from the enum declaration to where the serializer is declared. It's going to make it really annoying to keep the serializer up to date. So let's only use this serializer with enums that have a "sentinel" value at the end, like |eLast|. (That means you should add a sentinel to the delivery-state enum.) r=me with those fixes

Parts A to O have landed: https://hg.mozilla.org/mozilla-central/rev/00bc6d91c0e4 https://hg.mozilla.org/mozilla-central/rev/8af1322deb26 https://hg.mozilla.org/mozilla-central/rev/df37cbb57bfb https://hg.mozilla.org/mozilla-central/rev/099212b5f018 https://hg.mozilla.org/mozilla-central/rev/85e1daf90607 https://hg.mozilla.org/mozilla-central/rev/8456fe6baadd https://hg.mozilla.org/mozilla-central/rev/4ff83006b7bc https://hg.mozilla.org/mozilla-central/rev/d94567fea9de https://hg.mozilla.org/mozilla-central/rev/fcf8b5336711 https://hg.mozilla.org/mozilla-central/rev/423c6548b522 https://hg.mozilla.org/mozilla-central/rev/70aceb7cc312 https://hg.mozilla.org/mozilla-central/rev/c637a6a53ceb https://hg.mozilla.org/mozilla-central/rev/8fc0531d93f5 https://hg.mozilla.org/mozilla-central/rev/d09ef2ac2a3a https://hg.mozilla.org/mozilla-central/rev/dfb4684b40e0

Comment on attachment 576912 [details] [diff] [review] Part P - System to listen for sent messages in the Android backend Why does the SMS code need to be thread safe? In what circumstances will it not run on the java main thread?

Comment on attachment 576915 [details] [diff] [review] Part R - Save sent messages in the Android DB >+[scriptable, function, uuid(ab23f736-f545-4fcd-a298-3d6e2b380042)] > interface nsISmsDatabaseService : nsISupports > { >+ // Takes some information required to write the message and returns its id. >+ long writeSentMessage(in DOMString aReceiver, in DOMString aBody, in unsigned long long aDate); I prefer the name "saveSentMessage()". Feel free to call bikeshedding if you want. >diff --git a/dom/sms/src/android/SmsDatabaseService.cpp b/dom/sms/src/android/SmsDatabaseService.cpp >+NS_IMETHODIMP >+SmsDatabaseService::WriteSentMessage(const nsAString& aReceiver, >+ const nsAString& aBody, >+ PRUint64 aDate, PRInt32* aId) >+{ >+ if (!AndroidBridge::Bridge()) { >+ return NS_OK; >+ } >+ >+ *aId = AndroidBridge::Bridge()->WriteSentMessage(aReceiver, aBody, aDate); >+ return NS_OK; Nit: if (AndroidBridge* bridge = AndroidBridge::Bridge()) { *aId = bridge->WriteSentMessage(aReceiver, aBody, aDate); } return NS_OK; Except ... isn't the XPCOM contract that the outparam has to be written if you return NS_OK? I think you probably want to set *aId to -1 or 0 or something when we can't talk to android. So maybe add an |else| case to do that above. >diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl >+ sync WriteSentMessage(nsString aReceiver, nsString aBody, PRUint64 aDate) I would be tempted to re-use SmsMessageData here, but I'll leave that up to you.

Comment on attachment 577019 [details] [diff] [review] Part U - Delivered event: Android backend (Holding off on reviewing this pending "Part P".)

(In reply to Chris Jones [:cjones] [:warhammer] from comment #104) > Comment on attachment 576912 [details] [diff] [review] [diff] [details] [review] > Part P - System to listen for sent messages in the Android backend > > Why does the SMS code need to be thread safe? In what circumstances will it > not run on the java main thread? GeckoSmsManager.send() is always run in the Java main thread but GeckoSmsManager.onReceive() isn't. According to the doc, this method is called from another Dalvik process. I believe a Dalvik process is equivalent to a C-thread in the sense that the memory is shared but I have to make sure we don't have send() being called at the same time or even two onReceive() being called simultaneously. (In reply to Chris Jones [:cjones] [:warhammer] from comment #105) > Nit: > > if (AndroidBridge* bridge = AndroidBridge::Bridge()) { > *aId = bridge->WriteSentMessage(aReceiver, aBody, aDate); > } > return NS_OK; > > Except ... isn't the XPCOM contract that the outparam has to be > written if you return NS_OK? I think you probably want to set *aId to > -1 or 0 or something when we can't talk to android. So maybe add an > |else| case to do that above. I did that for SmsParent. I forgot to do it here. > >diff --git a/dom/sms/src/ipc/PSms.ipdl b/dom/sms/src/ipc/PSms.ipdl > > >+ sync WriteSentMessage(nsString aReceiver, nsString aBody, PRUint64 aDate) > > I would be tempted to re-use SmsMessageData here, but I'll leave that > up to you. It would increase the amount of code to write if we add a parameter in this list. In addition, I don't really like using half-complete objects (that's one of the reason the service isn't using nsIDOMSmsMessage).

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #107) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #104) > > Comment on attachment 576912 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review] > > Part P - System to listen for sent messages in the Android backend > > > > Why does the SMS code need to be thread safe? In what circumstances will it > > not run on the java main thread? > > GeckoSmsManager.send() is always run in the Java main thread but > GeckoSmsManager.onReceive() isn't. According to the doc, this method is > called from another Dalvik process. I believe a Dalvik process is equivalent > to a C-thread in the sense that the memory is shared but I have to make sure > we don't have send() being called at the same time or even two onReceive() > being called simultaneously. > http://stackoverflow.com/questions/5674518/does-broadcastreceiver-onreceive-always-run-in-the-ui-thread thinks otherwise. Since the google docs aren't clear on the point, I think it's fair to assume it's all UI-thread only, or else a mass of android apps would break.

Same patch without the semaphore.

Comment on attachment 576914 [details] [diff] [review] Part Q - SmsDatabaseService >+%{C++ >+#define SMS_DATABASE_SERVICE_CID { 0xb5bd6b55, 0x2e56, 0x4f1d, { 0xa4, 0x38, 0x1d, 0x10, 0xd6, 0xf3, 0x4d, 0xe5 } } Could you please split this to few lines. Or does .idl not allow that?

Same patch rebased.

Just a heads up. Part O was causing Firefox to crash on Android. Bug 706449 shows the fix. We'd appreciate getting one of the Android devs to do feedback reviews when landing into those files.

Comment on attachment 577638 [details] [diff] [review] Part P - System to listen for sent messages in the Android backend This is quite different than the original part P ... is this the right patch?

You did r+ that patch but it was the wrong one as you said in the comment. Did you meant to r+ "Part U" instead? Anyhow, that patch should be the good one.

If you all are at a stage where this is nearing completion or a design has been chosen to fix the problem we really should schedule a security review. Please take a look at the calendar (https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and let me know the date/time slot you would like.

dom/sms/Makefile.in, dom/sms/interfaces/Makefile.in and dom/sms/src/Makefile.in were created in the tree by part A of this bug, but were not listed in toolkit/toolkit-makefiles.sh, so I've included them here: https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

Comment on attachment 578027 [details] [diff] [review] Part P - System to listen for sent messages in the Android backend >diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java >+/** >+ * This class is returning unique ids for PendingIntent requestCode attribute. >+ * The ids are not really unique in the sense of if you are able to generate a >+ * number of PendingIntent of the same type equals to the number of values an >+ * Integer can have, you will have values conflicts. >+ * However, we can safely consider this issue as unlikely. Return a "unique" ID. There are only |Integer.MAX_VALUE - Integer.MIN_VALUE| unique IDs available, and they wrap around. >+ */ >+class PendingIntentUID >+{ >+ static private int sUID = Integer.MAX_VALUE; >+ >+ static public int generate() { >+ if (sUID == Integer.MAX_VALUE) { >+ sUID = Integer.MIN_VALUE; >+ return sUID; >+ } >+ >+ // Incrementing sUID *before* returning it. >+ return ++sUID; >+ } static private int sUID = Integer.MIN_VALUE; static public int generate() { return sUID++; } >+/** >+ * The envelope class contains all information that are needed to keep track of >+ * a sent SMS. >+ */ >+class Envelope >+{ >+ protected int mId; >+ protected int mRemainingParts; >+ protected boolean mFailing; >+ I tried very hard but failed to understand what benefit we gain from having a module-private Envelope "base class" along with another module-private EnvelopePrivate "subclass". Unless you have a really good reason for the current setup, please make mId+mRemainingParts+mFailing public, get rid of the accessor methods below, and move the EnvelopePrivate ctor into Envelope. Don't give in to the temptation of over-Java-ification! Gosling begone! > public class GeckoSmsManager > extends BroadcastReceiver > { > public static void send(String aNumber, String aMessage) { > if (aMessage.length() <= kMaxMessageSize) { Please remove this special case. AFAICT it's added maintenance burden for no gain. Looks OK, would like to see the next version though.

Comment on attachment 577650 [details] [diff] [review] Part U - Delivered event: Android backend Looking through here, I'm getting a bit confused about why we don't split the message parts up and then treat them as completely separate messages. That seems to be how they're exposed to gecko. If we did that, we could eliminate a lot of the state-tracking code in this patch, and in Part P. There are some other things I'd like to see changed in this patch, but let's discuss the above first. I'll be back on IRC around 1300-1400 Taiwan time, but we can discuss here too.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #117) > Comment on attachment 578027 [details] [diff] [review] [diff] [details] [review] > Part P - System to listen for sent messages in the Android backend > > >diff --git a/embedding/android/GeckoSmsManager.java b/embedding/android/GeckoSmsManager.java > >+ */ > >+class PendingIntentUID > >+{ > >+ static private int sUID = Integer.MAX_VALUE; > >+ > >+ static public int generate() { > >+ if (sUID == Integer.MAX_VALUE) { > >+ sUID = Integer.MIN_VALUE; > >+ return sUID; > >+ } > >+ > >+ // Incrementing sUID *before* returning it. > >+ return ++sUID; > >+ } > > static private int sUID = Integer.MIN_VALUE; > static public int generate() { return sUID++; } Do you know for sure that Java would behave like C here? I'm not, that's why my code is doing the checks. > > public class GeckoSmsManager > > extends BroadcastReceiver > > { > > public static void send(String aNumber, String aMessage) { > > > if (aMessage.length() <= kMaxMessageSize) { > > Please remove this special case. AFAICT it's added maintenance burden > for no gain. The reason why there are two paths is that I don't know what is the difference between sendTextMessage() and sendMultipartTextMessage() for a message that should take only one part. IOW, I don't know if calling sendMultipartTextMessage() puts some overhead that might make a 160 length message takes two parts. I would prefer to keep that as is and open a follow-up to investigate if it is safe to remove the special casing unless you already know how it is behaving.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #118) > Comment on attachment 577650 [details] [diff] [review] [diff] [details] [review] > Part U - Delivered event: Android backend > > Looking through here, I'm getting a bit confused about why we don't split > the message parts up and then treat them as completely separate messages. > That seems to be how they're exposed to gecko. If we did that, we could > eliminate a lot of the state-tracking code in this patch, and in Part P. > There are some other things I'd like to see changed in this patch, but let's > discuss the above first. I disagree. Android has a very bad API here. The developer has to split the SMS message into multiple messages and then get a callback for each parts being sent or delivered but what a developer really cares about when using the API is whether the message has been sent or delivered the fact that the message has 1, 2 or X parts should be a problem for the backend. Basically what I'm trying to do here is to make sure WebSMS consumers don't have to suffer from this exact same problem. Basically, the intent of this API is to let consumers do that regardless of the message: var request = navigator.mozSms.send(myNumber, myMessage); request.onsuccess = function() { alert("Message sent!"); }; Does that make sense?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #119) > Do you know for sure that Java would behave like C here? > I'm not, that's why my code is doing the checks. > Yes. > > > public class GeckoSmsManager > > > extends BroadcastReceiver > > > { > > > public static void send(String aNumber, String aMessage) { > > > > > if (aMessage.length() <= kMaxMessageSize) { > > > > Please remove this special case. AFAICT it's added maintenance burden > > for no gain. > > The reason why there are two paths is that I don't know what is the > difference between sendTextMessage() and sendMultipartTextMessage() for a > message that should take only one part. IOW, I don't know if calling > sendMultipartTextMessage() puts some overhead that might make a 160 length > message takes two parts. Read the code. I would bet money that sendTextMessage() just wraps sendMultipartTextMessage(). In fact, have you checked that sendTextMessage() doesn't automatically split for you? The docs really suck ...