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

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

({dev-doc-complete})

Trunk
mozilla15
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 27 obsolete attachments)

17.52 KB, patch
mounir
: review+
Details | Diff | Splinter Review
22.78 KB, patch
mounir
: review+
Details | Diff | Splinter Review
21.66 KB, patch
Details | Diff | Splinter Review
17.50 KB, patch
Details | Diff | Splinter Review
6.35 KB, patch
Details | Diff | Splinter Review
11.33 KB, patch
mounir
: review+
Details | Diff | Splinter Review
2.29 KB, patch
philikon
: review+
Details | Diff | Splinter Review
33.78 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
While building the OWD SMS application, we found out the need of a `read` flag for the messages stored in the database.
(Assignee)

Updated

5 years ago
Blocks: 674725, 712809
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...
Version: unspecified → Trunk
(Assignee)

Comment 2

5 years ago
(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?
(Assignee)

Comment 8

5 years ago
(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!
(Assignee)

Updated

5 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Comment 9

5 years ago
Created attachment 609800 [details] [diff] [review]
Part 1: IDLs WIP-0
(Assignee)

Comment 10

5 years ago
Created attachment 609801 [details] [diff] [review]
Part 2: WebSMS WIP-0
(Assignee)

Comment 11

5 years ago
Created attachment 609802 [details] [diff] [review]
Part 3: Android backend WIP-0
(Assignee)

Comment 12

5 years ago
Created attachment 609805 [details] [diff] [review]
Part 4: Fallback WIP-0
(Assignee)

Comment 13

5 years ago
Created attachment 609807 [details] [diff] [review]
Part 5: IPC WIP-0
(Assignee)

Comment 14

5 years ago
Created attachment 609809 [details] [diff] [review]
Part 6: B2G backend WIP-0
(Assignee)

Comment 15

5 years ago
Created attachment 609810 [details] [diff] [review]
Part 7: Tests WIP-0
(Assignee)

Comment 16

5 years ago
The above patches are just WIP that apply and build ok. I still need to implement both backends and test it properly.
(Assignee)

Comment 17

5 years ago
Created attachment 609812 [details]
Part 7: Tests WIP-0

Sorry, there was additional code on patch 7 not related to this bug
Attachment #609810 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
Created attachment 610072 [details] [diff] [review]
Part 1: IDLs WIP-1

`read` added to SmsFilter
Attachment #609800 - Attachment is obsolete: true
Attachment #610072 - Flags: review?(mounir)
(Assignee)

Comment 19

5 years ago
Created attachment 610073 [details] [diff] [review]
Part 2: WebSMS WIP-1
Attachment #609801 - Attachment is obsolete: true
Attachment #610073 - Flags: review?(mounir)
(Assignee)

Comment 20

5 years ago
Created attachment 610074 [details] [diff] [review]
Part 5: IPC WIP-1
Attachment #609807 - Attachment is obsolete: true
Attachment #610074 - Flags: review?(mounir)
(Assignee)

Comment 21

5 years ago
Created attachment 610075 [details] [diff] [review]
Part 6: B2G backend WIP-1

B2G backend done and tested.
Attachment #609809 - Attachment is obsolete: true
Attachment #610075 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #609805 - Flags: review?(mounir)
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #609805 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #610072 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #610073 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #610074 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #610075 - Flags: review?(philipp)
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Comment 24

5 years ago
Created attachment 610191 [details] [diff] [review]
Part 1: IDLs WIP-2
Attachment #610072 - Attachment is obsolete: true
Attachment #610191 - Flags: review?(mounir)
(Assignee)

Comment 25

5 years ago
Created attachment 610193 [details] [diff] [review]
Part 2: WebSMS WIP-2
Attachment #610073 - Attachment is obsolete: true
Attachment #610193 - Flags: review?(mounir)
(Assignee)

Comment 26

5 years ago
Created attachment 610195 [details] [diff] [review]
Part 6: B2G backend WIP-2

Now the filter part is also tested
Attachment #610075 - Attachment is obsolete: true
Attachment #610195 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #610074 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #609805 - Flags: review?(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 <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...)
Attachment #610191 - Flags: review?(mounir) → review-
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?
Attachment #610193 - Flags: review?(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 <mounir.lamouri@mozilla.com> (Original Author)
> + *   Fernando Jiménez Moreno <ferjmoreno@gmail.com>

Update the license header please.
Attachment #609805 - Flags: review?(mounir) → review+
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.
Attachment #610074 - Flags: review?(mounir)
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!
Attachment #610195 - Flags: review?(philipp) → review-
(Assignee)

Comment 32

5 years ago
(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.
(Assignee)

Comment 34

5 years ago
Created attachment 615745 [details] [diff] [review]
Part 1: IDLs WIP-2
Attachment #610191 - Attachment is obsolete: true
Attachment #615745 - Flags: review?
(Assignee)

Comment 35

5 years ago
Created attachment 615746 [details] [diff] [review]
Part 2: WebSMS WIP-3
Attachment #610193 - Attachment is obsolete: true
Attachment #615746 - Flags: review?(mounir)
(Assignee)

Comment 36

5 years ago
Created attachment 615748 [details] [diff] [review]
Part 3: Android backend WIP-1
(Assignee)

Updated

5 years ago
Attachment #609802 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
Created attachment 615749 [details] [diff] [review]
Part 4: Fallback WIP-1

The fallback part was alredy r+. I´ve just updated the headers with the new license.
Attachment #609805 - Attachment is obsolete: true
Attachment #615749 - Flags: review?(mounir)
(Assignee)

Comment 38

5 years ago
Created attachment 615751 [details] [diff] [review]
Part 5: IPC WIP-2
Attachment #610074 - Attachment is obsolete: true
Attachment #615751 - Flags: review?(mounir)
(Assignee)

Comment 39

5 years ago
Created attachment 615752 [details] [diff] [review]
Part 6: B2G backend WIP-3
Attachment #610195 - Attachment is obsolete: true
Attachment #615752 - Flags: review?(philipp)
(Assignee)

Updated

5 years ago
Attachment #609812 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #615745 - Flags: review? → review?(mounir)
(Assignee)

Comment 40

5 years ago
Created attachment 615753 [details] [diff] [review]
Part 7: Tests WIP-1

I´ve added the tests related to the `read` flag along with the ones missing from bug 733265.
Attachment #609812 - Attachment is obsolete: true
Attachment #615753 - Flags: review?(philipp)
Attachment #609812 - Flags: review?(mounir)
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?
Attachment #615752 - Flags: review?(philipp) → review+
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.
Attachment #615753 - Flags: review?(philipp) → review+
Attachment #615745 - Flags: review?(mounir) → review+
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.
Attachment #615746 - Flags: review?(mounir) → review+
Attachment #615749 - Flags: review?(mounir) → review+
Attachment #615751 - Flags: review?(mounir) → review+
(Assignee)

Comment 44

5 years ago
Created attachment 616662 [details] [diff] [review]
Part 2: WebSMS WIP-4

Incorporate Mounir´s review comments. 

(Not sure if I should ask again for r?)
Attachment #615746 - Attachment is obsolete: true
Attachment #616662 - Flags: review?(mounir)
(Assignee)

Comment 45

5 years ago
Created attachment 616665 [details] [diff] [review]
Part 6: B2G backend WIP-4

Incorporate philikon´s review comment.
Attachment #615752 - Attachment is obsolete: true
Attachment #616665 - Flags: review?(philipp)
(Assignee)

Comment 46

5 years ago
Created attachment 616671 [details] [diff] [review]
Part 7: Tests WIP-2
Attachment #615753 - Attachment is obsolete: true
Attachment #616671 - Flags: review?(philipp)
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!)
Attachment #616665 - Flags: review?(philipp) → review+
Attachment #616671 - Flags: review?(philipp) → review+
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.
Attachment #616662 - Flags: review?(mounir) → review+
(Assignee)

Comment 49

5 years ago
Created attachment 617022 [details] [diff] [review]
Part 2: WebSMS WIP-5

Address mounir´s review comment. Looks better that way :) Thanks.
Attachment #616662 - Attachment is obsolete: true
(Assignee)

Comment 50

5 years ago
Created attachment 617023 [details] [diff] [review]
Part 6: B2G backend WIP-5
Attachment #616665 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #615745 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #615748 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #615748 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #615749 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #615751 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #616671 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #617022 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #617023 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #617023 - Flags: approval-mozilla-central?
(Assignee)

Updated

5 years ago
Attachment #617023 - Flags: approval-mozilla-central?
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 ;)
Attachment #617023 - Flags: approval-mozilla-central?
Comment on attachment 616671 [details] [diff] [review]
Part 7: Tests WIP-2

