+++ 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.

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

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!

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

(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 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.

koi+ as it is added to comms team Sprint 3

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?

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

(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 on attachment 794574 [details] [diff] [review] Patch, V2 Review of attachment 794574 [details] [diff] [review]: ----------------------------------------------------------------- Let's go DOMRequestService::Fire{Error,Success}Async.

(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.

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.

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 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.

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

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. :)

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

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

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

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

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

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?

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

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)