Closed Bug 820780 Opened 7 years ago Closed 7 years ago

WebSMS: return more segmentation information on calling getNumberOfMessagesForText

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C4 (2jan on)
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: vicamo, Assigned: vicamo)

References

()

Details

(Keywords: feature, Whiteboard: [target:12/21])

Attachments

(5 files, 25 obsolete files)

2.17 KB, patch
sicking
: superreview+
Details | Diff | Splinter Review
12.40 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
15.64 KB, patch
blassey
: review+
Details | Diff | Splinter Review
9.51 KB, patch
rwood
: review+
Details | Diff | Splinter Review
19.13 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
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)".
BB? for it blocks bug 813682.
blocking-basecamp: --- → ?
Attached patch Part 1/5: interface changes (obsolete) — Splinter Review
Returns 1), 2), 4) only because 3) = 2) - 4).
Attachment #691378 - Flags: superreview?(jonas)
Attached patch Part 2/5: DOM, IPC changes : WIP (obsolete) — Splinter Review
Attached patch Part 3/5: B2G RIL backend : WIP (obsolete) — Splinter Review
Attached patch Part 4/5: Android backend : WIP (obsolete) — Splinter Review
Attachment #691381 - Attachment description: Part 4/5: Android backend → Part 4/5: Android backend : WIP
Assignee: nobody → vyang
blocking-basecamp: ? → +
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.
Attachment #691378 - Flags: superreview?(jonas) → superreview+
(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.
Attached patch Part 1/5: interface changes : v2 (obsolete) — Splinter Review
Add sr+. Previously sr+ in comment #6.
Attachment #691378 - Attachment is obsolete: true
Attachment #691859 - Flags: superreview+
Attached patch Part 2/5: DOM, IPC changes (obsolete) — Splinter Review
Attachment #691379 - Attachment is obsolete: true
Attachment #691860 - Flags: review?(jonas)
Attached patch Part 3/5: B2G RIL backend (obsolete) — Splinter Review
Yoshi, would you help review this patch? It's based on bug 816082 part 3.
Attachment #691380 - Attachment is obsolete: true
Attachment #691861 - Flags: review?(allstars.chh)
Attached patch Part 4/5: Android backend : WIP2 (obsolete) — Splinter Review
Attachment #691381 - Attachment is obsolete: true
Attached patch Part 5/5: marionette test case (obsolete) — Splinter Review
Hi Rob, would you like to review this patch? It will replace your test cases for getNumberOfMessagesForText() in bug 808865.
Attachment #691864 - Flags: review?(rwood)
Attached patch Part 4/5: Android backend (obsolete) — Splinter Review
Hi Blad, would you help review this patch?
try: https://tbpl.mozilla.org/?tree=Try&rev=9bfa7ce9b088
Attachment #691862 - Attachment is obsolete: true
Attachment #691874 - Flags: review?(blassey.bugs)
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/vyang@mozilla.com-9bfa7ce9b088
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[]?
Attachment #691874 - Flags: review?(blassey.bugs) → review-
Comment on attachment 691861 [details] [diff] [review]
Part 3/5: B2G RIL backend

Hsinyi should be better reviewer.
Attachment #691861 - Flags: review?(allstars.chh) → review?(htsai)
(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 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
Attachment #691864 - Flags: review?(rwood) → review-
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.
Attachment #691864 - Flags: review- → review?
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.
Attachment #691861 - Flags: review?(htsai)
Attached patch Part 3/5: B2G RIL backend (obsolete) — Splinter Review
Attachment #691861 - Attachment is obsolete: true
Attachment #692844 - Flags: review?(htsai)
Attached patch Part 4/5: Android backend : v2 (obsolete) — Splinter Review
Address comment #17.
Attachment #691874 - Attachment is obsolete: true
Attachment #692845 - Flags: review?(blassey.bugs)
Explicitly depends bug 816082 for we need its part 3 as mentioned in comment #19.
Depends on: 816082
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 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.
Attachment #692845 - Flags: review?(blassey.bugs) → review-
Attachment #691860 - Flags: review?(jonas) → review?(bent.mozilla)
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.)
Attachment #691860 - Flags: review?(bent.mozilla) → review-
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.
Attachment #692844 - Flags: review?(htsai)
Attached patch Part 1/5: interface changes : v3 (obsolete) — Splinter Review
Return object in internal interface nsISmsService as well.
Attachment #691859 - Attachment is obsolete: true
Attachment #695477 - Flags: superreview?(jonas)
Attached patch Part 2/5: DOM, IPC changes: v2 (obsolete) — Splinter Review
Address review comment #26.
Attachment #691860 - Attachment is obsolete: true
Attachment #695478 - Flags: review?(bent.mozilla)
Attached patch Part 3/5: B2G RIL backend : v2 (obsolete) — Splinter Review
Return object in internal interface nsISmsService as well.
Attachment #692844 - Attachment is obsolete: true
Attachment #695479 - Flags: review?(htsai)
Comment on attachment 691864 [details] [diff] [review]
Part 5/5: marionette test case

need test cases for strict7BitEncoding
Attachment #691864 - Flags: review?
Attached patch Part 2/5: DOM, IPC changes: v3 (obsolete) — Splinter Review
Remove unused varible xpc in SmsParent::RecvGetSegmentInfoForText
Attachment #695478 - Attachment is obsolete: true
Attachment #695478 - Flags: review?(bent.mozilla)
Attachment #695557 - Flags: review?(bent.mozilla)
Attached patch Part 4/5: Android backend : v4 (obsolete) — Splinter Review
Remove unused varible xpc in SmsParent::RecvGetSegmentInfoForText
Attachment #695497 - Attachment is obsolete: true
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 :)
Attachment #691864 - Attachment is obsolete: true
Attachment #695572 - Flags: review?(rwood)
Attached patch Part 4/5: Android backend : v4 (obsolete) — Splinter Review
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
Attachment #695558 - Attachment is obsolete: true
Attachment #695597 - Flags: review?(blassey.bugs)
Attachment #695479 - Flags: review?(htsai) → review+
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 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.
Attachment #695557 - Flags: review?(bent.mozilla)
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.
Attachment #695477 - Flags: superreview?(jonas) → superreview+
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.
Attachment #695477 - Attachment is obsolete: true
Attachment #696273 - Flags: superreview?(jonas)
Attached patch Part 2/5: DOM, IPC changes: v4 (obsolete) — Splinter Review
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 ;)
Attachment #695557 - Attachment is obsolete: true
Attachment #696274 - Flags: review?(bent.mozilla)
Attached patch Part 3/5: B2G RIL backend : v3 (obsolete) — Splinter Review
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
Attachment #695479 - Attachment is obsolete: true
Attachment #696275 - Flags: review?(bent.mozilla)
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.
Attachment #696273 - Flags: superreview?(jonas) → superreview+
Attached patch Part 4/5: Android backend : v5 (obsolete) — Splinter Review
Address review comment #38, create an interface for the returned segment info object.

