If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Need a speedy way to construct a thread list for SMS messages

RESOLVED FIXED in Firefox 18

Status

()

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

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

unspecified
mozilla19
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 679420 [details] [diff] [review]
Test patch

We need a quick way to construct the SMS thread list. Currently the app has to download *all* messages and sort them before displaying threads.
blocking-basecamp: --- → ?
(In reply to ben turner [:bent] from comment #0)
> Created attachment 679420 [details] [diff] [review]
> Test patch
> 
> We need a quick way to construct the SMS thread list. Currently the app has
> to download *all* messages and sort them before displaying threads.

This is not true. You can use SmsFilter to specify filters to apply when getting messages. Building a thread that way shouldn't require you to get *all* messages. You should be able to build such thread in two queries: the first one to get messages from A to B and the second to get messages from B to A.
Is that what you are trying to do? Is that slow? The only difference between doing those two requests and the method you are suggesting should be that the method would use only one request. I have a hard time believing that having two requests instead of one is crazily slower.
No, we're talking about the initial thread list when the SMS app starts up.
(In reply to ben turner [:bent] from comment #2)
> No, we're talking about the initial thread list when the SMS app starts up.

The SMS app should be able to keep some (all?) messages in its own idb. Especially for something as easy as "the last message received from person X or sent to person X" given that the SMS app sends and receive all messages.
No, what we're trying to do is create a view which shows you all the people that you have sent or received messages from. And for each of those people show the latest sent/received message as well as an indicator showing if there are *any* unread messages to/from that person.

So basically we need to do the following queries:
* A list of phone numbers that there are messages to/from.
* For each of those numbers, the last message to/from that number.
* For each of those numbers, are there any unread messages to/from that number.

The first one in particular is not possible to do in a reasonable way with the current API. The other two you can be done but requires a separate query for each number.


My sense here is that the "real" way of doing this API is to add enough capabilities to the SMS API that the app can keep its own cache of whatever it wants to keep a cache of. This enables the app to cache whatever it can't query fast enough from the API.

Adding such a callback mechanism is something that we should look at as soon as we have time. It'll be easy to drop this new temporary API at that time given that we only expose it to certified apps.
I feel like we are trying to solve a frontend issue with a backend fix.

What we want is a list of objects like:
{
  with: "Mum,
  lastMessage: {
    fromMe: false,
    body: "Hi"
  },
  numberUnread: 1,
}

Creating this list from scratch would cost a lot indeed, as you said. But updating it is very cheap.

I think we just need to handle those cases:
- a message is received from the contact, we update |lastMessage| with the information, we increment |numberUnread|;
- a message is sent to that contact, we update |lastMessage| with the information.
- a thread is deleted, we remove the related item from the list,
- a thread is read, we set |numberUnread| to 0,
- we receive or send a message from a contact that isn't present in the list, we create the appropriate object.

All those situations (sending, receiving, deleting, reading) are whether known by the app or actioned by the app. So, I think implementing that behaviour in the frontend wouldn't be too hard for someone who wrote the app.

Also, creating the list from scratch would have to happen only once and at the first startup of the app (or after a wipe of the app database) which means most of the case, creating that list would be very fast.
Created attachment 680285 [details] [diff] [review]
Patch, v1

This seems to work.
Attachment #679420 - Attachment is obsolete: true
Comment on attachment 680285 [details] [diff] [review]
Patch, v1

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

r=me, but we should talk with mounir too.

::: dom/sms/src/SmsRequest.cpp
@@ +542,5 @@
> +    // Gross, but StringToJsval wants a non-const string here.
> +    ThreadListItem& source = const_cast<ThreadListItem&>(aItems[i]);
> +
> +    jsval senderOrReceiver;
> +    ok = xpc::StringToJsval(cx, source.senderOrReceiver(), &senderOrReceiver);

Please create a temporary nsString here.

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +190,5 @@
> +              event.target.transaction.abort();
> +              callback("Old database version: " + event.oldVersion, null);
> +              break;
> +          }
> +          currentVersion++;

Please revert to the way it was before.

If you want to not worry about the extra index, just don't create it in createSchema and delete it conditionally later.

@@ +283,5 @@
> +     *
> +     */
> +    objectStore = db.createObjectStore(MOST_RECENT_STORE_NAME,
> +                                       { keyPath: "senderOrReceiver" });
> +    objectStore.createIndex("timestamp", "timestamp");

Revert this too

@@ +313,5 @@
>  
> +  upgradeSchema3: function upgradeSchema2(transaction) {
> +    // Delete redundant "id" index.
> +    let objectStore = transaction.objectStore(STORE_NAME);
> +    objectStore.deleteIndex("id");

And make this conditional on the index existing.
Attachment #680285 - Flags: review+
Created attachment 680291 [details] [diff] [review]
Patch, v1.1

Comments addressed.
Attachment #680285 - Attachment is obsolete: true
Attachment #680291 - Flags: review+

Updated

5 years ago
blocking-basecamp: ? → +

Comment 9

5 years ago
Lets land this. Frontend work depends on it.

Comment 10

5 years ago
(In reply to Jonas Sicking (:sicking) from comment #4)
> No, what we're trying to do is create a view which shows you all the people
> that you have sent or received messages from. And for each of those people
> show the latest sent/received message as well as an indicator showing if
> there are *any* unread messages to/from that person.
> 
> So basically we need to do the following queries:
> * A list of phone numbers that there are messages to/from.
> * For each of those numbers, the last message to/from that number.
> * For each of those numbers, are there any unread messages to/from that
> number.
> 
> The first one in particular is not possible to do in a reasonable way with
> the current API. The other two you can be done but requires a separate query
> for each number.
I feel that the change I suggested to the SMSFilter API can address the second and third point. Not the first one directly (though it may yield a better result than fetch-all-and-extract).
For reference: https://groups.google.com/forum/?fromgroups=#!topic/mozilla.dev.webapi/0j2yBNtd-n8

> My sense here is that the "real" way of doing this API is to add enough
> capabilities to the SMS API that the app can keep its own cache of whatever
> it wants to keep a cache of. This enables the app to cache whatever it can't
> query fast enough from the API.
I feel functional programming patterns (.filter, .map, .reduce) can help with this kind of issues. Maybe there is some work to do to make these patterns work with async JS.

> Adding such a callback mechanism is something that we should look at as soon
> as we have time. It'll be easy to drop this new temporary API at that time
> given that we only expose it to certified apps.
That's pretty good news :-)
Yeah, I talked this over with mounir and he's fine with adding this as a temporary measure until we have the proper (callback based) solution.

Ben is gone on vacation, but I'll land this unless someone beats me to it.
(In reply to Jonas Sicking (:sicking) from comment #11)
> Yeah, I talked this over with mounir and he's fine with adding this as a
> temporary measure until we have the proper (callback based) solution.

"fine" is quite a strong word. I would say that I'm resigned to the fact that we are adding crazy things to our API that will make our life harder later.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c60518351d
Backout
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db578a8dffa

because of
https://tbpl.mozilla.org/php/getParsedLog.php?id=17002126&tree=Mozilla-Inbound&full=1
Blocks: 808819
I got the emulator to run \o/

E/GeckoConsole(   42): [JavaScript Error: "db is not defined" {file: "jar:file:///system/b2g/omni.ja!/components/SmsDatabaseService.js" line: 311}]
Created attachment 681590 [details] [diff] [review]
followup
Attachment #681590 - Flags: review?(jonas)
Created attachment 681591 [details] [diff] [review]
followup

now the correct patch
Attachment #681590 - Attachment is obsolete: true
Attachment #681590 - Flags: review?(jonas)
Attachment #681591 - Flags: review?(jonas)
Comment on attachment 681591 [details] [diff] [review]
followup

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

Yay tests!
Attachment #681591 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/05d444e54f69
https://hg.mozilla.org/integration/mozilla-inbound/rev/508ecb4a6cb3
https://hg.mozilla.org/mozilla-central/rev/508ecb4a6cb3
https://hg.mozilla.org/mozilla-central/rev/05d444e54f69
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
https://hg.mozilla.org/releases/mozilla-aurora/rev/08e0aa1a2f1e
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d108390dbda
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Blocks: 824467
You need to log in before you can comment on or make changes to this bug.