820780 - WebSMS: return more segmentation information on calling getNumberOfMessagesForText

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

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

This bug is for DOM API part of bug 813682. Some new attributes are suggested:

1) Total segments to send: already have with getNumberOfMessagesForText().
2) Chars available per segment: depending on data code scheme, this might be 140/70. It might also vary with concatenation involved.
3) Chars already input in current segment.
4) Chars remains available in current segment.

iPhone gets "4)/2)", while Android gets "4)/1)".

Comment 1

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

BB? for it blocks bug 813682.

Comment 2

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

Attachment: Part 1/5: interface changes

Returns 1), 2), 4) only because 3) = 2) - 4).

Comment 3

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

Attachment: Part 2/5: DOM, IPC changes : WIP

Comment 4

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

Attachment: Part 3/5: B2G RIL backend : WIP

Comment 5

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

Attachment: Part 4/5: Android backend : WIP

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 691378 [details] [diff] [review]
Part 1/5: interface changes

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

Though why does this need RIL or IPC changes? Can't we just implement this in the child process directly? No need to proxy any calls to the parent process or the RIL.

Comment 7

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

(In reply to Jonas Sicking (:sicking) from comment #6)
> Comment on attachment 691378 [details] [diff] [review]
> Part 1/5: interface changes
> -----------------------------------------------------------------
> Though why does this need RIL or IPC changes? Can't we just implement this
> in the child process directly? No need to proxy any calls to the parent
> process or the RIL.

I think the main reason here is the backend divergence. SMS text segmentation involves several preferences in different backends. In B2G, we respect "dom.sms.strict7BitEncoding" introduced in bug 790192; bug 733331 also wants to add more SMS related preferences for extra default enabled GSM 7Bit Alphabets. All these settings affect the result of this call, and some of them are not be available/implemented on Android. We can't ensure its correctness if the result does not come directly from the backend, and backends are hidden in chrome process.

Comment 8

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

Attachment: Part 1/5: interface changes : v2

Add sr+. Previously sr+ in comment #6.

Comment 9

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

Attachment: Part 2/5: DOM, IPC changes

Comment 10

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

Attachment: Part 3/5: B2G RIL backend

Yoshi, would you help review this patch? It's based on bug 816082 part 3.

Comment 11

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

Attachment: Part 4/5: Android backend : WIP2

Comment 12

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

Attachment: Part 5/5: marionette test case

Hi Rob, would you like to review this patch? It will replace your test cases for getNumberOfMessagesForText() in bug 808865.

Comment 13

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

Attachment: Part 4/5: Android backend

Hi Blad, would you help review this patch?
try: https://tbpl.mozilla.org/?tree=Try&rev=9bfa7ce9b088

Comment 14

[Mozilla RelEng Bot]

Try run for 9bfa7ce9b088 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9bfa7ce9b088
Results (out of 48 total builds):
    success: 46
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9bfa7ce9b088

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 691874 [details] [diff] [review]
Part 4/5: Android backend

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

::: embedding/android/GeckoSmsManager.java
@@ +492,5 @@
>      }
>    }
>  
> +  public int getSegmentInfoForText(String aText) {
> +    int[] result = SmsMessage.calculateLength(aText, false);

ugh... this is an ugly API.

@@ +500,5 @@
> +    int segmentChars = (result[1] + result[2]) / result[0];
> +
> +    return (segments & 0xFF)
> +           | ((segmentChars & 0xFF) < 8)
> +           | ((charsAvailableInLastSegment & 0xFF) < 16);

can you explain what is going on here? What is this calculating exactly? Does this get unpacked further down the line? If so, wouldn't it be better to return int[]?

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 691861 [details] [diff] [review]
Part 3/5: B2G RIL backend

Hsinyi should be better reviewer.

Comment 17

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

(In reply to Brad Lassey [:blassey] from comment #15)
> Comment on attachment 691874 [details] [diff] [review]
> Part 4/5: Android backend
> 
> Review of attachment 691874 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +500,5 @@
> > +    int segmentChars = (result[1] + result[2]) / result[0];
> > +
> > +    return (segments & 0xFF)
> > +           | ((segmentChars & 0xFF) < 8)
> > +           | ((charsAvailableInLastSegment & 0xFF) < 16);

Oops, should be bitwise shift left operator(<<) here.

> can you explain what is going on here? What is this
> calculating exactly? Does this get unpacked further down the
> line? If so, wouldn't it be better to return int[]?

In public DOM API, it returns an object with three attributes, but in internal nsISmsService interface, it returns a packed uint32_t integer to save extra works for IPC. This packed integer will only be unpacked in dom/sms/src/SmsManager.cpp.

Comment 18

[Robert Wood [:rwood]]

Comment on attachment 691864 [details] [diff] [review]
Part 5/5: marionette test case

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

For some reason I can't apply the patch attachment, I get:

applying attachment.cgi?id=691864
patching file dom/sms/tests/marionette/manifest.ini
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file dom/sms/tests/marionette/manifest.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=691864

Also wondering what the last test in the manifest.ini file is? It doesn't exist on mozilla central or inbound.

::: dom/sms/tests/marionette/manifest.ini
@@ +25,4 @@
>  [test_mark_msg_read.js]
>  [test_mark_msg_read_error.js]
>  [test_bug814761.js]
>  [test_strict_7bit_encoding.js]

Do you know what test_strict_7bit_encoding.js is? This doesn't exist in the same manifest.ini in mozilla central

Comment 19

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

Comment on attachment 691864 [details] [diff] [review]
Part 5/5: marionette test case

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

Hi Rob,

If you want to run these new test cases, please apply patches in bug 816082 first.

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 691861 [details] [diff] [review]
Part 3/5: B2G RIL backend

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

This patch is also the part 3 of bug 816082. Seems you uploaded the wrong file here.

Comment 21

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

Attachment: Part 3/5: B2G RIL backend

Comment 22

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

Attachment: Part 4/5: Android backend : v2

Address comment #17.

Comment 23

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

Explicitly depends bug 816082 for we need its part 3 as mentioned in comment #19.

Comment 24

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

There are two major concerns in the implementation of this method:

1) the backend divergence forbids us to have some uniformed calculation logic in content process. It's highly platform dependent because there are a lot configurable parameters during the process. 'strict7BitEncoding' is one of them.

2) this method may be invoked every time an user press a key, so we'd better try our best to optimize it. The DOM API has to return a struct with no doubt, but we can save some work on the internal interfaces. That's the main reason I use packed integer here: to get rid of unnecessary jsapi things in internal interfaces.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 692845 [details] [diff] [review]
Part 4/5: Android backend : v2

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

