963475 - Provide a way to fetch the message threads in reverse order

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect, P1)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #947234 +++

As discussed on bug 947234 we need a way to retrieve the message threads starting from the most recent as they are currently provided in the opposite order. I'll post two patches for this and we'll evaluate which one we want to land depending on risk and API considerations:

- The first one just reverses the order in which threads are delivered; since to my knowledge the order is not specified in any documentation this should be OK and is a very low-risk fix (a one line change basically).

- The second one adds a parameter to mozMobileMessage.getThreads() that specifies the direction used for retrieving the threads. This would be more consistent with mozMobileMessage.getMessages() but carries a higher risk and changes the visible API (which this late in the 1.3 timeline doesn't feel right).

Comment 1

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Revert the order of retrieval of message threads

First patch, this is the one-line fix that reverts the order of retrieval. I've quickly tested on my phone and it seems to work fine, more testing is needed though.

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Gabriele Svelto [:gsvelto] from comment #1)
> First patch, this is the one-line fix that reverts the order of retrieval.
                                             ^^^^^^^
                                             I meant reverses

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8364937 [details] [diff] [review]
[PATCH] Revert the order of retrieval of message threads

Hi Vicamo, you should take a look.

Comment 4

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH alt] Add a paremeter to make mozMobileMessage.getThreads() return threads in reverse order

This is the second riskier patch that makes mozMobileMessage.getThreads() take an optional parameter |reverse| that returns threads in younger-to-older order. I've extended the marionette tests to cover this functionality and I'm in the process of testing this.

Not asking for review just yet as I'd like to get feedback first on how to proceed. Note that if we take this patch over the simpler the patch-set in bug 947234 requires a couple of minor changes too.

Comment 5

[Gabriele Svelto [:gsvelto]]

Nom'ing this now that the patches are ready; Fabrice could you tell me how to proceed here since this might require uplifting one of the two patches here to mozilla-b2g28_v1_3. Here's a description of the problem and related possibilities and risks:

In bug 947234 I've prepared a patch-set that displays incrementally the message threads as they come off the cursor, this dramatically speeds up application startup especially with large number of threads (the time to display the first thread in the x-heavy workload goes from 9+ seconds to ~400ms). The downside of this approach is that Gecko currently delivers threads in order starting from the oldest ones; this causes the user to be presented with older threads first which is quite unpleasant.

To move forward we've got three possible options:

- Leave things as they are, land only the gaia patches in bug 947234 and stick with the unpleasant but working behavior. This is obviously the less risky option.

- Take attachment 8364937 [details] [diff] [review] and uplift it to mozilla-b2g28_v1_3, this is a one line fix that reverses the order in which threads are iterated by mozMobileMessage.getThreads() and thus provides the behavior we need in bug 947234 with minimal risk. Changing the visible behavior of that method shouldn't affect other code because AFAIK the SMS app is its only user.

- Take 8365069 and uplift it to mozilla-b2g28_v1_3. This is a more "proper" fix that adds an optional parameter to mozMobileMessage.getThreads() to specify the direction we want to use for iteration. This has the upside of not changing the behavior of the method when called w/o parameters but requires more extensive changes in Gecko. From an API perspective this is more consistent with mozMobileMessage.getMessages() but still means we're changing a public API very late in the 1.3 cycle which feels a bit awkward.

Comment 6

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

Comment on attachment 8364937 [details] [diff] [review]
[PATCH] Revert the order of retrieval of message threads

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

I think it will at least fails "dom/mobilemessage/tests/marionette/test_getthreads.js" because thread ordering is also considered in that test script.  But I think you'll only need a line |checkFuncs.reverse()| to fix it.

Comment 7

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

(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> - Take 8365069 and uplift it to mozilla-b2g28_v1_3. This is a more "proper"
> fix that adds an optional parameter to mozMobileMessage.getThreads() to
> specify the direction we want to use for iteration. This has the upside of
> not changing the behavior of the method when called w/o parameters but
> requires more extensive changes in Gecko. From an API perspective this is
> more consistent with mozMobileMessage.getMessages() but still means we're
> changing a public API very late in the 1.3 cycle which feels a bit awkward.

I think they're quite the same because Gaia has no way (except Gecko version) to detect either API change, so both of them result in an unknown behaviour.  I mean passing an optional 'true' to that API doesn't guarantee that message threads are always returned in reversed order because one might just have a slightly outdated Gecko.  Then it seems to me that preserving current API interface, that is, taking the one liner solution, would be a better idea for me. :)

Comment 8

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

Comment on attachment 8365069 [details] [diff] [review]
[PATCH alt] Add a paremeter to make mozMobileMessage.getThreads() return threads in reverse order

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

But let's take the one liner way, shall we?

