While building the OWD SMS application, we found out the need of a `read` flag for the messages stored in the database.

The first draft of the spec had this but while implementing I realize that we will end up in some weird synchronization situations like this one: var m1 = navigator.sms.getMessage(42); var m2 = navigator.sms.getMessage(42); alert(m1.read); // should show "false" m1.read = true; alert(m2.read); // what should that show? "true" or "false"? If we want |m2.read| to be true in the example, this is not going to be trivial to implement. Otherwise, I think it might be confusing for the API consumer...

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #1) > The first draft of the spec had this but while implementing I realize that > we will end up in some weird synchronization situations like this one: > var m1 = navigator.sms.getMessage(42); > var m2 = navigator.sms.getMessage(42); > alert(m1.read); // should show "false" > m1.read = true; > alert(m2.read); // what should that show? "true" or "false"? > > If we want |m2.read| to be true in the example, this is not going to be > trivial to implement. Otherwise, I think it might be confusing for the API > consumer... Yes, that would be a tricky case and indeed not trivial to implement. Anyway that would probably be something that the API consumer must deal with(?). Correct me if I am wrong, not the same but a similar issue may occurr with |navigator.sms.delete| function. What would be the alternative for an sms client app that needs to keep a list of unread SMS? If we don´t provide a way to get the unread messages, we are forcing API consumers to keep another database of unread messages along with the actual sms database, right?

Yes, we should probably do that. Though, for delete(), I've no idea what would be the expected behavior. Sending a deleted event to the message maybe? Jonas, any ideas?

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #2) > What would be the alternative for an sms client app that needs to keep a > list of unread SMS? If we don´t provide a way to get the unread messages, we > are forcing API consumers to keep another database of unread messages along > with the actual sms database, right? I feel like this is a slippery slope. If the SMS app has to duplicate the data the system DB just to add functionality that's part of every SMS app (e.g. the read/unread flag), we're not really designing an API that developers will want to use.

FTR, I don't think we need a read/unread attribute to prevent authors to have a DB but because if you install multiple SMS apps, you might want to know which messages are read regardless of the app you used to read them.

Also a good point!

I've been speaking with Jonas and our conclusion is the following: - we can add a |readonly atrtibute boolean read| to SmsMessage; - we add a markMessageRead(in long id, in boolean aValue) to navigator.sms; - navigator.sms.getMessage{,s} will return a snapshot of messages, which means the message can change in the database, you will not be informed. Fernando, are you still interested in implementing that even in the Gecko side?

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7) > I've been speaking with Jonas and our conclusion is the following: > - we can add a |readonly atrtibute boolean read| to SmsMessage; > - we add a markMessageRead(in long id, in boolean aValue) to navigator.sms; > - navigator.sms.getMessage{,s} will return a snapshot of messages, which > means the message can change in the database, you will not be informed. Nice, it sounds like the easiest solution :) > Fernando, are you still interested in implementing that even in the Gecko > side? Sure!

The above patches are just WIP that apply and build ok. I still need to implement both backends and test it properly.

Sorry, there was additional code on patch 7 not related to this bug

`read` added to SmsFilter

B2G backend done and tested.

Still need to implement the Android backend. I am not sure if I should upload the tests to this bug after bug 733265 lands in m-c or just attach the tests for the `read` flag to this bug.

I just realized that I need `read` to allow the `undefined` value on SmsFilter, so I am canceling the review requests. Sorry for the spam.

Now the filter part is also tested