::: dom/sms/src/android/SmsService.cpp
@@ +29,5 @@
>      *aResult = 0;
>      return NS_OK;
>    }
>  
> +  *aResult = AndroidBridge::Bridge()->GetSegmentInfoForText(aText);

struct not packed int here too

::: embedding/android/GeckoSmsManager.java
@@ +504,5 @@
> +    // to save extra works for IPC. This packed integer will only be unpacked
> +    // in dom/sms/src/SmsManager.cpp.
> +    return (segments & 0xFF)
> +           | ((segmentChars & 0xFF) << 8)
> +           | ((charsAvailableInLastSegment & 0xFF) << 16);

I'm not a fan of packing this int, it seems like maintenance will be problematic and error prone. Just return the int array directly.

::: widget/android/AndroidBridge.cpp
@@ +1657,5 @@
>      env->CallStaticVoidMethod(mGeckoAppShellClass, jMarkUriVisited, jstrURI);
>  }
>  
> +uint32_t
> +AndroidBridge::GetSegmentInfoForText(const nsAString& aText)

Please define and return a struct rather than a packed int

@@ +1667,5 @@
>          return 0;
>  
>      AutoLocalJNIFrame jniFrame(env);
>      jstring jText = NewJavaString(&jniFrame, aText);
> +    uint32_t ret = env->CallStaticIntMethod(mGeckoAppShellClass, jGetSegmentInfoForText, jText);