Tests can be marked a=NPOTB (Not Part Of The Buld).
Attachment #616671 - Flags: approval-mozilla-central?
Attachment #615745 - Flags: approval-mozilla-central? → approval-mozilla-central+
Attachment #615749 - Flags: approval-mozilla-central? → approval-mozilla-central+
Attachment #615751 - Flags: approval-mozilla-central? → approval-mozilla-central+
Attachment #617022 - Flags: approval-mozilla-central? → approval-mozilla-central+
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?
(Assignee)

Comment 54

5 years ago
Created attachment 617719 [details] [diff] [review]
Part 6: B2G backend WIP-6

Added missing ril/SmsService.cpp part
(Assignee)

Updated

5 years ago
Attachment #617023 - Attachment is obsolete: true
(Assignee)

Comment 55

5 years ago
Created attachment 617814 [details] [diff] [review]
Part 4: Fallback WIP-2

Typo
Attachment #615749 - Attachment is obsolete: true
(Assignee)

Comment 56

5 years ago
Created attachment 617816 [details] [diff] [review]
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.
Attachment #615748 - Attachment is obsolete: true
Attachment #617816 - Flags: review?(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?
Attachment #617816 - Flags: review?(mounir) → review-
(Assignee)

Updated

5 years ago
Depends on: 748391
(Assignee)