Comment on attachment 610191 [details] [diff] [review] Part 1: IDLs WIP-2 Review of attachment 610191 [details] [diff] [review]: ----------------------------------------------------------------- r- because of nsIDOMSmsFilter read attribute that should be |boolean|. We should also figure out what to do with the notifying part. ::: dom/sms/interfaces/nsIDOMSmsFilter.idl @@ +18,5 @@ > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): > * Mounir Lamouri <mounir.lamouri@mozilla.com> (Original Author) > + * Fernando Jiménez Moreno <ferjmoreno@gmail.com> Instead of adding your name, could you update the license header to MPLv2? See: https://www.mozilla.org/MPL/headers/ It apply to other IDLs. @@ +56,5 @@ > [Null(Empty)] > attribute DOMString delivery; > + > + // A read flag that can be a boolean (0,1) or undefined (-1). > + attribute short read; Here, that should be jsval. Actually, boolean? in WebIDL. ::: dom/sms/interfaces/nsISmsDatabaseService.idl @@ +59,5 @@ > > void createMessageList(in nsIDOMMozSmsFilter filter, in boolean reverse, in long requestId, [optional] in unsigned long long processId); > void getNextMessageInList(in long listId, in long requestId, [optional] in unsigned long long processId); > void clearMessageList(in long listId); > + void markMessageRead(in long messageId, in boolean value, in long requestId, [optional] in unsigned long long processId); See next comment. ::: dom/sms/interfaces/nsISmsRequestManager.idl @@ +78,5 @@ > + void notifyMarkedMessageRead(in long aRequestId, > + in bool aRead); > + > + void notifyMarkMessageReadFailed(in long aRequestId, > + in long aError); So, I have the feeling we discussed about that but I don't remember if I told you that was useful. Jonas and I still don't see the use cases for that so we should probably not add those events for the moment. Do you remember what I told you? (sorry, I'm in a work week and a lot of things happen...)

Comment on attachment 610193 [details] [diff] [review] Part 2: WebSMS WIP-2 Review of attachment 610193 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks ok except for the nits. Canceling the review because of the request/notification issue. ::: dom/sms/src/SmsFilter.cpp @@ +19,5 @@ > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): > * Mounir Lamouri <mounir.lamouri@mozilla.com> (Original Author) > + * Fernando Jiménez Moreno <ferjmoreno@gmail.com> See what I've said for the previous patch. ::: dom/sms/src/SmsManager.cpp @@ +328,5 @@ > + nsresult rv = requestManager->CreateRequest(this, aRequest, &requestId); > + if (NS_FAILED(rv)) { > + NS_ERROR("Failed to create the request!"); > + return rv; > + } I don't think you need a request here... unless we end up sending an event. ::: dom/sms/src/SmsMessage.cpp @@ +97,5 @@ > } else { > return NS_ERROR_INVALID_ARG; > } > > + data.read() = aRead; nit: could you add that after timestamp setting? or after body setting. ::: dom/sms/src/SmsRequestManager.cpp @@ +255,5 @@ > +NS_IMETHODIMP > +SmsRequestManager::NotifyMarkMessageReadFailed(PRInt32 aRequestId, PRInt32 aError) > +{ > + return NotifyError(aRequestId, aError); > +} Do we need that?

Comment on attachment 609805 [details] [diff] [review] Part 4: Fallback WIP-0 Review of attachment 609805 [details] [diff] [review]: ----------------------------------------------------------------- r=me but that part will have to be merged with the part adding the parameter to SmsMessage::Create otherwise Gecko will not compile if someone has the previous patch applied and not this one. ::: dom/sms/src/fallback/SmsDatabaseService.cpp @@ +19,5 @@ > * the Initial Developer. All Rights Reserved. > * > * Contributor(s): > * Mounir Lamouri <mounir.lamouri@mozilla.com> (Original Author) > + * Fernando Jiménez Moreno <ferjmoreno@gmail.com> Update the license header please.