::: dom/mobilemessage/tests/marionette/test_getthreads.js
@@ +392,5 @@
>  
> +// Check reverse access.
> +tasks.push(getAllThreads.bind(null, function(threads) {
> +{
> +  is(threads.length, checkFuncs.length, "number of threads got");

should be |reverseCheckFuncs|.

@@ +400,5 @@
> +      tasks.next();
> +      return;
> +    }
> +
> +    checkFuncs.shift()(threads.shift(), callback);

ditto

Comment 9

[Gabriele Svelto [:gsvelto]]

Thanks for the feedback Vicamo, I'm also keener on the one-line patch as it doesn't change the external interface. And yes the marionette tests are broken in both patches. I'm fixing them up right now; unfortunately running tests in the emulator is slooow... :(

Comment 10

[Julien Wajsberg [:julienw]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #7)
> (In reply to Gabriele Svelto [:gsvelto] from comment #5)
> > - Take 8365069 and uplift it to mozilla-b2g28_v1_3. This is a more "proper"
> > fix that adds an optional parameter to mozMobileMessage.getThreads() to
> > specify the direction we want to use for iteration. This has the upside of
> > not changing the behavior of the method when called w/o parameters but
> > requires more extensive changes in Gecko. From an API perspective this is
> > more consistent with mozMobileMessage.getMessages() but still means we're
> > changing a public API very late in the 1.3 cycle which feels a bit awkward.
> 
> I think they're quite the same because Gaia has no way (except Gecko
> version) to detect either API change, so both of them result in an unknown
> behaviour.  I mean passing an optional 'true' to that API doesn't guarantee
> that message threads are always returned in reversed order because one might
> just have a slightly outdated Gecko.  Then it seems to me that preserving
> current API interface, that is, taking the one liner solution, would be a
> better idea for me. :)

The only user of this API is the SMS app so I'd rather favor consistency over the public API debate. (so, the opposite advice).

That said, I've no strong opinion so as long as we move forward I am happy.

Comment 11

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Revert the order of retrieval of message threads

Updated patch with the marionette test fixed, carried over the r+ flag.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH alt v2] Add a paremeter to make mozMobileMessage.getThreads() return threads in reverse order

Alternate patch adding the |reverse| parameter, now with updated tests working properly.

Comment 13

[Rick Waldron [:rwaldron]]

(In reply to Gabriele Svelto [:gsvelto] from comment #12)
> Created attachment 8365313 [details] [diff] [review]
> [PATCH alt v2] Add a paremeter to make mozMobileMessage.getThreads() return
> threads in reverse order
> 
> Alternate patch adding the |reverse| parameter, now with updated tests
> working properly.

Just to clarify, this means that the public facing API will look something like these: 

mozMobileMessage.getThreads(true);

mozMobileMessage.getThreads(false);


If you didn't write this patch and had never seen this, would you have any idea at all what `true` or `false` meant here? What happens when we need another getThreads filter or operational mechanism? This is a well known anti-pattern that should be avoided[0].

Please use an "options object"; this makes it obvious to the reader and is not future hostile:


mozMobileMessage.getThreads({ reverse: true }); 

mozMobileMessage.getThreads({ reverse: false }); // this is the default



[0] http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html

Comment 14

[[:fabrice] Fabrice Desré]

I'm fine with taking the "one line" fix on 1.3, but not the other one.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] Reverse the order of retrieval of message threads r=gene

(In reply to Fabrice Desré [:fabrice] from comment #14)
> I'm fine with taking the "one line" fix on 1.3, but not the other one.

Thanks Fabrice, I'm obsoleting the other patch and I've fixed the comment on this one (revert -> reverse), carrying over the r+ flag. This is the try run:

https://tbpl.mozilla.org/?tree=Try&rev=bb6da2773e73

Comment 16

[Preeti Raghunath(:Preeti)]

1.3+ for landing.

Comment 17

[Gabriele Svelto [:gsvelto]]

Try is green, ready for landing. This needs to land on mozilla-b2g28_v1_3 too.

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #13)
> If you didn't write this patch and had never seen this, would you have any
> idea at all what `true` or `false` meant here? What happens when we need
> another getThreads filter or operational mechanism? This is a well known
> anti-pattern that should be avoided[0].
>
> Please use an "options object"; this makes it obvious to the reader and is
> not future hostile:
>
> mozMobileMessage.getThreads({ reverse: true }); 
>
> mozMobileMessage.getThreads({ reverse: false }); // this is the default

This is a very good point and a pattern I'll keep in mind, thanks for pointing it out. Coming from a mainly C/C++ background I was used to replacing booleans with enums to clarify the parameter's meaning but in JS this wasn't available and I hadn't thought about this pattern.

Comment 19

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

(In reply to Rick Waldron [:rwaldron] from comment #13)
> Please use an "options object"; this makes it obvious to the reader and is
> not future hostile:
> 
> mozMobileMessage.getThreads({ reverse: true }); 
> mozMobileMessage.getThreads({ reverse: false }); // this is the default

That's even better. I'll f+ or r+ this once it's uploaded.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9441f9619749

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> That's even better. I'll f+ or r+ this once it's uploaded.

OK, so to keep the scope of this bug small I'll open a follow-up bug and move attachment 8365313 [details] [diff] [review] there. To keep the API consistent I'll open a follow up where I propose to add an optional object to specify the ordering of threads (as described above) and I'll also propose to change the second parameter of getMessages() [1] in a similar way for consistency. I'll shoot an e-mail to the dev-webapi list too.

[1] It's currently a boolean called |reverse|, changing it too will improve readability as Rick suggested

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9441f9619749

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a20477cb1190

You might want to take a look at this for sanity as Aurora doesn't have MobileMessageDB.jsm and this therefore required some fixing up :).

Comment 24

[Gabriele Svelto [:gsvelto]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #23)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/a20477cb1190
> 
> You might want to take a look at this for sanity as Aurora doesn't have
> MobileMessageDB.jsm and this therefore required some fixing up :).

It's looking good, thank you very much!