You could just call SmsMessage.calculateLenght() directly rather than wrapping it with GeckoAppShell.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 691860 [details] [diff] [review]
Part 2/5: DOM, IPC changes

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

::: dom/sms/src/SmsManager.cpp
@@ +135,5 @@
>  {
> +  nsresult rv;
> +  nsIScriptContext* sc = GetContextForEventHandlers(&rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_TRUE(sc, NS_OK);

You can't return NS_OK from an XPCOM method without setting your outparams. That will crash.

You repeat this pattern a lot below, please change them all to return error codes.

@@ +140,5 @@
> +
> +  JSContext* cx = sc->GetNativeContext();
> +  MOZ_ASSERT(cx);
> +
> +  nsCOMPtr<nsIScriptGlobalObject> sgo = do_QueryInterface(GetOwner());

I think GetOwner() can return null in some cases so you should NS_ENSURE_TRUE(sgo) here.

@@ +161,5 @@
>  
> +  JSObject* obj = JS_NewObject(cx, nullptr, nullptr, nullptr);
> +  NS_ENSURE_TRUE(obj, NS_OK);
> +
> +  jsval segments = JS_NumberValue(double(info & 0xFF));

Like Brad, I'm not a big fan of packing all these values into the same int here. Just wanted to get that down.

@@ +162,5 @@
> +  JSObject* obj = JS_NewObject(cx, nullptr, nullptr, nullptr);
> +  NS_ENSURE_TRUE(obj, NS_OK);
> +
> +  jsval segments = JS_NumberValue(double(info & 0xFF));
> +  bool ok = JS_SetProperty(cx, obj, "segments", &segments);

Let's use JS_DefineProperty for all of these, with JSPROP_ENUMERATE and no getter/setter. (JS_SetProperty tries to run setters which will slow down the assignments here.)

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 692844 [details] [diff] [review]
Part 3/5: B2G RIL backend

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

::: dom/sms/src/ril/SmsService.cpp
@@ +43,5 @@
>      *aResult = 0;
>      return NS_OK;
>    }
>  
> +  mRIL->GetSegmentInfoForText(aText, aResult);

Let's address Ben's and Brad's comments, i.e. not packing the result into an integer, here as well.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2387,1 @@
>    },

ditto.

Comment 28

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

Attachment: Part 1/5: interface changes : v3

Return object in internal interface nsISmsService as well.

Comment 29

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

Attachment: Part 2/5: DOM, IPC changes: v2

Address review comment #26.

Comment 30

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

Attachment: Part 3/5: B2G RIL backend : v2

Return object in internal interface nsISmsService as well.

Comment 31

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

Attachment: Part 4/5: Android backend : v3

trying: https://tbpl.mozilla.org/?tree=Try&rev=7249cfa34b0b

Comment 32

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

Comment on attachment 691864 [details] [diff] [review]
Part 5/5: marionette test case

need test cases for strict7BitEncoding

Comment 33

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

Attachment: Part 2/5: DOM, IPC changes: v3

Remove unused varible xpc in SmsParent::RecvGetSegmentInfoForText

Comment 34

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

Attachment: Part 4/5: Android backend : v4

Remove unused varible xpc in SmsParent::RecvGetSegmentInfoForText

Comment 35

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

Attachment: Part 5/5: marionette test case : v2

1) simplify times()
2) add test cases for strict 7bit encoding

Hi Rob, you should be able to apply these five patches directly onto m-c without any foreign references. Thank you :)

Comment 36

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

Attachment: Part 4/5: Android backend : v4

