903403 - [sms][mms] Make getSegmentInfoForText() Asynchronous to Improve Typing Performance

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Gene Lian (I already quit Mozilla)]

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

We're planing to make mozMobileMessage.getSegmentInfoForText(text) to be an asynchronous call. The synchronous IPC would heavily block the content process when users are typing an SMS message very fast.

To do so, we need to change the original API:

  nsIDOMMozSmsSegmentInfo getSegmentInfoForText(in DOMString text);

to be:

  nsIDOMDOMRequest getSegmentInfoForText(in DOMString text);

which also needs the Gaia changes at bug 902497. This bug is fired for Gecko only.

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Patch (WIP)

Not yet finished the Android bridging and fixed the test cases.

Comment 2

[Gene Lian (I already quit Mozilla)]

Comment on attachment 788116 [details] [diff] [review]
Patch (WIP)

Hi Mounir,

Could you please take the super-review for the IDL change? Vicamo will review the implementation details. Thanks!

Comment 3

[Gene Lian (I already quit Mozilla)]

Hi Mounir, it seems you're too busy to take super-reviews recently. Shall I pass this to Jonas or someone else?

Comment 4

[Julien Wajsberg [:julienw]]

(In reply to Gene Lian [:gene] from comment #0)
 
> We're planing to make mozMobileMessage.getSegmentInfoForText(text) to be an
> asynchronous call. The synchronous IPC would heavily block the content
> process when users are typing an SMS message very fast.

Even reasonably fast ;)

Comment 5

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

Comment on attachment 788116 [details] [diff] [review]
Patch (WIP)

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

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +106,5 @@
>    nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
>    NS_ENSURE_TRUE(smsService, NS_ERROR_FAILURE);
>  
> +  nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
> +  nsCOMPtr<nsIMobileMessageCallback> msgCallback = new MobileMessageCallback(request);

nit: 80 chars

::: widget/android/AndroidBridge.cpp
@@ +162,5 @@
>      jMarkUriVisited = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "markUriVisited", "(Ljava/lang/String;)V");
>      jSetUriTitle = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "setUriTitle", "(Ljava/lang/String;Ljava/lang/String;)V");
>  
>      jSendMessage = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "sendMessage", "(Ljava/lang/String;Ljava/lang/String;I)V");
> +    jGetSegmentInfoForText = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "getSegmentInfoForText", "(II)V");

Never called and you don't really need it.  We have a |jCalculateLength| static method defined at line 106 which calls Android SMS API for segment info.  In the implementation of |AndroidBridge::GetSegmentInfoForText|, we can create an nsIDOMMozSmsSegmentInfo object and pass it to |nsIMobileMessageCallback->notifySegmentInfoForTextGot()|.

However, the problem in Android SMS backend now is it can't even be built successfully because of bug 862718, so you might not be able to test it at all.  Maybe you can just create an follow-up and make that new bug block bug 862715.

Comment 6

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

koi+ as it is added to comms team Sprint 3

Comment 7

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Comment 8

[Gene Lian (I already quit Mozilla)]

The try server failed: https://tbpl.mozilla.org/?tree=Try&rev=ab20ada2ef78

After looking into this issue, I found out the OOP works well but the marionette (non-OOP) fails because: when calling |smsService->GetSegmentInfoForText(aText, msgCallback)| the |request.notifySegmentInfoForTextGot(...)| in RIL immediately returns *before* |MobileMessageManager::GetSegmentInfoForText(...)| returns the DOMRequest object to the content. At the moment, the DOMRequest would drop this event because the content doesn't yet set its onsuccess/onerror.

