733320 - [WebSMS][B2G SMS] Add 'read' attribute to nsIDOMMozSmsMessage

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Mounir Lamouri (:mounir)]

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

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 3

[Mounir Lamouri (:mounir)]

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?

Comment 4

[Philipp von Weitershausen [:philikon]]

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

Comment 5

[Mounir Lamouri (:mounir)]

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.

Comment 6

[Philipp von Weitershausen [:philikon]]

Also a good point!

Comment 7

[Mounir Lamouri (:mounir)]

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?

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: IDLs WIP-0

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-0

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Android backend WIP-0

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Fallback WIP-0

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: IPC WIP-0

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-0

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-0

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-0

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

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: IDLs WIP-1

`read` added to SmsFilter

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-1

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: IPC WIP-1

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-1

B2G backend done and tested.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: IDLs WIP-2

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-2

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-2

Now the filter part is also tested

Comment 27

[Mounir Lamouri (:mounir)]

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 <[e-mail]> (Original Author)
> + *  Fernando Jiménez Moreno <[e-mail]>

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 28

[Mounir Lamouri (:mounir)]

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 <[e-mail]> (Original Author)
> + *   Fernando Jiménez Moreno <[e-mail]>

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 29

[Mounir Lamouri (:mounir)]

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 <[e-mail]> (Original Author)
> + *   Fernando Jiménez Moreno <[e-mail]>

Update the license header please.

Comment 30

[Mounir Lamouri (:mounir)]

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 31

[Philipp von Weitershausen [:philikon]]

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!

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

(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 <[e-mail]> (Original Author)
> > + *  Fernando Jiménez Moreno <[e-mail]>
> 
> 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.

Comment 33

[Mounir Lamouri (:mounir)]

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

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: IDLs WIP-2

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-3

Comment 36

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Android backend WIP-1

Comment 37

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Fallback WIP-1

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

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: IPC WIP-2

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-3

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-1

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

Comment 41

[Philipp von Weitershausen [:philikon]]

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 42

[Philipp von Weitershausen [:philikon]]

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 43

[Mounir Lamouri (:mounir)]

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.

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-4

Incorporate Mounir´s review comments. 

(Not sure if I should ask again for r?)

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-4

Incorporate philikon´s review comment.

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-2

Comment 47

[Philipp von Weitershausen [:philikon]]

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 48

[Mounir Lamouri (:mounir)]

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.

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: WebSMS WIP-5

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

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-5

Comment 51

[Mounir Lamouri (:mounir)]

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 52

[Mounir Lamouri (:mounir)]

Comment on attachment 616671 [details] [diff] [review]
Part 7: Tests WIP-2

Tests can be marked a=NPOTB (Not Part Of The Buld).

Comment 53

[Philipp von Weitershausen [:philikon]]

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?

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: B2G backend WIP-6

Added missing ril/SmsService.cpp part

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: Fallback WIP-2

Typo

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Android backend WIP-2

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 57

[Mounir Lamouri (:mounir)]

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?

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Android backend WIP-3

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

Comment 59

[Mounir Lamouri (:mounir)]

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 60

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Android backend WIP-4

Comment 61

[Mounir Lamouri (:mounir)]

Comment on attachment 618099 [details] [diff] [review]
Part 3: Android backend WIP-4

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

r=me

Comment 62

[Mounir Lamouri (:mounir)]

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

Comment 63

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 64

[Philipp von Weitershausen [:philikon]]

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

Comment 65

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 8: RadioLayerInterface - WIP 0

I missed a spot :\. The RadioLayerInterface.js

Comment 66

[Philipp von Weitershausen [:philikon]]

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!

Comment 67

[Fernando Jiménez Moreno [:ferjm]]

(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

Comment 68

[Philipp von Weitershausen [:philikon]]

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

Comment 69

[Philipp von Weitershausen [:philikon]]

Comment on attachment 616671 [details] [diff] [review]
Part 7: Tests WIP-2

You also need to fix test_smsservice_createsmsmessage.js

Comment 70

[Fernando Jiménez Moreno [:ferjm]]

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 71

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-3

Comment 72

[Philipp von Weitershausen [:philikon]]

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.

Comment 73

[Philipp von Weitershausen [:philikon]]

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

Comment 74

[Mozilla RelEng Bot]

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/[e-mail]-cd57eee994b2

Comment 75

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 7: Tests WIP-4

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

Comment 76

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 77

[Philipp von Weitershausen [:philikon]]

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

Comment 78

[Ryan VanderMeulen [:RyanVM]]

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

Comment 79

[Florian Scholz (Open Web Docs)]

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