902497 - [sms][mms] use the new asynchronous getSegmentInfoText from bug 903403 for updateSmsSegmentLimit

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
* start writing a SMS
* type quickly (for example use 3 fingers in the same time on the keyboard)

=> we notice a lag for displaying the characters in the input

We profiled this with Gabriele and we noticed that updateSmsSegmentLimit takes about 30% of the used time, because w call mozMobileMessage.getSegmentInfoForText which does synchronous IPC, which takes time.

I can see 2 potential solutions:
* getSegmentInfoForText is moved to the child process (as bug 896810 is trying to do for other things)
* if this proves to be too much work, we can add a timer in the Messages app to update the sms counter only 500ms after the last key.

So Gene, the first question is for you: do you think it's easy/difficult to move getSegmentInfoForText to the child process ?

Comment 1

[Gene Lian (I already quit Mozilla)]

(Vicamo, please correct me if I'm wrong)

I think it's very hard to move the logic from parent process to child process because the calculation of getSegmentInfoForText() depends on:

1. |dom.sms.strict7BitEncoding| preference which can only be acquired in the parent process.

2. some RIL mapping tables like RIL.GSM_SMS_STRICT_7BIT_CHARMAP and RIL.PDU_NL_LOCKING_SHIFT_TABLES... etc, which are only available on the RIL, again, in the parent process.

The ordinary design of this API is expecting the input of the complete text, so it doesn't make sense to call it whenever one char is appended at the tail. I think we need to solve this issue at Gaia as suggested at comment #0.

Comment 2

[Julien Wajsberg [:julienw]]

(In reply to Gene Lian [:gene] from comment #1)

> 1. |dom.sms.strict7BitEncoding| preference which can only be acquired in the parent process.

When is this pref actually changed ? I think this could be cached somewhere anyway.

> The ordinary design of this API is expecting the input of the complete text,
> so it doesn't make sense to call it whenever one char is appended at the
> tail.

yes we need to call it so that we can display the character counter to the user (how much characters he can still type before sending another SMS).

But now I remember that you also need to call this function in the sending code, and therefore it's probably not a good idea to move it anyway.

> I think we need to solve this issue at Gaia as suggested at comment #0.

I wonder if we could try to be more clever in Gaia, ie detect when we would change the encoding and call the function only at that point. When we don't change the encoding (ie: the user added or removed a "normal" ascii character) we can probably do the calculation in Gaia. Therefore we would call the Gecko API only once in a while, when the user adds an extended ascii character.

This would probably work great for the latin alphabet but probably not for other alphabet. Could there be a way for the API to send us the "normal" alphabet for the current encoding ? Then when the user types something outside of the normal alphabet, we would call the API.

Comment 3

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

|getSegmentInfoForText| may have different behaviours in different backends.  In Android backend, we don't have strict 7bit encoding.  So moving this function to child process may cause mismatch in results of |getSegmentInfoForText| and |sendSMS| itself.

What about making it async?

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> What about making it async?

I think we'll do that. One of the ideas we were evaluating is to request the updated info from a timeout instead of synchronously. Every time the user types we might reset this timeout so the request kicks in only when there's a longer pause (say more than 500ms) instead of once for every typed key.

Comment 5

[Gene Lian (I already quit Mozilla)]

Fire bug 903409 to make getSegmentInfoForText() to be an asynchronous API on the Gecko side. Keep this bug open for Gaia.

Comment 6

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #5)
> Fire bug 903409 to make getSegmentInfoForText() to be an asynchronous API
       ^^^^^^^^^^ Sorry, I mean bug 903403.

Comment 7

[Julien Wajsberg [:julienw]]

Moved component and changed title then.

With the asynchronous updateSmsSegmentLimit, a small refactoring will need to be done in Gaia.

Comment 8

[Joe Cheng [:jcheng] (please needinfo)]

koi+ as it is added to comms team Sprint 3

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: wip patch v1

this is a WIP

especially I'd like to not call the API if another one is still ongoing.

Comment 10

[Julien Wajsberg [:julienw]]

Or maybe just call it once every 500ms.

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Github PR is https://github.com/mozilla-b2g/gaia/pull/11865

* we don't call getSegmentInfoForText more than once every 500ms
* getSegmentInfoForText is now asynchronous and the new code is handling this
* we now do the composeForm dataset type change in ThreadUI. As a result, the
  "type" event, sent by Compose, is not cancelable anymore, because we don't use
  this anymore.
---
 apps/sms/js/compose.js                      |   12 +-
 apps/sms/js/thread_ui.js                    |   97 +++++---
 apps/sms/test/unit/compose_test.js          |   29 ---
 apps/sms/test/unit/mock_navigatormoz_sms.js |   72 ++++--
 apps/sms/test/unit/thread_ui_test.js        |  339 +++++++++++++++------------
 5 files changed, 302 insertions(+), 247 deletions(-)


Asking review from both Steve (as a peer) and Rick (as someone who knows well some parts I change here). I'd like feedback from both before landing.

Comment 12

[Steve Chung [:steveck]]

Sorry for the late review because of some personal affairs. I'll start reviewing and test it with both gecko/gaia patches.

Comment 13

[Steve Chung [:steveck]]

Hi Julien, this patch looks ok at the first glance, but I need to wait for the building the gecko patch for verify(and it take looong time to builf it...).  I left few comments on github but I will take a deep look later(and test after build ready).

If possible, could we have some unit test to verify the update mechanism works? Because it takes me a while to think if there is any possible senario that will make the updateCounterForSms failed to match the latset status after update timeout ;p

Comment 14

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #13)
> 
> If possible, could we have some unit test to verify the update mechanism
> works? Because it takes me a while to think if there is any possible senario
> that will make the updateCounterForSms failed to match the latset status
> after update timeout ;p

Yep of course I can do a unit test for this, I prepared the Mock to be able to do this but didn't use it :)

Thanks !

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v2

updated Github PR at https://github.com/mozilla-b2g/gaia/pull/11865

follow-up:

* remove a log
* add more tests
* made the explanation about the timeout clearer

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 799096 [details] [diff] [review]
patch v2

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

Thnaks for the update and comments, it's very clear about the fixing and it work well after testing, I'm good with this patch except for the small concern about the request error here.

::: apps/sms/js/thread_ui.js
@@ +674,4 @@
>  
> +      var overLimit = segments > kMaxConcatenatedMessages;
> +      callback(overLimit);
> +    }).bind(this);

Not sure if any situation will cause the getSegmentInfoForText return error callback. Maybe it's rare case, but how about having a onerror callback function and hide the counter?

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: patch v3

Addressed the comment: added an onerror handler and added a test about this.

Updated Github PR at https://github.com/mozilla-b2g/gaia/pull/11865

thanks !

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: patch v4

Sorry Steve, I saw the log was back for some reason, so here another patch with the log removed.

The Github PR is updated https://github.com/mozilla-b2g/gaia/pull/11865

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 799384 [details] [diff] [review]
patch v4

Looks great, Thanks!

BTW, Travis failed on one error in message app, but it looks unrelated to this patch(and it shows green light in my local).

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/077da5aa65fc4e8be462c5e8357f17a2b694a75e