Address review comment #25, return object in internal interface nsISmsService as well. try result is available here: https://tbpl.mozilla.org/?tree=Try&rev=019db8f35167

Comment 37

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 695477 [details] [diff] [review]
Part 1/5: interface changes : v3

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

::: dom/sms/interfaces/nsIDOMSmsManager.idl
@@ +26,5 @@
> +   *
> +   * 'charsAvailableInLastSegment' is the maximum number of available
> +   * characters in the last segment.
> +   */
> +  jsval getSegmentInfoForText(in DOMString text);

why are we returning a jsval here and not a, relatively simple, interface?

interface nsISegmentInfo
{
    readonly attribute int segments;
    readonly attribute int charsPerSegment;
    readonly attribute int charsAvailableInLastSegment;
};

Comment 38

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 695557 [details] [diff] [review]
Part 2/5: DOM, IPC changes: v3

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

::: dom/sms/src/ipc/SmsIPCService.cpp
@@ +62,2 @@
>  {
> +  JSContext *cx = nsContentUtils::GetCurrentJSContext();

I think Brad is right, we shouldn't be returning a jsval here. Make a real interface and then XPConnect will handle this for you.

But in the future you can add [implicit_jscontext] to the IDL and then XPConnect will pass you a JSContext.

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 695477 [details] [diff] [review]
Part 1/5: interface changes : v3

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

I'm fine either way. I'm not sure which will result in less code given that this approach avoids creating a new C++ class.

Comment 40

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

Attachment: Part 1/5: interface changes : v4

Jonas, sorry. I have to revise the interfaces to address Blad & Bent's review comments. I've moved internal interface changes to latter patches and left only public interfaces in this patch.

Comment 41

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

Attachment: Part 2/5: DOM, IPC changes: v4

Address review comment #38, create an interface for the returned segment info object. We don't need implicit_jscontext for the createSmsSegmentInfo() because there is no jsapi involved in it. At least for now ;)

Comment 42

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

Attachment: Part 3/5: B2G RIL backend : v3

The JS part was reviewed but new C++ changes[1] made to address Bent's review comment #38.

[1]: implement nsISmsService.CreateSmsSegmentInfo() in dom/sms/src/ril/SmsService.cpp

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 696273 [details] [diff] [review]
Part 1/5: interface changes : v4

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

Since I said "I'm fine either way." that means that my review stands with either solution. So you didn't actually have to re-request review.

But no worries, better ask too much than too little.

Comment 44

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

Attachment: Part 4/5: Android backend : v5

Address review comment #38, create an interface for the returned segment info object.

try: https://tbpl.mozilla.org/?tree=Try&rev=1957b4c8d140

Comment 45

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #35)
> Created attachment 695572 [details] [diff] [review]
> Part 5/5: marionette test case : v2

No changes made to this patch in this run and B2G RIL tests passed as before.

Comment 46

[Mozilla RelEng Bot]

Try run for 1957b4c8d140 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1957b4c8d140
Results (out of 60 total builds):
    success: 4
    warnings: 49
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1957b4c8d140

Comment 47

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 696274 [details] [diff] [review]
Part 2/5: DOM, IPC changes: v4

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

::: dom/sms/interfaces/nsISmsService.idl
@@ +36,5 @@
>                                         in bool      read);
> +
> +  nsIDOMMozSmsSegmentInfo createSmsSegmentInfo(in long segments,
> +                                               in long charsPerSegment,
> +                                               in long charsAvailableInLastSegment);

I don't see any callers of this... Is it necessary?

::: dom/sms/src/SmsSegmentInfo.h
@@ +8,5 @@
> +
> +#include "mozilla/dom/sms/PSms.h"
> +#include "nsIDOMSmsSegmentInfo.h"
> +#include "Types.h"
> +#include "mozilla/Attributes.h"