Comment on attachment 610074 [details] [diff] [review] Part 5: IPC WIP-1 Review of attachment 610074 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/src/ipc/PSms.ipdl @@ +61,5 @@ > PRUint64 startDate; > PRUint64 endDate; > nsString[] numbers; > DeliveryState delivery; > + bool read; Shouldn't that be a TriState? (-1/0/1) @@ +103,5 @@ > PRUint64 aProcessId); > + NotifyRequestMarkedMessageRead(bool aRead, PRInt32 aRequestId, > + PRUint64 aProcessId); > + NotifyRequestMarkMessageReadFailed(PRInt32 aError, PRInt32 aRequestId, > + PRUint64 aProcessId); I don't think you need that. @@ +131,5 @@ > GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId, PRUint64 aProcessId); > > ClearMessageList(PRInt32 aListId); > > + MarkMessageRead(PRInt32 aMessageId, bool aValue, PRInt32 aRequestId, PRUint64 aProcessId); I don't think you need aRequestId. ::: dom/sms/src/ipc/SmsChild.cpp @@ +269,5 @@ > + do_GetService(SMS_REQUEST_MANAGER_CONTRACTID); > + requestManager->NotifyMarkMessageReadFailed(aRequestId, aError); > + > + return true; > +} IMO, you don't need that. ::: dom/sms/src/ipc/SmsChild.h @@ +61,5 @@ > NS_OVERRIDE virtual bool RecvNotifyRequestCreateMessageList(const PRInt32& aListId, const SmsMessageData& aMessage, const PRInt32& aRequestId, const PRUint64& aProcessId); > NS_OVERRIDE virtual bool RecvNotifyRequestGotNextMessage(const SmsMessageData& aMessage, const PRInt32& aRequestId, const PRUint64& aProcessId); > NS_OVERRIDE virtual bool RecvNotifyRequestReadListFailed(const PRInt32& aError, const PRInt32& aRequestId, const PRUint64& aProcessId); > + NS_OVERRIDE virtual bool RecvNotifyRequestMarkedMessageRead(const bool& aRead, const PRInt32& aRequestId, const PRUint64& aProcessId); > + NS_OVERRIDE virtual bool RecvNotifyRequestMarkMessageReadFailed(const PRInt32& aError, const PRInt32& aRequestId, const PRUint64& aProcessId); That neither.