MobileMessageManager::GetSegmentInfoForText(const nsAString& aText,
                                            nsIDOMDOMRequest** aRequest)
{
  nsCOMPtr<nsISmsService> smsService = do_GetService(SMS_SERVICE_CONTRACTID);
  NS_ENSURE_TRUE(smsService, NS_ERROR_FAILURE);

  nsRefPtr<DOMRequest> request = new DOMRequest(GetOwner());
  nsCOMPtr<nsIMobileMessageCallback> msgCallback =
    new MobileMessageCallback(request);
  nsresult rv = smsService->GetSegmentInfoForText(aText, msgCallback);
  NS_ENSURE_SUCCESS(rv, rv);

  request.forget(aRequest);
  return NS_OK;
}

Hi Vicamo, do you have any better idea to tackle this tricky issue?

Comment 9

[Gene Lian (I already quit Mozilla)]

Well, using DOMRequestService::FireSuccessAsync sounds a feasible solution.

Comment 10

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

(In reply to Gene Lian [:gene] from comment #8)
> Hi Vicamo, do you have any better idea to tackle this tricky issue?

No, and that should also have happened on other requests like sendSMS but it doesn't.  Hmmm...

Comment 11

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

Comment on attachment 794574 [details] [diff] [review]
Patch, V2

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

Let's go DOMRequestService::Fire{Error,Success}Async.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #10)
> (In reply to Gene Lian [:gene] from comment #8)
> > Hi Vicamo, do you have any better idea to tackle this tricky issue?
> 
> No, and that should also have happened on other requests like sendSMS but it
> doesn't.  Hmmm...

Thanks for the advice. Eventually, I think I know why... Because all the SMS/MMS need to be saved into DB before sending and that makes an asychronous call, so...

I'll work out a new patch and ask for another review.

Comment 13

[Julien Wajsberg [:julienw]]

Don't forget we need to wait for the gaia patch in bug 902497 before landing this, unless you change the method name and keep the old one temporary, or add an optional "async" argument.

I'd prefer that we wait for bug 902497 :) I have a WIP patch already.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V3

This patch is working but I'm not sure if you like what I did for NotifySegmentInfoForTextGot/NotifyGetSegmentInfoForTextFailed specific.

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

Comment 15

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

Comment on attachment 796790 [details] [diff] [review]
Patch, V3

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

Please file a follow-up bug and let's find out why other requests are immune from this problem.

Comment 16

[Julien Wajsberg [:julienw]]

bug 902497 is ready, let's check in both together !

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9dc1fcc1964e

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9dc1fcc1964e

Comment 19

[Gene Lian (I already quit Mozilla)]

It shocks me we've already landed this after I was back from vacation. Because I was told to wait for Gaia's codes, I didn't give the new patch a full test on the device but just on the try server. Anyway, I'll test it again with the Gaia codes. It should be working, though. :)

Comment 20

[Julien Wajsberg [:julienw]]

Gene> oops, sorry, it's my fault as I really thought it was ready. I tested it a lot on the device though !

Comment 21

[Gene Lian (I already quit Mozilla)]

No problem! Since you already did the test then I won't do it again. Thanks for you action. You make my day. :)

Comment 22

[Salvador de la Puente González [:salva]]

Please, update documentation in https://developer.mozilla.org/en-US/docs/Web/API/MozMobileMessageManager.getSegmentInfoForText

Comment 23

[Julien Wajsberg [:julienw]]

Jérémie, we need an updated doc here.

Comment 24

[Jean-Yves Perrier [:teoli]]

Julien, feel free to update it, it is a wiki.

Comment 25

[Jeremie Patonnier :Jeremie]

Which(In reply to Julien Wajsberg [:julienw] from comment #23)
> Jérémie, we need an updated doc here.

Which Firefox OS version is targeted? 1.2 or 1.3?

Comment 26

[Julien Wajsberg [:julienw]]

Hey Jérémie, this is a 1.2 bug.

Thanks!

Comment 27

[Jeremie Patonnier :Jeremie]

The documentation is now up to date: https://developer.mozilla.org/en-US/docs/Web/API/MozMobileMessageManager.getSegmentInfoForText

And it still expect a tech review ;) (Just let me know if it's okay or if something need to be changed)