It looks like you only need to include nsIDOMSmsSegmentInfo.h, mozilla/Attributes.h, and whatever is generated that declares SmsSegmentInfoData. I don't think these others are needed.

@@ +27,5 @@
> +  SmsSegmentInfo(const SmsSegmentInfoData& aData);
> +
> +private:
> +  // Don't try to use the default constructor.
> +  SmsSegmentInfo();

You shouldn't need this, you supplied a non-default constructor so the compiler won't generate this.

::: dom/sms/src/ipc/SmsIPCService.cpp
@@ +63,2 @@
>  {
> +  *aResult = nullptr;

Nit: This is not needed. You only need to set the result if you're going to return NS_OK.

::: dom/sms/src/ipc/SmsParent.cpp
@@ +160,5 @@
> +  NS_ENSURE_SUCCESS(rv, true);
> +
> +  info->GetSegments(&(aResult->segments()));
> +  info->GetCharsPerSegment(&(aResult->charsPerSegment()));
> +  info->GetCharsAvailableInLastSegment(&(aResult->charsAvailableInLastSegment()));

Please make this:

  if (NS_FAILED(info->GetSegments(...)) ||
      NS_FAILED(info->GetCharsPerSegment(...)) ||
      NS_FAILED(info->GetCharsAvailableInLastSegment(...))) {
    NS_ERROR(...);
  }

Comment 48

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 696275 [details] [diff] [review]
Part 3/5: B2G RIL backend : v3

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

::: dom/sms/src/ril/SmsService.cpp
@@ +82,5 @@
> +                                 int32_t aCharsAvailableInLastSegment,
> +                                 nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);

Nit: = on previous line.

@@ +83,5 @@
> +                                 nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);
> +  info.swap(*aSegmentInfo);

Please use info.forget()

Comment 49

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 696274 [details] [diff] [review]
Part 2/5: DOM, IPC changes: v4

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

::: dom/sms/src/fallback/SmsService.cpp
@@ +62,5 @@
> +                                 int32_t aCharsAvailableInLastSegment,
> +                                 nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);

Nit: = on previous line.

@@ +63,5 @@
> +                                 nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);
> +  info.swap(*aSegmentInfo);

Please use info.forget().

::: dom/sms/src/ipc/SmsIPCService.cpp
@@ +67,5 @@
> +  bool ok = GetSmsChild()->SendGetSegmentInfoForText(nsString(aText), &data);
> +  NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE);
> +
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info = new SmsSegmentInfo(data);
> +  info.swap(*aResult);

Please use info.forget().

@@ +106,5 @@
> +                                    int32_t aCharsAvailableInLastSegment,
> +                                    nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);

Nit: = on previous line.

