712809 - B2G SMS database

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Philipp von Weitershausen [:philikon]]

This is going to implement a nsISmsDatabaseService backend, probably using IndexedDB (tentatively marking this dependent on IndexedDB-in-chrome, bug 587797).

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: SmsDatabase WIP

This is the WIP sms database using indexeddb. It initial intent is to make it work as a mockup for our OWD demo.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

The above implementation is mostly based on philikon´s webcontact code

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

I´ve got several doubts about how the current WebSMS code works and that is not helping me with the b2g backend. I am currently trying to trace a .getMessage() call as an use case to figure out how it works. But with all the mix of xpcom, ipc, java and so on ... I am kind a lost.
As far as I can see, it starts with SmsManager::GetMessageMoz passing the id and a pointer to a nsIDOMMozSmsRequest, which is used by the SmsRequestManager to create the actual SmsRequest. Then it calls smsDBService->GetMessageMoz with the message id and the request id. SmsDatabaseService::GetMessageMoz calls via AndroidBridge the GeckoSmsManager.getMessage function, again with the message and request ids. This function reads from the Android SMS content provider (which AFAIK is synchronous) and calls a GeckoAppShell.notifyGetSms with the sms parameters and the requestId. And then it all gets pretty fuzzy to me...
As far as I can see the .getMessage function must return a nsIDOMMozSmsRequest, and I am seeing this public static native void notifyGetSms, which may refers to SmsRequestManager::NotifyGotSms function which I don´t know how receives the requestId and the message. I guess that the content of this message object must be filled somewhere, and that b2g backend must reproduce that same behaviour.
Any explanation about how is WebSMS structured would be much appreciated :)

Comment 4

[Mounir Lamouri (:mounir)]

This is roughly what happens when you do mozSms.getMessage(id):
- Calling SmsManager::GetMessageMoz [1] which is creating a request object. The message id and the request id are both given to smsDBService->GetMessageMoz(). The request object is returned to the script.
- Then, there is two possibility:
  - We have content processes (like in Fennec) so we go trough SmsIPCService which is calling the parent process but adds the content process id to the call (ContentChild::GetSingleton()->GetID()). This is why smsDBService->GetMessageMoz() has a 0 parameter at the end.
  - We are not in a content process or there are no content process so we just call the correct backend.
- On Android, at that point, we go trough AndroidBridge which is calling the appropriate Java method.
- The Java method send a runnable to get the message and when the runnable will be run, |notifyGetSms| will be called.
- notifyGetSms call is using JNI and the method is defined here: widget/android/AndroidJNI.cpp As you can see, we are notifying the SmsRequestManager that a message has been find and the message is passed in parameter with the request id and the process id if needed.
- Once the SmsRequestManager gets the notification, it is sending the correct event and set SmsRequest.result to the appropriate value.

To be able to access SmsRequestManager from Javascript, we need to do some changes. I was planning to do that as a follow-up but I still didn't find time to open all the follow-ups I had in mind :(
I will try to have this done next week so it will not block you.

[1] Big up to Windows! ;)

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> This is roughly what happens when you do mozSms.getMessage(id):
> - Calling SmsManager::GetMessageMoz [1] which is creating a request object.
> The message id and the request id are both given to
> smsDBService->GetMessageMoz(). The request object is returned to the script.
> - Then, there is two possibility:
>   - We have content processes (like in Fennec) so we go trough SmsIPCService
> which is calling the parent process but adds the content process id to the
> call (ContentChild::GetSingleton()->GetID()). This is why
> smsDBService->GetMessageMoz() has a 0 parameter at the end.
>   - We are not in a content process or there are no content process so we
> just call the correct backend.
> - On Android, at that point, we go trough AndroidBridge which is calling the
> appropriate Java method.
> - The Java method send a runnable to get the message and when the runnable
> will be run, |notifyGetSms| will be called.
> - notifyGetSms call is using JNI and the method is defined here:
> widget/android/AndroidJNI.cpp As you can see, we are notifying the
> SmsRequestManager that a message has been find and the message is passed in
> parameter with the request id and the process id if needed.
> - Once the SmsRequestManager gets the notification, it is sending the
> correct event and set SmsRequest.result to the appropriate value.
> 
Thanks Mounir! That´s a great explanation. I get it now, I was missing the widget/android/AndroidJNI.cpp code. 