Comment on attachment 610195 [details] [diff] [review] Part 6: B2G backend WIP-2 Review of attachment 610195 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/src/ril/SmsDatabaseService.js @@ +215,5 @@ > objectStore.createIndex("delivery", "delivery", { unique: false }); > objectStore.createIndex("sender", "sender", { unique: false }); > objectStore.createIndex("receiver", "receiver", { unique: false }); > objectStore.createIndex("timestamp", "timestamp", { unique:false }); > + objectStore.createIndex("read", "read", { unique:false }); Brrzzzzt. This alters the DB schema, which means we need to bump the DB_VERSION, and add a "case 1" upgrade step in ensureDB where we add this index to existing DBs. @@ +329,5 @@ > sender: sender, > receiver: null, //TODO see bug 733266 > body: body, > + timestamp: date, > + read: 0}; Why is `read` not a boolean? @@ +692,5 @@ > + > + getRequest.onerror = function onerror(event) { > + if (DEBUG) debug("Error on get request", event.target.errorCode); > + gSmsRequestManager.notifyMarkMessageReadFailed( > + requestId, Ci.nsISmsRequestManager.INTERNAL_ERROR); No need for this error handler if you already have the transaction error handler doing the exact same thing. We've been over this several times already!

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #27) > Comment on attachment 610191 [details] [diff] [review] > Part 1: IDLs WIP-2 > > Review of attachment 610191 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- because of nsIDOMSmsFilter read attribute that should be |boolean|. We > should also figure out what to do with the notifying part. > > ::: dom/sms/interfaces/nsIDOMSmsFilter.idl > @@ +18,5 @@ > > * the Initial Developer. All Rights Reserved. > > * > > * Contributor(s): > > * Mounir Lamouri <mounir.lamouri@mozilla.com> (Original Author) > > + * Fernando Jiménez Moreno <ferjmoreno@gmail.com> > > Instead of adding your name, could you update the license header to MPLv2? > See: https://www.mozilla.org/MPL/headers/ > > It apply to other IDLs. Sure! I was not sure what to do about the license. > > @@ +56,5 @@ > > [Null(Empty)] > > attribute DOMString delivery; > > + > > + // A read flag that can be a boolean (0,1) or undefined (-1). > > + attribute short read; > > Here, that should be jsval. Actually, boolean? in WebIDL. Hmmm... ok, that was what I was asking on the IRC. I had a jsval. > > ::: dom/sms/interfaces/nsISmsDatabaseService.idl > @@ +59,5 @@ > > > > void createMessageList(in nsIDOMMozSmsFilter filter, in boolean reverse, in long requestId, [optional] in unsigned long long processId); > > void getNextMessageInList(in long listId, in long requestId, [optional] in unsigned long long processId); > > void clearMessageList(in long listId); > > + void markMessageRead(in long messageId, in boolean value, in long requestId, [optional] in unsigned long long processId); > > See next comment. > > ::: dom/sms/interfaces/nsISmsRequestManager.idl > @@ +78,5 @@ > > + void notifyMarkedMessageRead(in long aRequestId, > > + in bool aRead); > > + > > + void notifyMarkMessageReadFailed(in long aRequestId, > > + in long aError); > > So, I have the feeling we discussed about that but I don't remember if I > told you that was useful. Jonas and I still don't see the use cases for that > so we should probably not add those events for the moment. Do you remember > what I told you? (sorry, I'm in a work week and a lot of things happen...) Yes, of course I remember :) But, we also had a short IRC conversation about that where I asked you about adding those events (similar to the `delete` function events) and you agreed. Anyway, no problem, I´ll remove the events. But just FTR, IMHO we should look for some kind of consistency here, so if we don´t notify about the `read` events, we should also not notify about the `delete` events.

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #32) > > @@ +56,5 @@ > > > [Null(Empty)] > > > attribute DOMString delivery; > > > + > > > + // A read flag that can be a boolean (0,1) or undefined (-1). > > > + attribute short read; > > > > Here, that should be jsval. Actually, boolean? in WebIDL. > Hmmm... ok, that was what I was asking on the IRC. I had a jsval. Yes, a short could make sense for the data we sent over IPC but not for the IDL. For the IDL, we need a boolean that can be undefined. > > ::: dom/sms/interfaces/nsISmsRequestManager.idl > > @@ +78,5 @@ > > > + void notifyMarkedMessageRead(in long aRequestId, > > > + in bool aRead); > > > + > > > + void notifyMarkMessageReadFailed(in long aRequestId, > > > + in long aError); > > > > So, I have the feeling we discussed about that but I don't remember if I > > told you that was useful. Jonas and I still don't see the use cases for that > > so we should probably not add those events for the moment. Do you remember > > what I told you? (sorry, I'm in a work week and a lot of things happen...) > > Yes, of course I remember :) But, we also had a short IRC conversation about > that where I asked you about adding those events (similar to the `delete` > function events) and you agreed. Anyway, no problem, I´ll remove the events. > But just FTR, IMHO we should look for some kind of consistency here, so if > we don´t notify about the `read` events, we should also not notify about the > `delete` events. Hmm, I would be okay with that actually. Though, your patch wasn't doing that IIRC.

The fallback part was alredy r+. I´ve just updated the headers with the new license.

I´ve added the tests related to the `read` flag along with the ones missing from bug 733265.