@@ +107,5 @@
> +                                    nsIDOMMozSmsSegmentInfo** aSegmentInfo)
> +{
> +  nsCOMPtr<nsIDOMMozSmsSegmentInfo> info
> +      = new SmsSegmentInfo(aSegments, aCharsPerSegment, aCharsAvailableInLastSegment);
> +  info.swap(*aSegmentInfo);

Please use info.forget().

Comment 50

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

(In reply to ben turner [:bent] from comment #47)
> Comment on attachment 696274 [details] [diff] [review]
> Part 2/5: DOM, IPC changes: v4
-----------------------------------------------------------------
> 
> ::: dom/sms/interfaces/nsISmsService.idl
> @@ +36,5 @@
> >                                         in bool      read);
> > +
> > +  nsIDOMMozSmsSegmentInfo createSmsSegmentInfo(...);
> 
> I don't see any callers of this... Is it necessary?

Like createSmsMessage(), this call is actually only required by B2G RIL because it's written in Javascript. Also like the nsIRilSmsDatabaseService introduced in bug 774621 attachment 687763 [details] [diff] [review], I've tried to have another nsIRilSmsService to provide these B2G only calls, but that will affect SmsServiceFactory and other native stuff.

> ::: dom/sms/src/SmsSegmentInfo.h
> @@ +8,5 @@
> > +
> > +#include "mozilla/dom/sms/PSms.h"
> > +#include "nsIDOMSmsSegmentInfo.h"
> > +#include "Types.h"
> > +#include "mozilla/Attributes.h"
> 
> It looks like you only need to include nsIDOMSmsSegmentInfo.h,
> mozilla/Attributes.h, and whatever is generated that declares
> SmsSegmentInfoData. I don't think these others are needed.

Yes, I can replace the two with SmsTypes.h (generated from SmsTypes.ipdlh).

Comment 51

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

Attachment: Part 2/5: DOM, IPC changes: v5

Address Bent's review comments in comment #47

Comment 52

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

Attachment: Part 3/5: B2G RIL backend : v4

Address Bent's review comments in comment #48.

Comment 53

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

Attachment: Part 4/5: Android backend : v6

Fix try bustage. Will request for review once try gives all greens.

Comment 54

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

Attachment: Part 4/5: Android backend : v7

Apply Bent's review comments as well. Try result: https://tbpl.mozilla.org/?tree=Try&rev=1a45e6599aa4

Comment 55

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

Attachment: Part 5/5: marionette test case : v3

Fix dom/tests/mochitest/general/test_interfaces.html bustage.

Comment 56

[Alex Keybl [:akeybl]]

Bug 813682 was resolved/fixed. Does this truly still block that bug?

Comment 57

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

Well, I don't know how could Gaia resolve the issue without API changes.

Comment 58

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

(In reply to Alex Keybl [:akeybl] (currently out, please email [e-mail] for quicker response) from comment #56)
> Bug 813682 was resolved/fixed. Does this truly still block
> that bug?

They merged an incomplete implementation.

Comment 59

[Robert Wood [:rwood]]

Comment on attachment 696530 [details] [diff] [review]
Part 5/5: marionette test case : v3

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

Lgtm. I applied the patches and ran the tests locally. The marionette test passed fine. The test_interfaces.html mochitest failed however it looks like the failures are not related to the change - and the particular test case for this change passed:

TEST-PASS | unknown test url | Unexpected interface name in global scope: MozSmsSegmentInfo

Comment 60

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

(In reply to Rob Wood [:rwood] from comment #59)
> The test_interfaces.html mochitest failed however it looks like
> the failures are not related to the change

Waiting for https://tbpl.mozilla.org/?tree=Try&rev=3db30cbd61f1 , to see whether the problem mentioned by Rob persists.

Comment 61

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

Attachment: Part 2/5: DOM, IPC changes: v6

Update patch used in previous try run, fix build break in OSX.

Comment 62

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5a47d623e4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/67801b556c28
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a971360b41a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a68fdc0effd
https://hg.mozilla.org/integration/mozilla-inbound/rev/259982750c29

Comment 63

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f5a47d623e4f
https://hg.mozilla.org/mozilla-central/rev/67801b556c28
https://hg.mozilla.org/mozilla-central/rev/3a971360b41a
https://hg.mozilla.org/mozilla-central/rev/2a68fdc0effd
https://hg.mozilla.org/mozilla-central/rev/259982750c29

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff3f1eeeb0c6
https://hg.mozilla.org/releases/mozilla-aurora/rev/65cdaf6f9c08
https://hg.mozilla.org/releases/mozilla-aurora/rev/4e8283d720c0
https://hg.mozilla.org/releases/mozilla-aurora/rev/96f80ba6deb5
https://hg.mozilla.org/releases/mozilla-aurora/rev/74bccc5f5c04

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b93a756224cc
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c3a7f9b5c445
https://hg.mozilla.org/releases/mozilla-b2g18/rev/c6b145fceaf3
https://hg.mozilla.org/releases/mozilla-b2g18/rev/92e7e951ec77
https://hg.mozilla.org/releases/mozilla-b2g18/rev/69b8fe47fedf