> To be able to access SmsRequestManager from Javascript, we need to do some
> changes. I was planning to do that as a follow-up but I still didn't find
> time to open all the follow-ups I had in mind :(
> I will try to have this done next week so it will not block you.
Thanks, that would be great.

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: SmsDatabaseService-v1 (WIP)

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

The above patch is only a first sketch of the structure of the SmsDatabaseService js implementation.
As you can see, the idea is to have an SmsDatabase object that will handle all the indexeddb stuff and an SmsDatabaseService object that will implement the nsISmsDatabaseService.idl functions (should I call the file nsSmsDatabaseService.js?) and will use SmsDatabase.
I tried to add this code to the current WebSms code. It compiles and I can get an instance of the service from an xpcshelltest, but I am not sure if this is the best way to do it.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: SmsDatabaseService-v2 (WIP)

Needed to upgrade the patch to include the nsIIndexedDatabaseManager calls

Comment 9

[Mounir Lamouri (:mounir)]

This is a nit, but we are trying to avoid prefixing our files with ns nowadays.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> This is a nit, but we are trying to avoid prefixing our files with ns
> nowadays.

Agreed (for everything that's not an interface).

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 590764 [details] [diff] [review]
SmsDatabaseService-v2 (WIP)

>+ * The Original Code is mozilla.org code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2011

Please always copy a new license header from https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c and fill out the blank spaces (*without* reformatting the text). In particular, in this case you want to say 2012 :)

>+//TODO: own number must be retrieved somehow from the RIL
>+const CURRENT_ADDRESS = "[phone]";

Please file a bug for this.


Apart from this, this still mostly resembles my WebContacts prototype and not the nsISmsDatabaseService API. I think you're probably better off to start from scratch since the two APIs are very different.

Comment 12

[Philipp von Weitershausen [:philikon]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #4)
> To be able to access SmsRequestManager from Javascript, we need to do some
> changes. I was planning to do that as a follow-up but I still didn't find
> time to open all the follow-ups I had in mind :(

Filed bug 720632 for this.

Comment 13

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> Please always copy a new license header from
> https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c and fill out
> the blank spaces (*without* reformatting the text). In particular, in this
> case you want to say 2012 :)

Turns out, MPL 2 is active already, so please take headers from http://www.mozilla.org/MPL/headers/. Much simpler, too :)

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #9)
> This is a nit, but we are trying to avoid prefixing our files with ns
> nowadays.
Ok, then I´ll leave it as SmsDatabaseService.

(In reply to Philipp von Weitershausen [:philikon] from comment #11)
> Comment on attachment 590764 [details] [diff] [review]
> SmsDatabaseService-v2 (WIP)
> >+//TODO: own number must be retrieved somehow from the RIL
> >+const CURRENT_ADDRESS = "[phone]";
> 
> Please file a bug for this.
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=720638 for this.

> 
> Apart from this, this still mostly resembles my WebContacts prototype and
> not the nsISmsDatabaseService API. I think you're probably better off to
> start from scratch since the two APIs are very different.
Yes, you are right. Anyway, there´s a lot of your prototype that can be reused for this, right?

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> (In reply to Philipp von Weitershausen [:philikon] from comment #11)
> > Please always copy a new license header from
> > https://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c and fill out
> > the blank spaces (*without* reformatting the text). In particular, in this
> > case you want to say 2012 :)
> 
> Turns out, MPL 2 is active already, so please take headers from
> http://www.mozilla.org/MPL/headers/. Much simpler, too :)
Ok, I´ll do that.

Thanks!

Comment 15

[Philipp von Weitershausen [:philikon]]

Attachment: wip v3

Here's a WIP that I created from scratch while borrowing some of the IndexedDB bootstrapping code from the previous WIPs. This patch focuses on actually implementing nsISmsDatabaseService and should have all the XPCOM boilerplate.

Note that it makes a few assumptions that are not valid, at least right now. One of them is that SmsServiceFactory and the IPC stuff can deal with this JS component, which is probably not the case. I haven't thought deeply about this yet. For all the other invalid assumptions, see comments in the code and the bugs they refer to.

Comment 16

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
>//The message list stuff could be elegantly implemented using IDB cursors,
>//except we'd need to keep the txn open, so maybe not such a good idea
>//(unless we find a way to queue other requests while a list is being
>//processed, but that sounds messy).
I am trying to figure out the alternatives to avoid the use of IDBCursor for this and I am wondering if retrieving the list of messages within getMessageList and keeping a copy in memory (within the current this._messageLists) would be too much overhead. What other options we have?

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #16)
> I am trying to figure out the alternatives to avoid the use of IDBCursor for
> this and I am wondering if retrieving the list of messages within
> getMessageList and keeping a copy in memory (within the current
> this._messageLists) would be too much overhead. What other options we have?

You can get just the keys from the index you're querying via index.getAllKeys(). I would say, keep that list around, and then just fetch one object at a time from the store via store.get(key). That list would be useful for doing unions and intersections, anyway. (For sorted results, we'd have to do this via index.openKeyCursor(), but that's an implementation detail.)

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v4

Here it goes another WIP with the addition of the message lists stuff avoiding the use of IDBCursors.

The createMessageList function now retrieves the IDBKey´s that match the search criteria given by the filter object that is passed to this function. It makes the intersection of this keys and keep this list with MessagesListManager. When getNextMessage is called, it takes the next IDBKey on the list that matches the listId given as parameter. This key is deleted from that list, as I don´t really see the point to keep it. Once the key is retrieved, the store.get(key) retrieves the message and notify properly.

(I am not sure if I have to ask for review with every WIP patch...)

Comment 19

[Philipp von Weitershausen [:philikon]]

Comment on attachment 592727 [details] [diff] [review]
WIP v4

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

I haven't looked super closely at the message list stuff yet. At a glance it seemed to make sense to me, but the way it was written is a bit convoluted. I gave some tips below on how to spell it better.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +20,5 @@
> +const eNoError = 0;
> +const eNoSignalError = 1;
> +const eNotFoundError = 2;
> +const eUnknownError = 3;
> +const eInternalError = 4;

FYI, These will be available from nsISmsRequestManager. At least that's how my patch in bug 720632 does it.

@@ +47,5 @@
> +  // message list.
> +  let _keys = Object.create(null);
> +
> +  // Public methods for managing the message lists.
> +  return {

The closure pattern means we're indenting a lot here, just to keep the `_keys` object private. That seems unnecessary to me.

@@ +52,5 @@
> +    /**
> +     * Add a list to the manager.
> +     *
> +     * @param keys[]
> +     *        Object containing a list of IDBKeys as Object properties.

`keys[]` is not a standard spelling and very confusing if it's actually not an array but an object.

@@ +210,5 @@
> +  newTxn: function newTxn(txn_type, callback, oncompleteCb, onerrorCb) {
> +    this.ensureDB(function (error, db) {
> +      if (error) {
> +        if (DEBUG) debug("Could not open database: " + error);
> +        callback(null, null, error);

Let's be consistent about the placement of the 'error' parameter. A common convention is to have it be the first one.

@@ +218,5 @@
> +      let txn = db.transaction([STORE_NAME], txn_type);
> +      if (DEBUG) debug("Retrieving object store", STORE_NAME);
> +      let store = txn.objectStore(STORE_NAME);
> +      txn.oncomplete = oncompleteCb;
> +      txn.onerror = onerrorCb;

Please attach these in the individual methods, like so:

  newTxn(..., function (txn, store) {
    txn.oncomplete = function oncomplete() {
      ...
    };
    txn.onerror = function onerror() {
      ...
    };
  });

Makes it much easier to read and understand what's going on.

@@ +343,5 @@
> +    //      obtention and it is definitely sorted.
> +    let filteredKeys = [];
> +    // We need to apply the searches according to all the parameters of the
> +    // filter. filterCount will decrease with each of this searches.
> +    let filterCount = 4;

Where does this number come from? It should probably be a global const with a nice descriptive name!

@@ +349,5 @@
> +    let successCb = function (event) {
> +      let result = event.target.result;
> +      // Once the cursor has retrieved all keys that matches its key range,
> +      // the filter search is done and filterCount is decreased.
> +      if (!!result == false) {

if (!result)

@@ +427,5 @@
> +          filterCount -= 2;
> +        }
> +      }, function (event) {
> +        if (filterCount == 0) {
> +          if (filteredKeys == 0) {

Huh? I thought `filteredKeys` was an array? Do you mean `if (!filteredKeys.length)`?

@@ +434,5 @@
> +          }
> +          // We need to get rid off the duplicated keys.
> +          let result = [];
> +          for (let i = 0; i < filteredKeys.length; i++ ) {
> +            if ( result.indexOf( filteredKeys[i], 0, filteredKeys ) < 0 ) {

Nit: no space after ( and before ).

Comment 20

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #19)
> Comment on attachment 592727 [details] [diff] [review]
> WIP v4
> 
> Review of attachment 592727 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I haven't looked super closely at the message list stuff yet. At a glance it
> seemed to make sense to me, but the way it was written is a bit convoluted.
> I gave some tips below on how to spell it better.
> 
> ::: dom/sms/src/ril/SmsDatabaseService.js
> @@ +20,5 @@
> > +const eNoError = 0;
> > +const eNoSignalError = 1;
> > +const eNotFoundError = 2;
> > +const eUnknownError = 3;
> > +const eInternalError = 4;
> 
> FYI, These will be available from nsISmsRequestManager. At least that's how
> my patch in bug 720632 does it.
> 
Once again, I didn´t notice :)

> @@ +47,5 @@
> > +  // message list.
> > +  let _keys = Object.create(null);
> > +
> > +  // Public methods for managing the message lists.
> > +  return {
> 
> The closure pattern means we're indenting a lot here, just to keep the
> `_keys` object private. That seems unnecessary to me.
> 
So, there is no problem for making _keys public?

> @@ +52,5 @@
> > +    /**
> > +     * Add a list to the manager.
> > +     *
> > +     * @param keys[]
> > +     *        Object containing a list of IDBKeys as Object properties.
> 
> `keys[]` is not a standard spelling and very confusing if it's actually not
> an array but an object.
That comment inherits of a previous implementation. I forgot to change it. Thanks.

> 
> @@ +210,5 @@
> > +  newTxn: function newTxn(txn_type, callback, oncompleteCb, onerrorCb) {
> > +    this.ensureDB(function (error, db) {
> > +      if (error) {
> > +        if (DEBUG) debug("Could not open database: " + error);
> > +        callback(null, null, error);
> 
> Let's be consistent about the placement of the 'error' parameter. A common
> convention is to have it be the first one.
Ok, then the success case must be called as callback(null, txn, store), right? That looks unnatural to me, but it´s ok :) 

> 
> @@ +218,5 @@
> > +      let txn = db.transaction([STORE_NAME], txn_type);
> > +      if (DEBUG) debug("Retrieving object store", STORE_NAME);
> > +      let store = txn.objectStore(STORE_NAME);
> > +      txn.oncomplete = oncompleteCb;
> > +      txn.onerror = onerrorCb;
> 
> Please attach these in the individual methods, like so:
> 
>   newTxn(..., function (txn, store) {
>     txn.oncomplete = function oncomplete() {
>       ...
>     };
>     txn.onerror = function onerror() {
>       ...
>     };
>   });
> 
> Makes it much easier to read and understand what's going on.
Ok.

> @@ +343,5 @@
> > +    //      obtention and it is definitely sorted.
> > +    let filteredKeys = [];
> > +    // We need to apply the searches according to all the parameters of the
> > +    // filter. filterCount will decrease with each of this searches.
> > +    let filterCount = 4;
> 
> Where does this number come from? It should probably be a global const with
> a nice descriptive name!
Ok.

> @@ +349,5 @@
> > +    let successCb = function (event) {
> > +      let result = event.target.result;
> > +      // Once the cursor has retrieved all keys that matches its key range,
> > +      // the filter search is done and filterCount is decreased.
> > +      if (!!result == false) {
> 
> if (!result)
> 
> @@ +427,5 @@
> > +          filterCount -= 2;
> > +        }
> > +      }, function (event) {
> > +        if (filterCount == 0) {
> > +          if (filteredKeys == 0) {
> 
> Huh? I thought `filteredKeys` was an array? Do you mean `if
> (!filteredKeys.length)`?
Yes, that´s it :)
 
> @@ +434,5 @@
> > +          }
> > +          // We need to get rid off the duplicated keys.
> > +          let result = [];
> > +          for (let i = 0; i < filteredKeys.length; i++ ) {
> > +            if ( result.indexOf( filteredKeys[i], 0, filteredKeys ) < 0 ) {
> 
> Nit: no space after ( and before ).
Ok. 

I´ll be uploading a new WIP patch addressing your corrections soon.

Thanks!

Comment 21

[Philipp von Weitershausen [:philikon]]

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #20)
> > @@ +47,5 @@
> > > +  // message list.
> > > +  let _keys = Object.create(null);
> > > +
> > > +  // Public methods for managing the message lists.
> > > +  return {
> > 
> > The closure pattern means we're indenting a lot here, just to keep the
> > `_keys` object private. That seems unnecessary to me.
> > 
> So, there is no problem for making _keys public?

Nope. XPCOM will ensure that only attributes defined in nsISmsDatabaseService are available to other code. We don't have to do anything special to protect our data structures.

> > @@ +210,5 @@
> > > +  newTxn: function newTxn(txn_type, callback, oncompleteCb, onerrorCb) {
> > > +    this.ensureDB(function (error, db) {
> > > +      if (error) {
> > > +        if (DEBUG) debug("Could not open database: " + error);
> > > +        callback(null, null, error);
> > 
> > Let's be consistent about the placement of the 'error' parameter. A common
> > convention is to have it be the first one.
> Ok, then the success case must be called as callback(null, txn, store),
> right? That looks unnatural to me, but it´s ok :) 

Right. It may look awkward, but error first is a pretty common convention.

Comment 22

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v5

Here it goes another WIP. Still need to test it and do some clean up.

Comment 23

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599009 [details] [diff] [review]
WIP v5

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

Nice, epic patch! Lots of nits and little bugs to fix still, but we're getting there.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const SMS_DATABASE_SERVICE_CONTRACTID = "@mozilla.org/sms/smsdatabaseservice;1";

So, I think in order to make this work with the SmsServiceFactory, we should pick a different contract ID, e.g.

  @mozilla.org/sms/rilsmsdatabaseservice;1

@@ +11,5 @@
> +
> +const SMS_DATABASE_SERVICE_CONTRACTID = "@mozilla.org/sms/smsdatabaseservice;1";
> +const SMS_DATABASE_SERVICE_CID = Components.ID("{2454c2a1-efdd-4d96-83bd51a29a21f5ab}");
> +
> +const DEBUG = true;

Don't forget to flip this to `false` in the final patch!

@@ +20,5 @@
> +// Number of filters contained within the nsIDOMMozSmsFilter object
> +// passed to createMessageList. As the searches are done asynchronouly,
> +// we need to get the count of applied filters. When all the filters are
> +// applied, the intersection of the IDBKeys retrieved is done and stored
> +// in memory as a new message list.

"IDBKeys" makes it sound like it's a rich object whereas it's just an array of primary keys. Please just say "primary keys".

@@ +33,5 @@
> +                                   "nsISmsRequestManager");
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> +                                   "@mozilla.org/uuid-generator;1",
> +                                   "nsIUUIDGenerator");

SMS ids are just longs, not UUIDs. Please do s/uuid/id/g throughout the whole file.

@@ +40,5 @@
> + * MessagesListManager
> + *
> + * This object keeps a list of IDBKey arrays to iterate over messages lists
> + * and provides the functions to manage the insertion and deletion of arrays
> + */

Same comment re: IDBKey.

Also, the 2nd part of the sentence doesn't make much sense to me. Insertion and deletion of which arrays? It seems to me that sentence is superfluous.

@@ +41,5 @@
> + *
> + * This object keeps a list of IDBKey arrays to iterate over messages lists
> + * and provides the functions to manage the insertion and deletion of arrays
> + */
> +let MessagesListManager = {

So why is this a separate object? It's just a fancy hash table. Seems to me that this functionality can easily live in SmsDatabaseManager.

@@ +49,5 @@
> +  /**
> +   * Add a list to the manager.
> +   *
> +   * @param keys
> +   *        Object containing a list of IDBKeys as Object properties.

Same comment re: IDBKeys.

Also, I think this is wrong. `keys` is just an array, no? Please fix.

@@ +55,5 @@
> +   * @return the id of the list.
> +   */
> +  add: function add(keys) {
> +    // Generate the message list uuid.
> +    let uuid = gUUIDGenerator.generateUUID().toString();

Message list IDs are longs, not UUIDs.

@@ +72,5 @@
> +   */
> +  get: function get(uuid) {
> +    if (this._keys[uuid]) {
> +      return this._keys[uuid];
> +    }

More elegantly:

  let keys = this._keys[id];
  if (keys) {
    return keys;
  }

@@ +89,5 @@
> +  getNextInList: function getNextInList(uuid) {
> +    if (this._keys[uuid]) {
> +      //TODO do we want to keep the list content?
> +      return this._keys[uuid].shift();
> +    }

Get rid of the TODO comment and apply elegance mentioned above.

@@ +117,5 @@
> +/**
> + * SmsDatabaseService
> + */
> +function SmsDatabaseService() {
> +  (function getUUID() {

Why this a private function? Seems useless to me, it just adds indentation. Just put this stuff directly in the constructor, please.

@@ +132,5 @@
> +      request.onsuccess = function onsuccess(event) {
> +        let result = event.target.result;
> +        if (!result) {
> +          if (DEBUG) debug("Could not get the last key from sms database. " +
> +                           "Probably empty database");

If it spawns two lines, please use braces.

@@ +135,5 @@
> +          if (DEBUG) debug("Could not get the last key from sms database. " +
> +                           "Probably empty database");
> +          return;
> +        }
> +        that.lastIndex = event.target.result.key;

You called it `lastKey` below.

@@ +139,5 @@
> +        that.lastIndex = event.target.result.key;
> +        if (!that.lastIndex) {
> +          if (DEBUG) debug("Could not get the last key from sms database");
> +          return;
> +        }

This `if` block seems useless. We're at the end of the function anyway. I think what we should do is simply:

  this.lastKey = event.target.result.key || 0;

to make sure we fall back to an integer always. If you want, you can log it, but no need to do anything else IMHO.

@@ +269,5 @@
> +  },
> +
> +  // nsISmsDatabaseService
> +
> +  //TODO this method should not be synchronous (bug 720653)

Remove this comment, bug 720653 has been morphed.

@@ +270,5 @@
> +
> +  // nsISmsDatabaseService
> +
> +  //TODO this method should not be synchronous (bug 720653)
> +  //TODO need to save incoming SMS, too!

I filed bug 729042 for this, please refer to it in this comment.

@@ +303,5 @@
> +    this.newTxn(IDBTransaction.READ_ONLY, function (error, txn, store) {
> +      if (error) {
> +        if (DEBUG) debug(error);
> +        gSmsRequestManager.notifyGetSmsFailed(requestId,
> +                                              eInternalError);

Where is `eInternalError` coming from? I think you mean Ci.nsISmsRequestManager.INTERNAL_ERROR? Please fix here and everywhere else (also the other error constants!)

@@ +329,5 @@
> +          gSmsRequestManager.notifyGetSmsFailed(requestId, eUnkwownError);
> +          return;
> +        }
> +        gSmsRequestManager.notifyGotSms(requestId,
> +                                        data);

notifyGotSms(requestId, message) takes an nsISmsMessage object for the 2nd argument. So you want to use gSmsService.createSmsMessage() first and then pass the result to notifyGotSms().

@@ +357,5 @@
> +      };
> +
> +      txn.oncomplete = function oncomplete(event) {
> +        //TODO check number of deleted sms
> +        //TODO check notifySmsDeleted interface

I don't understand these TODOs. If they don't apply anymore, please remove. If they do, please file bugs for them and refer to the bug no. here.

@@ +362,5 @@
> +        //TODO we probably want to remove the boolean parameter from
> +        // notifySmsDeleted. It just indicates if the deletion count was 1,
> +        // but in case that it is more than 1, we may want to notify via
> +        // notifySmsDeleteFailed, not using the bool param. At least, 
> +        // that´s how android WebSMS code does.

Please file a bug for this and remove this comment.

@@ +377,5 @@
> +
> +//The message list stuff could be elegantly implemented using IDB cursors,
> +//except we'd need to keep the txn open, so maybe not such a good idea
> +//(unless we find a way to queue other requests while a list is being
> +//processed, but that sounds messy).

Please remove this.

@@ +390,5 @@
> +    // TODO not sure if this is the best approach for storing the keys...
> +    //      An object make the insertion O(1), but the retrieval of keys
> +    //      would be unsorted. An array has an extra cost of post-insertion as
> +    //      we need to remove duplicated keys, but it has O(1) cost for key
> +    //      obtention and it is definitely sorted.

I would not worry about the performance right now. JS arrays are, in fact, a lot like hash tables. Of course, the JS engines can often optimize them, but I wouldn't try to assume behaviour right now. Best is to profile and tune later. Just remove this comment for now, please. If you want, you can file a bug about profiling the SMS db and tuning it.

(Also, "obtention" is not an English word ;))

@@ +398,5 @@
> +    // TODO we probably need to iterate filter object to get filterCount
> +    let filterCount = FILTER_COUNT;
> +
> +    // Callback function to iterate throw request results via IDBCursor.
> +    let successCb = function (event) {

Name your function please.

@@ +404,5 @@
> +      // Once the cursor has retrieved all keys that matches its key range,
> +      // the filter search is done and filterCount is decreased.
> +      if (!result) {
> +        filterCount--;
> +        return;

"and filterCount is decreased" -- yeah, I can see that. I know how to read code :). Comments are better when they explain the why, not the what.

@@ +412,5 @@
> +      filteredKeys.push(primaryKey);
> +      result.continue();
> +    };
> +
> +    let errorCb = function (event) {

Ditto.

@@ +423,5 @@
> +
> +    // As we need to get the list of keys that match the filter criteria
> +    // sorted by timestamp index, we will split the key obtention in two
> +    // different transactions. One for the timestamp index and another one
> +    // for the rest of indexes to query.

"obtention" is not an English word.

@@ +425,5 @@
> +    // sorted by timestamp index, we will split the key obtention in two
> +    // different transactions. One for the timestamp index and another one
> +    // for the rest of indexes to query.
> +    let self = this;
> +    this.newTxn(IDBTransaction.READ_ONLY,function (error, txn, store) {

Nit: space after the comma.

@@ +444,5 @@
> +        timeRequest = store.index("timestamp").openKeyCursor(timeKeyRange,
> +                                                             IDBCursor.PREV);
> +      } else {
> +        timeRequest = store.index("timestamp").openKeyCursor(timeKeyRange);
> +      }

More elegantly:

  let direction = reverse ? IDBCursor.PREV : IDBCursor.NEXT;
  let timeRequest = store.index("timestamp").openKeyCursor(timeKeyRange, direction);

@@ +472,5 @@
> +          if (filter.numbers) {
> +            // Retrieve the keys from the 'sender' and 'receiver' indexes that match
> +            // the values of filter.numbers
> +            let numberKeyRange = IDBKeyRange.bound(filter.numbers[0],
> +                                                   filter.numbers[filter.numbers.length-1]);

Nit: spaces around operators.

@@ +514,5 @@
> +                txn.oncomplete = function oncomplete(event) {
> +                  let message = event.target.result;
> +                  if (!message) {
> +                    errorCb("Could not retrieve first message in list");
> +                  }

Do you want to return here?

@@ +518,5 @@
> +                  }
> +                  let messageListId = MessagesListManager.add(filteredKeys);
> +                  gSmsRequestManager.notifyCreateMessageList(requestId,
> +                                                             messageListId,
> +                                                             message);

Same as above with notifyGotSms(): notifyCreateMessageList()  takes an nsISmsMessage object for the 3rd argument. Use gSmsService.createSmsMessage() to create a message object first.

@@ +529,5 @@
> +              });
> +            } else {
> +              errorCb("There are filters left to apply.");
> +              return;
> +            }

Please make this `else` an `if (!filterCount)` at the top. Then you don't need to indent all the stuff that's currently in the `if (filterCount == 0)` block (bail out early style).

@@ +577,5 @@
> +        txn.oncomplete = function oncomplete(event) {
> +          let message = event.target.result;
> +          if (message) {
> +            gSmsRequestManager.notifyGotNextMessage(requestId,
> +                                                    message);

Same as above: `message` needs to be an `nsISmsMessage` object.

::: dom/sms/src/ril/SmsDatabaseService.manifest
@@ +1,2 @@
> +component {2454c2a1-efdd-4d96-83bd51a29a21f5ab}
> +contract @mozilla.org/sms/smsdatabaseservice;1 {2454c2a1-efdd-4d96-83bd51a29a21f5ab}

As said before, in order to make this work with the SmsServiceFactory, we should pick a different contract ID, e.g.

  @mozilla.org/sms/rilsmsdatabaseservice;1

Also, we need to register ourselves with the category manager to be instantiated after startup:

  category profile-after-change SmsDatabaseService service,@mozilla.org/sms/rilsmsdatabaseservice;1

Comment 24

[Philipp von Weitershausen [:philikon]]

Attachment: WIP v5 w/o boilerplate

This is WIP v5 but without the XPCOM boilerplate that has moved to bug 729104.

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Philipp von Weitershausen [:philikon] from comment #23)
> Comment on attachment 599009 [details] [diff] [review]
> WIP v5
> 
> Review of attachment 599009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice, epic patch! Lots of nits and little bugs to fix still, but we're
> getting there.
> 
> ::: dom/sms/src/ril/SmsDatabaseService.js
> @@ +8,5 @@
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +const SMS_DATABASE_SERVICE_CONTRACTID = "@mozilla.org/sms/smsdatabaseservice;1";
> 
> So, I think in order to make this work with the SmsServiceFactory, we should
> pick a different contract ID, e.g.
> 
>   @mozilla.org/sms/rilsmsdatabaseservice;1
> 
Ok, still needs to learn a lot about XPCOM :|

> @@ +11,5 @@
> > +
> > +const SMS_DATABASE_SERVICE_CONTRACTID = "@mozilla.org/sms/smsdatabaseservice;1";
> > +const SMS_DATABASE_SERVICE_CID = Components.ID("{2454c2a1-efdd-4d96-83bd51a29a21f5ab}");
> > +
> > +const DEBUG = true;
> 
> Don't forget to flip this to `false` in the final patch!
Yep, in fact, I was thinking about leave it as `undefined`, to avoid overriding this value outside the current scope.

> @@ +33,5 @@
> > +                                   "nsISmsRequestManager");
> > +
> > +XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> > +                                   "@mozilla.org/uuid-generator;1",
> > +                                   "nsIUUIDGenerator");
> 
> SMS ids are just longs, not UUIDs. Please do s/uuid/id/g throughout the
> whole file.
Hmmm... yes. I was using UUIDs for the ids of each list within MessagesListManager. But I´ve just realized that the list ids are also longs, same as message ids.
 
> Also, the 2nd part of the sentence doesn't make much sense to me. Insertion
> and deletion of which arrays? It seems to me that sentence is superfluous.
>
Well, that´s probably not make much sense because of my perfect English :) I´ll just remove that sentence.

> @@ +41,5 @@
> > + *
> > + * This object keeps a list of IDBKey arrays to iterate over messages lists
> > + * and provides the functions to manage the insertion and deletion of arrays
> > + */
> > +let MessagesListManager = {
> 
> So why is this a separate object? It's just a fancy hash table. Seems to me
> that this functionality can easily live in SmsDatabaseManager.
>
No reason, just for clarity, I´ll merge it into SmsDatabaseManager anyway.
> @@ +89,5 @@
> > +  getNextInList: function getNextInList(uuid) {
> > +    if (this._keys[uuid]) {
> > +      //TODO do we want to keep the list content?
> > +      return this._keys[uuid].shift();
> > +    }
> 
> Get rid of the TODO comment and apply elegance mentioned above.
>
Sure. So we won´t keep the list :)
 
> @@ +135,5 @@
> > +          if (DEBUG) debug("Could not get the last key from sms database. " +
> > +                           "Probably empty database");
> > +          return;
> > +        }
> > +        that.lastIndex = event.target.result.key;
> 
> You called it `lastKey` below.
> 
How could you notice this kind of things...?

> @@ +303,5 @@
> > +    this.newTxn(IDBTransaction.READ_ONLY, function (error, txn, store) {
> > +      if (error) {
> > +        if (DEBUG) debug(error);
> > +        gSmsRequestManager.notifyGetSmsFailed(requestId,
> > +                                              eInternalError);
> 
> Where is `eInternalError` coming from? I think you mean
> Ci.nsISmsRequestManager.INTERNAL_ERROR? Please fix here and everywhere else
> (also the other error constants!)
>
Yes, more XPCOM lack of knowledge. Thanks.
 
> @@ +357,5 @@
> > +      };
> > +
> > +      txn.oncomplete = function oncomplete(event) {
> > +        //TODO check number of deleted sms
> > +        //TODO check notifySmsDeleted interface
> 
> I don't understand these TODOs. If they don't apply anymore, please remove.
> If they do, please file bugs for them and refer to the bug no. here.
> 
This TODOs are related to the bool parameter that is passed to notifySmsDelete. As far as I can see Android backend (https://hg.mozilla.org/mozilla-central/file/4038ffaa5d82/embedding/android/GeckoSmsManager.java#l723) checks if more than one sms has been deleted, in that case it throws an exception and calls GeckoAppShell.notifySmsDeleteFailed. So I guess that bool is always true in notifySmsDelete, right? (@Mounir?)

> @@ +362,5 @@
> > +        //TODO we probably want to remove the boolean parameter from
> > +        // notifySmsDeleted. It just indicates if the deletion count was 1,
> > +        // but in case that it is more than 1, we may want to notify via
> > +        // notifySmsDeleteFailed, not using the bool param. At least, 
> > +        // that´s how android WebSMS code does.
> 
> Please file a bug for this and remove this comment.
> 
Ok, that is what I was talking about before. Filling a bug now.

> (Also, "obtention" is not an English word ;))
> 
:D My english is similar to my XPCOM knowledge... near to null.
 
> ::: dom/sms/src/ril/SmsDatabaseService.manifest
> @@ +1,2 @@
> > +component {2454c2a1-efdd-4d96-83bd51a29a21f5ab}
> > +contract @mozilla.org/sms/smsdatabaseservice;1 {2454c2a1-efdd-4d96-83bd51a29a21f5ab}
> 
> As said before, in order to make this work with the SmsServiceFactory, we
> should pick a different contract ID, e.g.
> 
>   @mozilla.org/sms/rilsmsdatabaseservice;1
> 
> Also, we need to register ourselves with the category manager to be
> instantiated after startup:
> 
>   category profile-after-change SmsDatabaseService
> service,@mozilla.org/sms/rilsmsdatabaseservice;1
Ok.

Thanks!

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

> > > +      };
> > > +
> > > +      txn.oncomplete = function oncomplete(event) {
> > > +        //TODO check number of deleted sms
> > > +        //TODO check notifySmsDeleted interface
> > 
> > I don't understand these TODOs. If they don't apply anymore, please remove.
> > If they do, please file bugs for them and refer to the bug no. here.
> > 
> This TODOs are related to the bool parameter that is passed to
> notifySmsDelete. As far as I can see Android backend
> (https://hg.mozilla.org/mozilla-central/file/4038ffaa5d82/embedding/android/
> GeckoSmsManager.java#l723) checks if more than one sms has been deleted, in
> that case it throws an exception and calls
> GeckoAppShell.notifySmsDeleteFailed. So I guess that bool is always true in
> notifySmsDelete, right? (@Mounir?)
> 
> > @@ +362,5 @@
> > > +        //TODO we probably want to remove the boolean parameter from
> > > +        // notifySmsDeleted. It just indicates if the deletion count was 1,
> > > +        // but in case that it is more than 1, we may want to notify via
> > > +        // notifySmsDeleteFailed, not using the bool param. At least, 
> > > +        // that´s how android WebSMS code does.
> > 
> > Please file a bug for this and remove this comment.
> > 
> Ok, that is what I was talking about before. Filling a bug now.

After talking with Mounir, there is not need to fill a bug for this. My fault. The boolean param can be null if count == 0.
Anyway, we need to check how many records has the deletion affected and AFAIK, the only way to do that with IDB is to try to get the record again.

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v6

Here it goes another WIP trying to address all the feedback provided in the previous review.

Comment 28

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599549 [details] [diff] [review]
WIP v6

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

Nice. Just a few more nits.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +11,4 @@
>  const RIL_SMSDATABASESERVICE_CONTRACTID = "@mozilla.org/sms/rilsmsdatabaseservice;1";
>  const RIL_SMSDATABASESERVICE_CID = Components.ID("{a1fa610c-eb6c-4ac2-878f-b005d5e89249}");
>  
> +const DEBUG;

= false

@@ +76,5 @@
> +  /**
> +   * This object keeps the message lists associated with each search. Each
> +   * message list is stored as an array of primary keys.
> +   */
> +  messageLists: { length: 0, primaryKeys: Object.create(null) },

This is not a good idea because these objects would be shared between all instances of SmsDatabaseService. Now, I know that SmsDatabaseService is only going to be a singleton, but I'd still say it's bad practice.

Initialize these in the constructor.

@@ +81,5 @@
> +
> +  /**
> +   * Last key value stored in the database.
> +   */
> +  lastKey: 1,  

Why not 0?

@@ +246,5 @@
> +    this.messageLists = {
> +      length: 0,
> +      primaryKeys: {}
> +    };
> +  },

This seems to be unused and it's not in nsISmsDatabaseService. Remove it?

If you don't want to remove it, please make it reuse the `this.messageList` object and make `primaryKeys` an `Object.create(null)` as well.

@@ +403,5 @@
>    },
>  
> +  createMessageList: function createMessageList(filter,
> +                                                reverse,
> +                                                requestId) {

No need to wrap this line if it fits into 80 chars.

@@ +461,5 @@
> +                                                               direction);
> +      timeRequest.onsuccess = successCb;
> +      timeRequest.onerror = errorCb;
> +
> +      txn.oncomplete = function oncomplete(event) {

We should do the whole search in one transaction and not split it over multiple ones. In the interest of moving forward, this is ok. But please file a follow-up bug for this now and refer to it in a comment here.

@@ +564,4 @@
>    },
>  
> +  getNextMessageInList: function getNextMessageInList(listId,
> +                                                      requestId) {

Ditto about wrapping the line.

@@ +621,4 @@
>    },
>  
>    clearMessageList: function clearMessageList(listId) {
> +    this.removeMessageList(listId);

Inline `removeMessageList` here.

Comment 29

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599549 [details] [diff] [review]
WIP v6

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

Nice. Just a few more nits.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +11,4 @@
>  const RIL_SMSDATABASESERVICE_CONTRACTID = "@mozilla.org/sms/rilsmsdatabaseservice;1";
>  const RIL_SMSDATABASESERVICE_CID = Components.ID("{a1fa610c-eb6c-4ac2-878f-b005d5e89249}");
>  
> +const DEBUG;

= false

@@ +35,5 @@
>   * SmsDatabaseService
>   */
>  function SmsDatabaseService() {
> +  let that = this;
> +  this.newTxn(IDBTransaction.READ_ONLY, function(error, txn, store){

You need to refer to IDBTransaction as Ci.nsIIDBTransaction here because it's not available through the global scope in XPCOM components. Please do that for all other IDB* interfaces as well!

@@ +76,5 @@
> +  /**
> +   * This object keeps the message lists associated with each search. Each
> +   * message list is stored as an array of primary keys.
> +   */
> +  messageLists: { length: 0, primaryKeys: Object.create(null) },

This is not a good idea because these objects would be shared between all instances of SmsDatabaseService. Now, I know that SmsDatabaseService is only going to be a singleton, but I'd still say it's bad practice.

Initialize these in the constructor.

@@ +81,5 @@
> +
> +  /**
> +   * Last key value stored in the database.
> +   */
> +  lastKey: 1,  

Why not 0?

@@ +246,5 @@
> +    this.messageLists = {
> +      length: 0,
> +      primaryKeys: {}
> +    };
> +  },

This seems to be unused and it's not in nsISmsDatabaseService. Remove it?

If you don't want to remove it, please make it reuse the `this.messageList` object and make `primaryKeys` an `Object.create(null)` as well.

@@ +403,5 @@
>    },
>  
> +  createMessageList: function createMessageList(filter,
> +                                                reverse,
> +                                                requestId) {

No need to wrap this line if it fits into 80 chars.

@@ +461,5 @@
> +                                                               direction);
> +      timeRequest.onsuccess = successCb;
> +      timeRequest.onerror = errorCb;
> +
> +      txn.oncomplete = function oncomplete(event) {

We should do the whole search in one transaction and not split it over multiple ones. In the interest of moving forward, this is ok. But please file a follow-up bug for this now and refer to it in a comment here.

@@ +564,4 @@
>    },
>  
> +  getNextMessageInList: function getNextMessageInList(listId,
> +                                                      requestId) {

Ditto about wrapping the line.

@@ +621,4 @@
>    },
>  
>    clearMessageList: function clearMessageList(listId) {
> +    this.removeMessageList(listId);

Inline `removeMessageList` here.

Comment 30

[Philipp von Weitershausen [:philikon]]

Gee thx splinter. The only thing I meant to add was this:

@@ +35,5 @@
>   * SmsDatabaseService
>   */
>  function SmsDatabaseService() {
> +  let that = this;
> +  this.newTxn(IDBTransaction.READ_ONLY, function(error, txn, store){

You need to refer to IDBTransaction as Ci.nsIIDBTransaction here because it's not available through the global scope in XPCOM components. Please do that for all other IDB* interfaces as well!

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v7

Comment 32

[Philipp von Weitershausen [:philikon]]

Another thing: mozIndexedDB won't be available in the global scope unless we actually explicitly put it there. To put it there, we should do this:

  XPCOMUtils.defineLazyServiceGetter(this, "gIDBManager",
                                     "@mozilla.org/dom/indexeddb/manager;1",
                                     "nsIIndexedDatabaseManager");

  const GLOBAL_SCOPE = this;

  function SmsDatabaseService() {
    gIDBManager.initWindowless(GLOBAL_SCOPE);
    // use GLOBAL_SCOPE.mozIndexedDB below.
  }

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

One more error: DELIVERY_SENT, DELIVERY_RECEIVED not defined.

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v8

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP V9

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Includes changes to fix [JavaScript Error: "uncaught exception: [Exception... "The object could not be cloned."  code: "25" nsresult: "0x80530019 (NS_ERROR_DOM_DATA_CLONE_ERR)"  location: "jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js Line: 248"]"]

(In reply to Vicamo Yang from comment #35)
> Created attachment 599634 [details] [diff] [review]
> WIP V9

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part2: hook RadioInterfaceLayer

Comment 38

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599642 [details] [diff] [review]
part2: hook RadioInterfaceLayer

Nice!

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

Thanks Vicamo!
I still need to review the createMessageList function, as I am not comfortable with the current implementation.

Comment 40

[Vicamo Yang [:vicamo][:vyang]]

Then I'll rebuild with your new patch and see if I should adjust something ;)

Comment 41

[Philipp von Weitershausen [:philikon]]

Comment on attachment 599634 [details] [diff] [review]
WIP V9

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +274,5 @@
> +                                               sender,
> +                                               null,
> +                                               body,
> +                                               date);
> +    return this.saveMessage(message);

Umm, this is wrong, actually. We don't store nsISmsMessage objects. We store plain JS objects and then create nsISmsMessage objects from them.

Comment 42

[Philipp von Weitershausen [:philikon]]

Attachment: WIP v10

Several bugs as well as stylistical nits addressed.

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v11

Timestamp issue fixed. Also final interesection on createMessageList modified. Still need to test deeper.

Comment 44

[Fernando Jiménez Moreno [:ferjm]]

Sorry, the previous patch os incomplete. So me filters won't give expected results. Working on a new one

Comment 45

[Fernando Jiménez Moreno [:ferjm]]

Sorry, the previous patch is incomplete. Some filters won't give expected results. Working on a new one.

Comment 46

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v12

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v13

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v14

One more WIP

Comment 49

[Philipp von Weitershausen [:philikon]]

Comment on attachment 601322 [details] [diff] [review]
WIP v14

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +85,5 @@
> +  /**
> +   * This object keeps the message lists associated with each search. Each
> +   * message list is stored as an array of primary keys.
> +   */
> +  let messageLists = { length: 0, primaryKeys: Object.create(null) };

Um, you want to assign this to `this.messageLists`, not a local variable. For clarity there should also be a `messageList: null` placeholder in the prototype (e.g. like `db: null`).

@@ +401,3 @@
>    },
>  
>    createMessageList: function createMessageList(filter, reverse, requestId) {

I'm not too happy with this method still:

* r- on using multiple transactions.
* It's way too big, partially due to repetition, partially due to the fact that it does two things: compile the initial message list and fetch the first message. That should be split in at least two separate methods.
* There are still some crucial flaws in the query logic (see below).

@@ +428,5 @@
> +      // The cursor primaryKey is stored in its corresponding partial array
> +      // according to the filter parameter.
> +      let primaryKey = result.primaryKey;
> +      switch(filter) {
> +        case FILTER_TIMESTAMP:

Repetition alert! Please use a mapping object instead of a big switch statement.

@@ +527,5 @@
> +          if (filter.numbers) {
> +            // Retrieve the keys from the 'sender' and 'receiver' indexes that
> +            // match the values of filter.numbers
> +            let numberKeyRange = Ci.nsIIDBKeyRange.bound(
> +              filter.numbers[0], filter.numbers[filter.numbers.length - 1]);

What? That makes no sense. You need to query for each individual number and then union the results.

@@ +556,5 @@
> +              result = numbersKeys.filter(function(i) result.indexOf(i) != -1);;
> +            }
> +            if (deliveryKeys.length || filter.delivery) {
> +              result = deliveryKeys.filter(function(i) result.indexOf(i) != -1);
> +            }

Repetition alert: please use a helper, e.g. "arrayIntersection" or similar.

@@ +671,5 @@
> +        };
> +      });
> +    } else {
> +      gSmsRequestManager.notifyNoMessageInList(requestId);
> +    }

Bail out early style, please.

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v15

I´ve finally did the createMessageList on a single transaction.

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v15

Sorry, the previous patch had a typo.

Comment 52

[Philipp von Weitershausen [:philikon]]

Attachment: tests WIP 1

Here's a start for some xpcshell tests. This is by far not done. I think we want two kinds of tests:

(a) tests that verify the data was written to the DB (white box)
(b) tests that install a fake nsISmsRequestManager and just exercise the nsISmsDatabaseService API (black box)

Also we should disable this test by default since it only works against the RIL backend implementation. Or, if we can, gate it on MOZ_B2G_RIL or whatever.

Comment 53

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v16

Another WIP still need to properly test it within gecko. I´ll try to write some more tests tomorrow.

Comment 54

[Philipp von Weitershausen [:philikon]]

Attachment: tests WIP 2

More tests, now with a fake nsISmsRequestManager implementation.

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v17

Comment 56

[Fernando Jiménez Moreno [:ferjm]]

Attachment: tests WIP 3

In order to make this tests work, you first need to apply Bug 732414.
createMessageList is failing.

Comment 57

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP v18

Comment 58

[Fernando Jiménez Moreno [:ferjm]]

Attachment: tests WIP 4

Comment 59

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603068 [details] [diff] [review]
WIP v18

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

This is getting close to being ready. There are still some follow-up issues to be looked at (I filed those), but let's get this thing out the door.

I have addressed the review comments below locally for a quicker review cycle. I'll upload a final patch shortly and land this.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +46,5 @@
> +  let that = this;
> +  this.newTxn(Ci.nsIIDBTransaction.READ_ONLY, function(error, txn, store){
> +    if (error) {
> +      if (DEBUG) debug(error);
> +      return;

No need for the debug() call here, newTxn() already logs the error. All we need to do here is bail out.

@@ +74,5 @@
> +      }
> +    };
> +  });
> +
> +  this.messageLists = { length: 0, primaryKeys: Object.create(null) };

So, I don't know what I was smoking, thinking that this.messageLists.length is a sensible solution, because it can easily lead to conflicts.

I think we can just have an ever-increasing integer here.

@@ +221,5 @@
> +   *        Array containing a list of primary keys as Object properties.
> +   *
> +   * @return the id of the list.
> +   */
> +  addMessageList: function addMessageList(keys) {

This method is only ever used in one place and can be inlined.

@@ +236,5 @@
> +   *        Number representing the id of the message list to retrieve
> +   *
> +   * @return Array of keys
> +   */
> +  getMessageList: function getMessageList(id) {

This is unused and can be removed.

@@ +252,5 @@
> +   * @param id
> +   *        Number representing the id of the message list of where to take
> +   *        the next message primary key
> +   */
> +  getNextInList: function getNextInList(id) {

This method is only ever used in one place and can be inlined.

@@ +277,5 @@
> +  keyIntersection: function keyIntersection(keys, filter) {
> +    let result = keys[FILTER_TIMESTAMP];
> +    if (keys[FILTER_NUMBERS].length || filter.numbers) {
> +      result = keys[FILTER_NUMBERS].filter(function(i) {
> +        return result.indexOf(i) != -1

Missing semicolon.

@@ +282,5 @@
> +      });
> +    }
> +    if (keys[FILTER_DELIVERY].length || filter.delivery) {
> +      result = keys[FILTER_DELIVERY].filter(function(i) {
> +        return result.indexOf(i) != -1

Ditto.

@@ +303,5 @@
> +   */
> +  onMessageListCreated: function onMessageListCreated(messageList,
> +                                                      requestId) {
> +    let self = this;
> +    self.newTxn(Ci.nsIIDBTransaction.READ_ONLY, function (error, txn, store) {

Need to check error and call notifyReadMessageListFailed.

@@ +312,5 @@
> +      };
> +
> +      txn.oncomplete = function oncomplete(event) {
> +        if (DEBUG) debug("Transaction " + txn + " completed.");
> +        let message = event.target.result;

Brrzzt. I don't think the event bubbling works so that the request results will be available on the transaction events...

@@ +315,5 @@
> +        if (DEBUG) debug("Transaction " + txn + " completed.");
> +        let message = event.target.result;
> +        if (!message) {
> +          gSmsRequestManager.notifyGetSmsFailed(
> +                    requestId, Ci.nsISmsRequestManager.INTERNAL_ERROR);

I don't think this should be `notifyGetSmsFailed` but `notifyReadMessageListFailed`.

@@ +353,3 @@
>  
>    saveReceivedMessage: function saveReceivedMessage(sender, body, date) {
> +    this.lastKey += 1;

This can be moved to saveMessage()

@@ +363,4 @@
>    },
>  
>    saveSentMessage: function saveSentMessage(receiver, body, date) {
> +    this.lastKey += 1;

Ditto.

@@ +376,5 @@
>    getMessage: function getMessage(messageId, requestId) {
> +    if (DEBUG) debug("Retrieving message with ID " + messageId);
> +    this.newTxn(Ci.nsIIDBTransaction.READ_ONLY, function (error, txn, store) {
> +      if (error) {
> +        if (DEBUG) debug(error);

Again, duplicate logging.

@@ +489,5 @@
> +    // keys for the message list that we are creating.
> +    let filteredKeys = {
> +      "timestamp": [],
> +      "numbers": [],
> +      "delivery": []

Since you define these names as consts already, we should use them.

@@ +521,5 @@
> +
> +    let self = this;
> +    this.newTxn(Ci.nsIIDBTransaction.READ_ONLY, function (error, txn, store) {
> +      if (error) {
> +        if (DEBUG) debug(error);

Again, duplicate logging.

@@ +532,5 @@
> +      let timeKeyRange = null;
> +      if (!filter.startDate != null && filter.endDate != null) {
> +        timeKeyRange = IDBKeyRange.bound(filter.startDate.getTime(),
> +                                         filter.endDate.getTime());
> +      } else if (filter.starDate != null) {

Typo: starDate

Needs tests :)

@@ +609,5 @@
> +      if (DEBUG) debug("Wrong list id or empty list");
> +      gSmsRequestManager.notifyReadMessageListFailed(
> +        requestId, Ci.nsISmsRequestManager.NOT_FOUND_ERROR);
> +      return;
> +    }

This gets it wrong for the end of the list. There we should be notifying `notifyNoMessageInList`, so we should distinguish between "no such list" and "no more messages in this list".

@@ +624,5 @@
> +          return;
> +        }
> +        if (DEBUG) debug("Could not get message with key " + key);
> +        gSmsRequestManager.notifyReadMessageListFailed(
> +          requestId, Ci.nsISmsRequestManager.NOT_FOUND_ERROR);

We're already notifying this error in the transaction oncomplete handler. No need to do it twice (could even be harmful.)

@@ +635,5 @@
> +      };
> +
> +      txn.oncomplete = function oncomplete(event) {
> +        if (DEBUG) debug("Transaction " + txn + " completed.");
> +        let result = event.target.result;

Same as above: I don't think this will work, the result will be on the request success event.

Comment 60

[Philipp von Weitershausen [:philikon]]

Attachment: final v19

Comment 61

[Philipp von Weitershausen [:philikon]]

Attachment: tests v5

Comment 62

[Philipp von Weitershausen [:philikon]]

I pushed the v19 patch as well as Vicamo's RIL hookup:

https://hg.mozilla.org/mozilla-central/rev/b66d90efe07f
https://hg.mozilla.org/mozilla-central/rev/c1e5daa36ab2

I opted out of pushing the tests as is (even disabled), since Mounir suggested to convert them to mochitests which seemed sensible to me. Let's deal with the tests in bug 733265.

Comment 63

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/b44a23917a99