Comment 58

5 years ago
Created attachment 617894 [details] [diff] [review]
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.
Attachment #617816 - Attachment is obsolete: true
Attachment #617894 - Flags: review?(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.
Attachment #617894 - Flags: review?(mounir)
(Assignee)

Comment 60

5 years ago
Created attachment 618099 [details] [diff] [review]
Part 3: Android backend WIP-4
Attachment #617894 - Attachment is obsolete: true
Attachment #618099 - Flags: review?(mounir)
(Assignee)

Updated

5 years ago
Attachment #618099 - Attachment is patch: true
Comment on attachment 618099 [details] [diff] [review]
Part 3: Android backend WIP-4

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

r=me
Attachment #618099 - Flags: review?(mounir) → review+
BTW, approval isn't required anymore so you can just push everything now.
(Assignee)

Comment 63

5 years ago
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
(Assignee)

Comment 65

5 years ago
Created attachment 618415 [details] [diff] [review]
Part 8: RadioLayerInterface - WIP 0

I missed a spot :\. The RadioLayerInterface.js
Attachment #618415 - Flags: review?(philipp)
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!
Attachment #618415 - Flags: review?(philipp) → review+
(Assignee)

Comment 67

5 years ago
(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
Attachment #616671 - Flags: review+ → review-
(Assignee)

Comment 70

5 years ago
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!
(Assignee)

Comment 71

5 years ago
Created attachment 618580 [details] [diff] [review]
Part 7: Tests WIP-3
Attachment #616671 - Attachment is obsolete: true
Attachment #618580 - Flags: review?(philipp)
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.
Attachment #618580 - Flags: review?(philipp) → review+
New try build: https://tbpl.mozilla.org/?tree=Try&rev=cd57eee994b2

Comment 74

5 years ago
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
(Assignee)

Comment 75

5 years ago
Created attachment 619257 [details] [diff] [review]
Part 7: Tests WIP-4

Damn! I was running the wrong tests. Sorry again!
Attachment #618580 - Attachment is obsolete: true
(Assignee)

Comment 76

5 years ago
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
Target Milestone: --- → mozilla15
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.