Comment on attachment 615752 [details] [diff] [review] Part 6: B2G backend WIP-3 Review of attachment 615752 [details] [diff] [review]: ----------------------------------------------------------------- r+ in general, but please address the questions and points below. Thanks! ::: dom/sms/src/ril/SmsDatabaseService.js @@ +24,5 @@ > const FILTER_NUMBERS = "numbers"; > const FILTER_DELIVERY = "delivery"; > +const FILTER_READ = "read"; > + > +const FILTER_READ_UNDEFINED = -1; This value seems unused. What's up with that? @@ +26,5 @@ > +const FILTER_READ = "read"; > + > +const FILTER_READ_UNDEFINED = -1; > +const FILTER_READ_UNREAD = 0; > +const FILTER_READ_READ = 1; Not being able to use bools makes me sad. Can you please add a comment here as to why we're using numbers? Thanks! @@ +161,5 @@ > self.createSchema(db); > break; > > + case 1: > + if (DB_VERSION == 2) { No need for that. This piece of code here can assume that it's in sync with the top of the file. ;) If we go to DB_VERSION = 3, we need to change this `case` block, of course. But then the hole system gets more complicated anyway. @@ +563,5 @@ > + // filter.read > + if (filter.read != undefined) { > + let read; > + filter.read ? read = FILTER_READ_READ : > + read = FILTER_READ_UNREAD; let read = filter.read ? FILTER_READ_READ : FILTER_READ_UNREAD; @@ +691,5 @@ > + if (DEBUG) debug("The value of message.read is already " + value); > + gSmsRequestManager.notifyMarkedMessageRead(requestId, message.read); > + return; > + } > + message.read = value; Maybe I'm missing something, but isn't `value` a boolean? Shouldn't we convert this value to one of the FILTER_READ_* consts before storing it in the DB?

Comment on attachment 615753 [details] [diff] [review] Part 7: Tests WIP-1 Review of attachment 615753 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/tests/test_smsdatabaseservice.xul @@ +843,5 @@ > +}); > + > +add_test(function test_markMessageRead_success() { > + info("test_markMessageRead_success"); > + let rId = Math.floor(Math.random()*111); let fakeRequestId = newRandomId(); like in the other tests, please. @@ +861,5 @@ > +}); > + > +add_test(function test_markMessageRead_failed() { > + info("test_markMessageRead_failed"); > + let rId = Math.floor(Math.random()*111); Ditto.

Comment on attachment 615746 [details] [diff] [review] Part 2: WebSMS WIP-3 Review of attachment 615746 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the comments applied. ::: dom/sms/src/SmsFilter.cpp @@ +235,5 @@ > + *aRead = JSVAL_VOID; > + return NS_OK; > + } > + > + *aRead = BOOLEAN_TO_JSVAL(mData.read()); Please use the C++-style JS API: aRead->setBoolean(mData.read()); @@ +248,5 @@ > + mData.read() = eReadState_Undefined; > + return NS_OK; > + } > + > + if (!JSVAL_IS_BOOLEAN(aRead)) { if (!aRead.isBoolean()) { @@ +253,5 @@ > + return NS_ERROR_INVALID_ARG; > + } > + > + bool read = JSVAL_TO_BOOLEAN(aRead); > + if (read) { if (aRead.toBoolean()) { ::: dom/sms/src/Types.h @@ +24,5 @@ > }; > > +// For SmsFilterData.read > +enum ReadState { > + eReadState_Undefined = -1, Could you use "Unknown" instead of "Undefined" just for consistency with DeliveryState.

Incorporate Mounir´s review comments. (Not sure if I should ask again for r?)

Incorporate philikon´s review comment.

Comment on attachment 616665 [details] [diff] [review] Part 6: B2G backend WIP-4 Review of attachment 616665 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/sms/src/ril/SmsDatabaseService.js @@ +559,5 @@ > + // Retrieve the keys from the 'read' index that matches the value of > + // filter.read > + if (filter.read != undefined) { > + let read = filter.read ? read = FILTER_READ_READ : > + read = FILTER_READ_UNREAD; Just let read = filter.read ? FILTER_READ_READ : FILTER_READ_UNREAD; please. r=me with that (no need to ask for review again, just upload a new patch please, thx!)

Comment on attachment 616662 [details] [diff] [review] Part 2: WebSMS WIP-4 Review of attachment 616662 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/sms/src/SmsFilter.cpp @@ +255,5 @@ > + > + if (aRead.toBoolean()) { > + mData.read() = eReadState_Read; > + } else { > + mData.read() = eReadState_Unread; FWIW, this could have been written this way: mData.read() = aRead.toBoolean() ? eReadState_Read : eReadState_Unread; It's just for information, you don't have to change it if you don't want.

Address mounir´s review comment. Looks better that way :) Thanks.

Comment on attachment 617023 [details] [diff] [review] Part 6: B2G backend WIP-5 This is a=b2g-only. The approval process is because Fennec is close to an important milestone so we don't have to ask for an approval if it's not touching Fennec in any way like this patch ;)

Comment on attachment 616671 [details] [diff] [review] Part 7: Tests WIP-2 Tests can be marked a=NPOTB (Not Part Of The Buld).

Is this ready to land yet? Part 3 (Android backend) doesn't seem to have had review at all, nor landing approval. Should we postpone it to a follow-up? Will the other patches work without part 3?

Added missing ril/SmsService.cpp part

Typo

As the part 3 is required in order to make it build for mobile/android, I am updating the patch implementing only dummy method, as Mounir suggested, and asking him for review.

Comment on attachment 617816 [details] [diff] [review] Part 3: Android backend WIP-2 Review of attachment 617816 [details] [diff] [review]: ----------------------------------------------------------------- I think you should stay outside of AndroidBridge. Basically, do what has to be done to make this code compile on Android without calling AndroidBridge. Also, I doubt this this linking, AndroidBridge::MarkMessageRead was never defined. ::: dom/sms/src/android/SmsDatabaseService.cpp @@ +108,5 @@ > + return NS_OK; > + } > + > + AndroidBridge::Bridge()->MarkMessageRead(aMessageId, aValue, aRequestId, > + aProcessId); Please do not call AndroidBridge here. Instead, mark in a // TODO that we should implement this in Bug XXXX (which requires you to create the bug ;)). ::: embedding/android/GeckoSmsManager.java @@ +1,4 @@ > /* -*- Mode: Java; c-basic-offset: 4; tab-width: 4; indent-tabs-mode: nil; -*- > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. */ You don't need to do that change. ::: widget/android/AndroidBridge.cpp @@ +178,5 @@ > jDeleteMessage = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "deleteMessage", "(IIJ)V"); > jCreateMessageList = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "createMessageList", "(JJ[Ljava/lang/String;IIZIJ)V"); > jGetNextMessageinList = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "getNextMessageInList", "(IIJ)V"); > jClearMessageList = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "clearMessageList", "(I)V"); > + jMarkMessageRead = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "markMessageRead", "(IZIJ)V"); Please don't touch that file. ::: widget/android/AndroidBridge.h @@ +405,5 @@ > void DeleteMessage(PRInt32 aMessageId, PRInt32 aRequestId, PRUint64 aProcessId); > void CreateMessageList(const dom::sms::SmsFilterData& aFilter, bool aReverse, PRInt32 aRequestId, PRUint64 aProcessId); > void GetNextMessageInList(PRInt32 aListId, PRInt32 aRequestId, PRUint64 aProcessId); > void ClearMessageList(PRInt32 aListId); > + void MarkMessageRead(PRInt32 aMessageId, bool aValue, PRInt32 aRequestId, PRUint64 aProcessId); Don't touch that file either. ::: widget/android/AndroidJNI.cpp @@ -284,2 @@ > return NS_OK; > } What's the change here?