try: https://tbpl.mozilla.org/?tree=Try&rev=1957b4c8d140
Attachment #695597 - Attachment is obsolete: true
Attachment #695597 - Flags: review?(blassey.bugs)
Attachment #696279 - Flags: review?(blassey.bugs)
(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.
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/vyang@mozilla.com-1957b4c8d140
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(...);
  }
Attachment #696274 - Flags: review?(bent.mozilla) → review+
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()
Attachment #696275 - Flags: review?(bent.mozilla) → review+
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().
(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).
Attached patch Part 2/5: DOM, IPC changes: v5 (obsolete) — Splinter Review
Address Bent's review comments in comment #47
Attachment #696274 - Attachment is obsolete: true
Attachment #696504 - Flags: review+
Address Bent's review comments in comment #48.
Attachment #696275 - Attachment is obsolete: true
Attachment #696505 - Flags: review+
Attached patch Part 4/5: Android backend : v6 (obsolete) — Splinter Review
Fix try bustage. Will request for review once try gives all greens.
Attachment #696279 - Attachment is obsolete: true
Attachment #696279 - Flags: review?(blassey.bugs)
Apply Bent's review comments as well. Try result: https://tbpl.mozilla.org/?tree=Try&rev=1a45e6599aa4
Attachment #696506 - Attachment is obsolete: true
Attachment #696529 - Flags: review?(blassey.bugs)
Fix dom/tests/mochitest/general/test_interfaces.html bustage.
Attachment #695572 - Attachment is obsolete: true
Attachment #695572 - Flags: review?(rwood)
Attachment #696530 - Flags: review?(rwood)
Attachment #696529 - Flags: review?(blassey.bugs) → review+
Bug 813682 was resolved/fixed. Does this truly still block that bug?
Well, I don't know how could Gaia resolve the issue without API changes.
(In reply to Alex Keybl [:akeybl] (currently out, please email release-mgmt@mozilla.com for quicker response) from comment #56)
> Bug 813682 was resolved/fixed. Does this truly still block
> that bug?

They merged an incomplete implementation.
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
Attachment #696530 - Flags: review?(rwood) → review+
(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.
Target Milestone: B2G C3 (12dec-1jan) → B2G C4 (2jan on)
Update patch used in previous try run, fix build break in OSX.
Attachment #696504 - Attachment is obsolete: true
Attachment #697803 - Flags: review+
Blocks: 826614
You need to log in before you can comment on or make changes to this bug.