Address Mounir´s review comments. I think these are the minimum changes to make it compile on Android. It doesn´t call AndroidBridge.

Comment on attachment 617894 [details] [diff] [review] Part 3: Android backend WIP-3 Review of attachment 617894 [details] [diff] [review]: ----------------------------------------------------------------- I wasn't able to see the changes in AndroidJNI.cpp last time (thanks to splinter review). This file will require some changes. Basically, you shouldn't add parameters to the methods. Set read to arbitrary values and add a TODO comment when appropriate.

Comment on attachment 618099 [details] [diff] [review] Part 3: Android backend WIP-4 Review of attachment 618099 [details] [diff] [review]: ----------------------------------------------------------------- r=me

BTW, approval isn't required anymore so you can just push everything now.

Thanks Mounir! I've got no push privileges. Philipp told me that he can push this patches until I get enough privileges to push it myself. Anyway I'd like to make a final test before pushing the patches, if that is ok for you. I'll let you know when I'm done asap.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f7ca01b1d5fd

I missed a spot :\. The RadioLayerInterface.js

Comment on attachment 618415 [details] [diff] [review] Part 8: RadioLayerInterface - WIP 0 Looks good to me... but it seems like you had never really tested this code, otherwise you would've seen exceptions in logcat. Can you please confirm that these patches work in fact on the phone? Thanks!

(In reply to Philipp von Weitershausen [:philikon] from comment #66) > Comment on attachment 618415 [details] [diff] [review] > Part 8: RadioLayerInterface - WIP 0 > > Looks good to me... but it seems like you had never really tested this code, > otherwise you would've seen exceptions in logcat. Can you please confirm > that these patches work in fact on the phone? Thanks! Yes, it works on the phone

New try build: https://tbpl.mozilla.org/?tree=Try&rev=4f3e87ce9ef8

Comment on attachment 616671 [details] [diff] [review] Part 7: Tests WIP-2 You also need to fix test_smsservice_createsmsmessage.js

Right. Actually that went on WIP 0. I must have missplaced it. I'll merge it and upload a new patch for the tests. Thanks!

Comment on attachment 618580 [details] [diff] [review] Part 7: Tests WIP-3 r=me with nits > /** > * Ensure an SmsMessage object created has sensible initial values. > */ > add_test(function test_interface() { >- let sms = newMessage(null, "sent", null, null, null, new Date()); >+ let sms = newMessage(null, "sent", null, null, null, new Date(), true); > do_check_true(sms instanceof Ci.nsIDOMMozSmsMessage); > do_check_eq(sms.id, 0); > do_check_eq(sms.delivery, "sent"); > do_check_eq(sms.receiver, null); > do_check_eq(sms.sender, null); > do_check_eq(sms.body, null); > do_check_true(sms.timestamp instanceof Date); >+ do_check_true(!sms.read); do_check_false plz > /** > * Test supplying the timestamp as a Date object. > */ > add_test(function test_timestamp_date() { > let date = new Date(); >- let sms = newMessage(42, "sent", "the sender", "the receiver", "the body", date); >+ let sms = newMessage(42, "sent", "the sender", "the receiver", "the body", >+ date, true); Align, plz.

New try build: https://tbpl.mozilla.org/?tree=Try&rev=cd57eee994b2

Try run for cd57eee994b2 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=cd57eee994b2 Results (out of 293 total builds): exception: 2 success: 235 warnings: 52 failure: 4 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-cd57eee994b2

Damn! I was running the wrong tests. Sorry again!

New try build: https://tbpl.mozilla.org/?tree=Try&rev=9599c54e73e9

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce2c136db1c https://hg.mozilla.org/integration/mozilla-inbound/rev/941825d1ced2 https://hg.mozilla.org/integration/mozilla-inbound/rev/eed9f67b76ac https://hg.mozilla.org/integration/mozilla-inbound/rev/ff546a006f8d https://hg.mozilla.org/integration/mozilla-inbound/rev/1be468638f6a https://hg.mozilla.org/integration/mozilla-inbound/rev/97cfed024d9b https://hg.mozilla.org/integration/mozilla-inbound/rev/26013982cb61 https://hg.mozilla.org/integration/mozilla-inbound/rev/8ccb4b796c56

https://hg.mozilla.org/mozilla-central/rev/3ce2c136db1c https://hg.mozilla.org/mozilla-central/rev/941825d1ced2 https://hg.mozilla.org/mozilla-central/rev/eed9f67b76ac https://hg.mozilla.org/mozilla-central/rev/ff546a006f8d https://hg.mozilla.org/mozilla-central/rev/1be468638f6a https://hg.mozilla.org/mozilla-central/rev/97cfed024d9b https://hg.mozilla.org/mozilla-central/rev/26013982cb61 https://hg.mozilla.org/mozilla-central/rev/8ccb4b796c56

Updated: https://developer.mozilla.org/en/Firefox_15_for_developers https://developer.mozilla.org/en/DOM/SmsFilter https://developer.mozilla.org/en/DOM/SmsManager https://developer.mozilla.org/en/DOM/SmsMessage https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISmsDatabaseService https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISmsRequestManager https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsISmsService