Closed Bug 1044721 Opened 5 years ago Closed 4 years ago

[B2G][SMS] Provide an API to modify the SMSC address of the inserted UICC.

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(feature-b2g:2.2r+, tracking-b2g:backlog, firefox40 fixed, b2g-v2.2r fixed)

RESOLVED FIXED
2.2 S11 (1may)
feature-b2g 2.2r+
tracking-b2g backlog
Tracking Status
firefox40 --- fixed
b2g-v2.2r --- fixed

People

(Reporter: bevis, Assigned: freesamael)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [grooming])

Attachments

(8 files, 24 obsolete files)

8.02 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
11.64 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
7.97 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
1.41 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
6.67 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
9.36 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
3.88 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
48.31 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
From the discussion in the b2g-dev mail list [1],
there is one use-case where the carrier provides different rate for sending SMS to different SMSC address.

File this bug to address the possibility for supporting this in gecko side.

[1] https://groups.google.com/forum/#!searchin/mozilla.dev.b2g/SMSC/mozilla.dev.b2g/sTB2mLhCRAs/lwzXGV_5TX8J
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #0)
> From the discussion in the b2g-dev mail list [1],
> there is one use-case where the carrier provides different rate for sending
> SMS to different SMSC address.
> 

Looks like this is on demand?
Can we take advantage of "SmsSendParameters" that we support 'smsc' in that dictionary?

> File this bug to address the possibility for supporting this in gecko side.
> 
> [1]
> https://groups.google.com/forum/#!searchin/mozilla.dev.b2g/SMSC/mozilla.dev.
> b2g/sTB2mLhCRAs/lwzXGV_5TX8J
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> 
> Looks like this is on demand?
> Can we take advantage of "SmsSendParameters" that we support 'smsc' in that
> dictionary?

Yes, RIL_REQUEST_SEND_SMS allows us to pass optional SMSC for sending.
Then, there will be different user-story to apply smsc on the fly:
1. Gaia has to provide an UI to store user-added smsc.
2. This list might have to be clear or disabled if the inserted SIM is changed.

However, IMHO, 
1. the SMSC won't be changed too frequently.
   (flatrate SMSC will always be chosen if user has paid.)
2. A setting for SMSC per SIM shall be good enough to handle this.
   (We already have getSmscAddress() exposed from gecko.)
3. set SMSC API not only provide the capability to send SMS via specified SMSC but
   also provide a way for user to modify the SMSC value in the UICC.

Hence, I suggest to have set SMSC API exposed instead. :)
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #2)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> > 
> > Looks like this is on demand?
> > Can we take advantage of "SmsSendParameters" that we support 'smsc' in that
> > dictionary?
> 
> Yes, RIL_REQUEST_SEND_SMS allows us to pass optional SMSC for sending.
> Then, there will be different user-story to apply smsc on the fly:
> 1. Gaia has to provide an UI to store user-added smsc.
> 2. This list might have to be clear or disabled if the inserted SIM is
> changed.
> 
> However, IMHO, 
> 1. the SMSC won't be changed too frequently.
>    (flatrate SMSC will always be chosen if user has paid.)
> 2. A setting for SMSC per SIM shall be good enough to handle this.
>    (We already have getSmscAddress() exposed from gecko.)
> 3. set SMSC API not only provide the capability to send SMS via specified
> SMSC but
>    also provide a way for user to modify the SMSC value in the UICC.
> 
> Hence, I suggest to have set SMSC API exposed instead. :)

If that's our UX wants, then it's indeed good enough!
Update some interesting finding in bug 1043250 [1] of the SMSC formats returned from different device solutions:
1. In HTC One (M8), the response is readable phone number "+886932400821".
2. In Nexus 5, the response is in form of '"+886932400821",145' which is formed in +CSCA response defined TS 27.005.
3. In Nexus S, a PDU format is shown instead "07913396050096f5" in the RP-Address format defined in TS 24.011.

It seems that the API of RIL_REQUEST_SEND_SMS in AOSP was not specified in detail of how the content of the response string shall be formatted.

A plain text input is more flexible for supporting setting SMSC address in different devices.
However, it introduces bad UX to the user, because the knowledge of SMSC defined in either TS 27.005 or 
TS 24.011 is required. :(

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1043250#c6
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #4)
> Update some interesting finding in bug 1043250 [1] of the SMSC formats
> returned from different device solutions:
> 1. In HTC One (M8), the response is readable phone number "+886932400821".
> 2. In Nexus 5, the response is in form of '"+886932400821",145' which is
> formed in +CSCA response defined TS 27.005.
> 3. In Nexus S, a PDU format is shown instead "07913396050096f5" in the
> RP-Address format defined in TS 24.011.
> 
> It seems that the API of RIL_REQUEST_SEND_SMS in AOSP was not specified in
> detail of how the content of the response string shall be formatted.
> 
> A plain text input is more flexible for supporting setting SMSC address in
> different devices.
> However, it introduces bad UX to the user, because the knowledge of SMSC
> defined in either TS 27.005 or 
> TS 24.011 is required. :(
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1043250#c6

Is it able to handle the format transformation in gecko and still let user to input readable phone number?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #4)
> > Update some interesting finding in bug 1043250 [1] of the SMSC formats
> > returned from different device solutions:
> > 1. In HTC One (M8), the response is readable phone number "+886932400821".
> > 2. In Nexus 5, the response is in form of '"+886932400821",145' which is
> > formed in +CSCA response defined TS 27.005.
> > 3. In Nexus S, a PDU format is shown instead "07913396050096f5" in the
> > RP-Address format defined in TS 24.011.
> > 
> > It seems that the API of RIL_REQUEST_SEND_SMS in AOSP was not specified in
> > detail of how the content of the response string shall be formatted.
> > 
> > A plain text input is more flexible for supporting setting SMSC address in
> > different devices.
> > However, it introduces bad UX to the user, because the knowledge of SMSC
> > defined in either TS 27.005 or 
> > TS 24.011 is required. :(
> > 
> > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1043250#c6
> 
> Is it able to handle the format transformation in gecko and still let user
> to input readable phone number?

Curious, how does other phone's UI look like?
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> > (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #4)
> > > Update some interesting finding in bug 1043250 [1] of the SMSC formats
> > > returned from different device solutions:
> > > 1. In HTC One (M8), the response is readable phone number "+886932400821".
> > > 2. In Nexus 5, the response is in form of '"+886932400821",145' which is
> > > formed in +CSCA response defined TS 27.005.
> > > 3. In Nexus S, a PDU format is shown instead "07913396050096f5" in the
> > > RP-Address format defined in TS 24.011.
> > > 
> > > It seems that the API of RIL_REQUEST_SEND_SMS in AOSP was not specified in
> > > detail of how the content of the response string shall be formatted.
> > > 
> > > A plain text input is more flexible for supporting setting SMSC address in
> > > different devices.
> > > However, it introduces bad UX to the user, because the knowledge of SMSC
> > > defined in either TS 27.005 or 
> > > TS 24.011 is required. :(
> > > 
> > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1043250#c6
> > 
> > Is it able to handle the format transformation in gecko and still let user
> > to input readable phone number?
There are some difficulties listed as followed:
1. We have to read it first to know what format shall be applied.
   However, we have no idea if this field is empty. (Might be cleared by the user.)
2. Some possible value becomes ambiguous in PDU and readable smsc address formats:
  - Take "0481339605" as an example, it could be a readable smsc address but also a RP-Destination Address:
    - "06" - Length of RP-Destination Address contents
    - "81" - Type of Address
    - "339605" -> Digits - "3369500069"
> 
> Curious, how does other phone's UI look like?
I can go to a retail store to see what other phones behave. :)
For HTC One (m8) in my hand, the built in message app provides the access to the SMSC in UICC and a readable address like "+886932400821" is presented in the UI.
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #7)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #5)
> > > (In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #4)
> > > > Update some interesting finding in bug 1043250 [1] of the SMSC formats
> > > > returned from different device solutions:
> > > > 1. In HTC One (M8), the response is readable phone number "+886932400821".
> > > > 2. In Nexus 5, the response is in form of '"+886932400821",145' which is
> > > > formed in +CSCA response defined TS 27.005.
> > > > 3. In Nexus S, a PDU format is shown instead "07913396050096f5" in the
> > > > RP-Address format defined in TS 24.011.
> > > > 
> > > > It seems that the API of RIL_REQUEST_SEND_SMS in AOSP was not specified in
> > > > detail of how the content of the response string shall be formatted.
> > > > 
> > > > A plain text input is more flexible for supporting setting SMSC address in
> > > > different devices.
> > > > However, it introduces bad UX to the user, because the knowledge of SMSC
> > > > defined in either TS 27.005 or 
> > > > TS 24.011 is required. :(
> > > > 
> > > > [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1043250#c6
> > > 
> > > Is it able to handle the format transformation in gecko and still let user
> > > to input readable phone number?
> There are some difficulties listed as followed:
> 1. We have to read it first to know what format shall be applied.
>    However, we have no idea if this field is empty. (Might be cleared by the
> user.)
> 2. Some possible value becomes ambiguous in PDU and readable smsc address
> formats:
>   - Take "0481339605" as an example, it could be a readable smsc address but
> also a RP-Destination Address:
>     - "06" - Length of RP-Destination Address contents
>     - "81" - Type of Address
>     - "339605" -> Digits - "3369500069"
> > 
> > Curious, how does other phone's UI look like?
> I can go to a retail store to see what other phones behave. :)
> For HTC One (m8) in my hand, the built in message app provides the access to
> the SMSC in UICC and a readable address like "+886932400821" is presented in
> the UI.

Hmmm, then I think we shall have UX input first before diving into the API design.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> > > > Is it able to handle the format transformation in gecko and still let user
> > > > to input readable phone number?
> > There are some difficulties listed as followed:
> > 1. We have to read it first to know what format shall be applied.
> >    However, we have no idea if this field is empty. (Might be cleared by the
> > user.)
> > 2. Some possible value becomes ambiguous in PDU and readable smsc address
> > formats:
> >   - Take "0481339605" as an example, it could be a readable smsc address but
> > also a RP-Destination Address:
> >     - "06" - Length of RP-Destination Address contents
> >     - "81" - Type of Address
> >     - "339605" -> Digits - "3369500069"
> > > 
> > > Curious, how does other phone's UI look like?
> > I can go to a retail store to see what other phones behave. :)
> > For HTC One (m8) in my hand, the built in message app provides the access to
> > the SMSC in UICC and a readable address like "+886932400821" is presented in
> > the UI.
> 
> Hmmm, then I think we shall have UX input first before diving into the API
> design.

I think a built-time preference or a system property from ril daemon is needed to differentiate how the physical device interpret it.
blocking-b2g: --- → backlog
Whiteboard: [grooming]
Blocks: b2g-sms
Assignee: btseng → sawang
Just a note. It looks we should not enable any national languages by default except GSM default alphabet according to 3GPP TS 23.038 [1] 6.2.1.2.5 NOTE 2:
> Encoding of a message using the national locking shift mechanism is not intended to be implemented until
> a formal request is issued by the relevant national regulatory body. This is because a receiving entity 
> not supporting the relevant locking-shift decoding will present different characters from the ones 
> intended by the sending entity.

[1] http://www.etsi.org/deliver/etsi_ts/123000_123099/123038/12.00.00_60/ts_123038v120000p.pdf
Forget about the previous comment. Wrong ticket.

Had a discussion with Bevis. The major concern is that if the API accepts only a simple number without specifying the type-of-address, then the API might not be able to support all TOAs defined in TS 23.040 [1].

We took a look at Android's implementation [2]. It simply put TOA as "unknown" if the number doesn't begin with '+' sign. 

I was kind of curious about how the major player in the last decades did, so I took a look at Symbian's implementation [3] and it turns out to be the same as Android:

>  void CSmsAddress::SetAddressL(const TDesC& aAddress)
>	{
>	OstTraceDef0(OST_TRACE_CATEGORY_DEBUG, TRACE_INTERNALS, CSMSADDRESS_SETADDRESSL_1, "CSmsAddress::SetAddressL()");
>	TInt length=aAddress.Length();
>	NewBufferL(length);
>	iBuffer->Des().Copy(aAddress);
>	const TGsmSmsTypeOfNumber typeofnumber=length && (iBuffer->Des()[0]=='+')? EGsmSmsTONInternationalNumber: EGsmSmsTONUnknown;
>	iTypeOfAddress.SetTON(typeofnumber);
>	} // CSmsAddress::SetAddressL

[1] http://www.etsi.org/deliver/etsi_ts/123000_123099/123040/12.02.00_60/ts_123040v120200p.pdf
[2] https://github.com/android/platform_frameworks_base/blob/master/telephony/java/android/telephony/PhoneNumberUtils.java#L1094
[3] http://sourceforge.net/projects/symbiandump/?source=typ_redirect
blocking-b2g: backlog → ---
I have a proposal that we provide 2 APIs for this feature.

The first one supports only human-readable numbers, and behaves as Android / Symbian implementations, i.e. use the '+' symbol to choose type-of-number between "international" or "unknown"; While the second one uses raw PDU format, which essentially supports any type-of-number and number-plan-identifier specified in 3GPP standards.

The point is that we don't really need to support all representation formats (readable number, +CSCA response, PDU format). What we want is keeping the possibility of setting TON/NPI from the caller. By providing 2 APIs separately, it keeps most Gaia app developers away from the technical details, while still has the flexibility of making "raw" configurations.

>  [Throws]
>  DOMRequest setSmscAddress(DOMString readableAddress, optional unsigned long serviceId);
> 
>  [Throws]
>  DOMRequest setRawSmscAddress(DOMSring rawPduAddress, optional unsigned long serviceId);

=====

Another option is adding an optional field in setSmscAddress, which might look like this:
>  [Throws]
>  DOMRequest setSmscAddress(DOMString readableAddress, optional unsigned long serviceId, optional TypeOfNumberEnum ton);

In this design, the address field is always a readable number. If callers need to specify TON, it's passed as another argument.

However it's more confusing since Gaia app developers might need to know what TON means before they can ensure that ignoring the argument is safe.
Per offline discussion,

To be more flexible to both normal user and the user who requires specific TOA other then 
"international", we could summary these APIs as followed:
1. Modify the result of getSmscAddress() from a DOMString to a SmscAddress dictionary:
   enum TypeOfNumber { "international", "national", ... };
   enum NumberPlanIdentification { "unknown", "isdn", ... };
   dictionary TypeOfAddress
   {
     TypeOfNumber ton;
     NumberPlanIdentification npi;
   };
   dictionary SmscAddress
   {
     DOMString? address;
     TypeOfAddress? toa;
   }
2. define |setSmsAddress| as followed:
   [Throws]
   DOMRequest setSmscAddress(DOMString address, long serviceId, optional TypeOfAddress toa);

With this design,
For normal user using gaia settings, we can simply get/set smsc address with 'unknown' TOA or 'international' TOA if "+" prefix presents in the parameter of |address|.
For advance user, these API design provides the possibility to get/set additional TOA of the SMSC address.

To support this new design,
1. Gaia change is required for backward-compatibility.
2. The migration of the interface change in nsISmsService.idl after ril-interface-frozen is required.
3. Build configuration of the device to identify that the support of +CSCA is in Text Mode/PDU mode is required.

Hi Hsinyi,

May we have your feedback of this proposal and do you have any idea/suggestion about how to have a
build configuration to identify modem's capability in step 3) mentioned above?

Thanks!
Flags: needinfo?(htsai)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #13)
> Hi Hsinyi,
> 
> May we have your feedback of this proposal and do you have any
> idea/suggestion about how to have a
> build configuration to identify modem's capability in step 3) mentioned
> above?

Just saw your comment in bug 1043250 comment 12 regarding the device configuration. :)
Generally, I like the proposal, thanks Bevis and Samael. :)
Let's also invite Julien to the discussion!

Some minor comments/questions inline:

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #13)
> Per offline discussion,
> 
> To be more flexible to both normal user and the user who requires specific
> TOA other then 
> "international", we could summary these APIs as followed:
> 1. Modify the result of getSmscAddress() from a DOMString to a SmscAddress
> dictionary:
>    enum TypeOfNumber { "international", "national", ... };
>    enum NumberPlanIdentification { "unknown", "isdn", ... };
>    dictionary TypeOfAddress
>    {
>      TypeOfNumber ton;
>      NumberPlanIdentification npi;
>    };
>    dictionary SmscAddress
>    {
>      DOMString? address;
>      TypeOfAddress? toa;
>    }
> 2. define |setSmsAddress| as followed:
>    [Throws]
>    DOMRequest setSmscAddress(DOMString address, long serviceId, optional
> TypeOfAddress toa);
> 

Why can't it be setSmscAddress(SmscAddress address, optional long serviceId)?

Apart from that, please use Promise for a new WebAPI or a modified one if possible.

> With this design,
> For normal user using gaia settings, we can simply get/set smsc address with
> 'unknown' TOA or 'international' TOA if "+" prefix presents in the parameter
> of |address|.
> For advance user, these API design provides the possibility to get/set
> additional TOA of the SMSC address.
> 
> To support this new design,
> 1. Gaia change is required for backward-compatibility.
> 2. The migration of the interface change in nsISmsService.idl after
> ril-interface-frozen is required.
> 3. Build configuration of the device to identify that the support of +CSCA
> is in Text Mode/PDU mode is required.

That's true. Let's take the Flame behaviour as default and correct the behaviour on other devices if needed.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #15)
> Generally, I like the proposal, thanks Bevis and Samael. :)
> Let's also invite Julien to the discussion!
> 
> Some minor comments/questions inline:
> 
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #13)
> > Per offline discussion,
> > 
> > To be more flexible to both normal user and the user who requires specific
> > TOA other then 
> > "international", we could summary these APIs as followed:
> > 1. Modify the result of getSmscAddress() from a DOMString to a SmscAddress
> > dictionary:
> >    enum TypeOfNumber { "international", "national", ... };
> >    enum NumberPlanIdentification { "unknown", "isdn", ... };
> >    dictionary TypeOfAddress
> >    {
> >      TypeOfNumber ton;
> >      NumberPlanIdentification npi;
> >    };
> >    dictionary SmscAddress
> >    {
> >      DOMString? address;
> >      TypeOfAddress? toa;
> >    }
> > 2. define |setSmsAddress| as followed:
> >    [Throws]
> >    DOMRequest setSmscAddress(DOMString address, long serviceId, optional
> > TypeOfAddress toa);
> > 
> 
> Why can't it be setSmscAddress(SmscAddress address, optional long serviceId)?
Yes, this looks better.
I was struggling that gaia can just simply provide the number of smsc address 
and they don't have to know what TOA means.
Your proposal is better and gaia can just provide SmsAddress.address if TOA is not important.
> 
> Apart from that, please use Promise for a new WebAPI or a modified one if
> possible.
Thanks for reminding this.
> > With this design,
> > For normal user using gaia settings, we can simply get/set smsc address with
> > 'unknown' TOA or 'international' TOA if "+" prefix presents in the parameter
> > of |address|.
> > For advance user, these API design provides the possibility to get/set
> > additional TOA of the SMSC address.
> > 
> > To support this new design,
> > 1. Gaia change is required for backward-compatibility.
> > 2. The migration of the interface change in nsISmsService.idl after
> > ril-interface-frozen is required.
> > 3. Build configuration of the device to identify that the support of +CSCA
> > is in Text Mode/PDU mode is required.
> 
> That's true. Let's take the Flame behaviour as default and correct the
> behaviour on other devices if needed.
Hi Julien,

As what we found in bug 1043250 comment 6 and what we discussed in this bug,
to make our implementation flexible to get/set SMSC address in different H/W platform, 
we'd like to revise existing getSmscAddress() and define setSmscAddress() to the following new design.

May we have your feedback to see if these APIs is okay from Gaia's viewpoint?

Thanks!
--
The purpose of the design change is to:
1. Ensure that we could interpret the SMSC properly and display it as a normal phone number in Gaia.
2. Provide an easy way for Gaia to modify SMSC from Mobile Message Settings by supplying a different phone number to change SMSC or empty to clear the SMSC.
3. Keep the possibility in Web APIs to set all possible types of SMSC according to |8.2.5.2 Destination address element| in 3GPP TS 24.011[1] and |10.5.4.7 Called party BCD number| in 3GPP TS 24.008 [2].

Here is the new design of these APIs:
>   enum TypeOfNumber { "unknown", "international", "national", ... };
>
>   enum NumberPlanIdentification { "unknown", "isdn", ... };
>
>   dictionary TypeOfAddress
>   {
>     TypeOfNumber ton;
>     NumberPlanIdentification npi;
>   };
>
>   dictionary SmscAddress
>   {
>     DOMString? address;
>     TypeOfAddress? toa;
>   };
>
>   [NewObject]
>   Promise<SmscAddress> getSmscAddress(optional long serviceId);
>
>   [NewObject]
>   Promise<void> setSmscAddress(SmscAddress address, optional long serviceId);

[1] http://www.etsi.org/deliver/etsi_ts/124000_124099/124011/12.00.00_60/ts_124011v120000p.pdf
[2] http://www.etsi.org/deliver/etsi_ts/124000_124099/124008/12.08.00_60/ts_124008v120800p.pdf
Flags: needinfo?(felash)
I think this looks good but because this code will live in Settings I'm forwarding the question to Arthur.

Hey Arthur, could you please provide feedback to comment 17?
Flags: needinfo?(felash) → needinfo?(arthur.chen)
That looks good to me. Although currently there is no use case of `setSmscAddress` in gaia, so I may not be able to give comments.
Flags: needinfo?(arthur.chen)
I would like to simplify the structure

>   enum TypeOfNumber { "unknown", "international", "national", ... };
>   enum NumberPlanIdentification { "unknown", "isdn", ... };
>   dictionary TypeOfAddress
>   {
>     TypeOfNumber ton;
>     NumberPlanIdentification npi;
>   };
> 
>   dictionary SmscAddress
>   {
>     DOMString? address;
>     TypeOfAddress? toa;
>   };

To 

>   enum TypeOfNumber { "unknown", "international", "national", ... };
>   enum NumberPlanIdentification { "unknown", "isdn", ... };
> 
>   dictionary SmscAddress
>   {
>     DOMString? address;
>     TypeOfNumber? ton;
>     NumberPlanIdentification? npi;
>   };

For simplification and for the reason that "type of address" seems to be kind of SM-TL term -- the term is only mentioned in |9.1.2.5 Address fields| of TS 23.040 [1] for SM-TL addressing, but not in |8.2.5.2 Destination address element| of 3GPP TS 24.011[2] or |10.5.4.7 Called party BCD number| of 3GPP TS 24.008 [3] for SM-RL addressing.

Just a very minor change. Does anyone have concern on this?

[1] http://www.etsi.org/deliver/etsi_ts/123000_123099/123040/12.02.00_60/ts_123040v120200p.pdf
[2] http://www.etsi.org/deliver/etsi_ts/124000_124099/124011/12.00.00_60/ts_124011v120000p.pdf
[3] http://www.etsi.org/deliver/etsi_ts/124000_124099/124008/12.08.00_60/ts_124008v120800p.pdf
(In reply to Samael Wang [:freesamael][:sawang] from comment #20)
> I would like to simplify the structure
> 
> >   enum TypeOfNumber { "unknown", "international", "national", ... };
> >   enum NumberPlanIdentification { "unknown", "isdn", ... };
> >   dictionary TypeOfAddress
> >   {
> >     TypeOfNumber ton;
> >     NumberPlanIdentification npi;
> >   };
> > 
> >   dictionary SmscAddress
> >   {
> >     DOMString? address;
> >     TypeOfAddress? toa;
> >   };
> 
> To 
> 
> >   enum TypeOfNumber { "unknown", "international", "national", ... };
> >   enum NumberPlanIdentification { "unknown", "isdn", ... };
> > 
> >   dictionary SmscAddress
> >   {
> >     DOMString? address;
> >     TypeOfNumber? ton;
> >     NumberPlanIdentification? npi;
> >   };
> 
My concern is that TON/NPI are represented in pair.
If TOA has to be specified, TON/NPI are required.
Looks good to me :)

I've a concern with the names though: "ton" and "npi" are really cryptic. Can we use "typeOfNumber" and "numberPlanIdentification" instead?

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #21)
> My concern is that TON/NPI are represented in pair.
> If TOA has to be specified, TON/NPI are required.

And you'd rather handle this through WebIDL than through code, I guess?
(In reply to Julien Wajsberg [:julienw] from comment #22)
> Looks good to me :)
> 
> I've a concern with the names though: "ton" and "npi" are really cryptic.
> Can we use "typeOfNumber" and "numberPlanIdentification" instead?
> 
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #21)
> > My concern is that TON/NPI are represented in pair.
> > If TOA has to be specified, TON/NPI are required.
> 
> And you'd rather handle this through WebIDL than through code, I guess?

Yes, I'd suggest to keep the one in comment 17 and rename the toa/ton/npi to be more readable.
Some updates on the interface due to compilation error:

=====

> [NewObject]
> Promise<void> setSmscAddress(SmscAddress address, optional long serviceId);

Compiler complains "Dictionary argument or union argument containing a dictionary not followed by a required argument must be optional.", so it's changed to

> [NewObject]
> Promise<void> setSmscAddress(optional SmscAddress address, optional long serviceId);

instead.

=====

> dictionary SmscAddress
> {
>   DOMString? address;
>   TypeOfAddress? typeOfAddress;
> };

Compiler complains "Dictionary SmscAddress has member with nullable dictionary type". 

Found a mail discussion [1] on this. It mentions nullable dictionary is not allowed anymore.

Further, it turns out all members of a dictionary type are optional, as described in |3.3. Dictionaries| of Web IDL standard [2]:

> On a given dictionary value, the presence of each dictionary member is optional.

The difference between specifying ? or not on a dictionary member is that a nullable member can be "present", "not present" or "null"; while a regular member can be "present" or "not present".

However the standard also mentioned that default value is possible:

> Dictionary members can also optionally have a default value, which is the value the dictionary member 
> is to be considered to have when not present.

Hence we might consider adding default value to NPI:

> dictionary TypeOfAddress {
>   TypeOfNumber typeOfNumber;
>   NumberPlanIdentification numberPlanIdentification = "isdn";
> };

However we'll still need to take care of the case that typeOfNumber doesn't present. Do we still stick with the TypeOfAddress dictionary design in this case?

[1] http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0385.html
[2] http://www.w3.org/TR/WebIDL/#dfn-dictionary
(In reply to Samael Wang [:freesamael][:sawang] from comment #24)
> Some updates on the interface due to compilation error:
> 
> =====
> 
> > [NewObject]
> > Promise<void> setSmscAddress(SmscAddress address, optional long serviceId);
> 
> Compiler complains "Dictionary argument or union argument containing a
> dictionary not followed by a required argument must be optional.",

Do you know the rationale behind this rule?
(In reply to Julien Wajsberg [:julienw] from comment #25)
> (In reply to Samael Wang [:freesamael][:sawang] from comment #24)
> > Some updates on the interface due to compilation error:
> > 
> > =====
> > 
> > > [NewObject]
> > > Promise<void> setSmscAddress(SmscAddress address, optional long serviceId);
> > 
> > Compiler complains "Dictionary argument or union argument containing a
> > dictionary not followed by a required argument must be optional.",
> 
> Do you know the rationale behind this rule?

It's specified in |3.2.3. Operations| of WebIDL 2nd edition draft [1]:

> If the type of an argument is a dictionary type or a union type that has a dictionary type 
> as one of its flattened member types, and this argument is either the final argument or 
> is followed only by optional arguments, then the argument MUST be specified as optional. 
> Such arguments are always considered to have a default value of an empty dictionary, 
> unless otherwise specified. 

The discussion behind this spec can be found in a W3C bug [2]. 

We could also think if we should re-design the API rather than just fulfill the spec.

[1] http://heycam.github.io/webidl/#idl-operations
[2] https://www.w3.org/Bugs/Public/show_bug.cgi?id=16725
Thanks for the pointers, it's very informative!
Attachment #8592639 - Attachment is obsolete: true
Attachment #8592643 - Attachment description: Part 6: Add marionette and xpcshell test cases → Part 6(v2): Add marionette and xpcshell test cases
Attachment #8592634 - Flags: review?(btseng)
Attachment #8592635 - Flags: review?(btseng)
Attachment #8592636 - Flags: review?(btseng)
Attachment #8592637 - Flags: review?(btseng)
Attachment #8592638 - Flags: review?(btseng)
Attachment #8592643 - Flags: review?(btseng)
(In reply to Samael Wang [:freesamael][:sawang] from comment #35)
> try result:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6f7bb729f2

For DOM API changes, please have all platform built & tested to ensure no compiled error and function broken in other platform. :)

Thanks!
Attachment #8593193 - Attachment description: Part 4: Update IPC protocols and implementation for setSmscAddress API → Part 4(v2): Update IPC protocols and implementation for setSmscAddress API
Comment on attachment 8592637 [details] [diff] [review]
Part 4: Update IPC protocols and implementation for setSmscAddress API

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

Recall for building failure.
Attachment #8592637 - Flags: review?(btseng)
Attachment #8592637 - Attachment is obsolete: true
Attachment #8593210 - Attachment description: Part 5: Add dummy implementation for Android backend → Part 5(v2): Add dummy implementation for Android backend
Attachment #8593210 - Attachment is obsolete: true
Attachment #8593211 - Attachment description: Part 5: Add dummy implementation for Android backend → Part 5(v3): Add dummy implementation for Android backend
Comment on attachment 8592638 [details] [diff] [review]
Part 5: Add dummy implementation for Android backend

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

Recall for building failure.
Attachment #8592638 - Flags: review?(btseng)
Attachment #8592638 - Attachment is obsolete: true
Attachment #8593193 - Flags: review?(btseng)
Attachment #8593211 - Flags: review?(btseng)
Comment on attachment 8592636 [details] [diff] [review]
Part 3: Add setSmscAddress implementation for Gonk backend

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

Please see my comment inlined.

Thank you!

::: dom/mobilemessage/interfaces/nsISmsService.idl
@@ +56,5 @@
>    void removeSilentNumber(in DOMString number);
>  
>    void getSmscAddress(in unsigned long serviceId,
>                        in nsIMobileMessageCallback request);
> +

Let's define all the TON/NPI constants here and have the protection in compiled time to ensure the value is the same to the one in .webidl. You can refer to what have been done in ICC. [1] Then, we can remove the explanation related to the implementation from the webidl. :)

In addition, since this idl will be referred by vendor, can you help to document this idl in this bug?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/icc/Assertions.cpp

::: dom/system/gonk/ril_worker.js
@@ +44,5 @@
>  importScripts("ril_consts.js");
>  importScripts("resource://gre/modules/workers/require.js");
>  importScripts("ril_worker_buf_object.js");
>  importScripts("ril_worker_telephony_request_queue.js");
> +importScripts("resource://gre/modules/systemlibs.js");

Let's follow the rule to put the QUIRKs together in common place at [1].

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#1530-1546

@@ +1743,5 @@
> +    let ton = options.typeOfNumber;
> +    let npi = options.numberPlanIdentification;
> +
> +    let ADDR_FMT =
> +      libcutils.property_get("ro.moz.ril.smsc_address_format", "text");

Ditto.

@@ +1776,5 @@
> +          'F' /* end mark code 1111 */ : pureNumber[idx + 1];
> +        semiOctets[4 + idx + 1] = pureNumber[idx];
> +      }
> +
> +      sca = semiOctets.join("");

You can refer to what have been done in writeDiallingNumber
https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#9644-9654

@@ +1787,2 @@
>      let Buf = this.context.Buf;
>      Buf.newParcel(REQUEST_SET_SMSC_ADDRESS, options);

Wrap all the encoding logic between "Buf.newParcel(REQUEST_SET_SMSC_ADDRESS, options);" & "Buf.sendParcel();"
Then you can reused refer to the logic in writeDiallingNumber() with additional logic of writing the length of SMSC address for PDU mode.

@@ +5607,5 @@
> +    this.sendChromeMessage({
> +      rilMessageType: options.rilMessageType,
> +      rilMessageToken: options.rilMessageToken,
> +    });
> +  }

You can simplify the entire logic as followed:
  if (options.rilRequestError) {
    options.errorMsg = RIL_ERROR_TO_GECKO_ERROR[options.rilRequestError];
  }

  this.sendChromeMessage(options);
Attachment #8592636 - Flags: review?(btseng)
Comment on attachment 8593193 [details] [diff] [review]
Part 4(v2): Update IPC protocols and implementation for setSmscAddress API. r=btseng

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

Looks good to me.
Thank you!
Attachment #8593193 - Flags: review?(btseng) → review+
Comment on attachment 8593211 [details] [diff] [review]
Part 5(v3): Add dummy implementation for Android backend. r=btseng

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

Looks good to me.

Thanks!
Attachment #8593211 - Flags: review?(btseng) → review+
Comment on attachment 8592635 [details] [diff] [review]
Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress. r=btseng

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

Looks good to me.
Thanks!
Attachment #8592635 - Flags: review?(btseng) → review+
Comment on attachment 8592643 [details] [diff] [review]
Part 6(v2): Add marionette and xpcshell test cases

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

I'd like to review this patch again after the QUIRK has been added in patch part#3. :)

::: dom/mobilemessage/tests/marionette/test_set_smsc_address.js
@@ +9,5 @@
> +const SMSC_O2 = '+447802000332';
> +const SMSC_O2_TEXT = '"+447802000332",145';
> +const SMSC_DEF = '+123456789';
> +const SMSC_DEF_TEXT = '"+123456789",145';
> +

I think we should have some protection of the number passed to ril_worker.
We can filter valid BCD numbers before sending the parcel to the modem. [1]
And have additional test case here to ensure that we can set the SMSC successfully if invalid characters included here.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#9683-9689

::: dom/system/gonk/tests/test_ril_worker_smsc_address.js
@@ +7,5 @@
> +const SMSC_ATT_TEXT = '"+13123149810",145';
> +const SMSC_ATT_PDU = '07913121139418F0';
> +const SMSC_O2 = '+447802000332';
> +const SMSC_O2_TEXT = '"+447802000332",145';
> +const SMSC_O2_PDU = '0791448720003023';

I think we should have some protection of the number passed to ril_worker.
We can filter valid BCD numbers before sending the parcel to the modem. [1]
And have additional test case here to ensure that we can set the SMSC successfully if invalid characters included here.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js#9683-9689

@@ +37,5 @@
> +  })();
> +  worker.libcutils = libcutils;
> +
> +  // Test text mode.
> +  libcutils.property_set("ro.moz.ril.smsc_address_format", "text");

Just change the worker.RILQUIRKS_XXX here for testing of text/pdu mode.
Attachment #8592643 - Flags: review?(btseng)
Comment on attachment 8592634 [details] [diff] [review]
Part 1: Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class

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

Please see my comment inlined.

Thanks!

::: dom/mobilemessage/MobileMessageManager.cpp
@@ +747,5 @@
> +  // If the address begins with +, set TON to international no matter what has
> +  // passed in.
> +  if (!address.IsEmpty() && address[0] == '+') {
> +    ton = TypeOfNumber::International;
> +  }

I think we can put this together in ril_worker since that's the only place we check the format of smsc address.

::: dom/mobilemessage/MobileMessageManager.h
@@ +7,5 @@
>  #define mozilla_dom_mobilemessage_MobileMessageManager_h
>  
>  #include "mozilla/Attributes.h"
>  #include "mozilla/DOMEventTargetHelper.h"
> +#include "mozilla/dom/Promise.h"

forward declaration for Promise instead.

::: dom/webidl/MozMobileMessageManager.webidl
@@ +81,5 @@
> + * |8.2.5.2 Destination address element| of 3GPP TS 24.011.
> + *
> + * Note the value is directly converted to uint32_t in underlying implementation,
> + * hence the order of the enumeration must follow the standard to represent
> + * correct values.

Let's remove these notes add add assertion for the protection in compiled time instead.

@@ +93,5 @@
> + * |8.2.5.2 Destination address element| of 3GPP TS 24.011.
> + *
> + * Note the value is directly converted to uint32_t in underlying implementation,
> + * hence the order of the enumeration must follow the standard to represent
> + * correct values.

ditto.
Attachment #8592634 - Flags: review?(btseng)
Attachment #8597069 - Attachment description: Part 1: Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class → Part 1(v2): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class
Attachment #8597070 - Attachment description: Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress → Part 2(rebase): Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress
Attachment #8597070 - Attachment is obsolete: true
Attachment #8597072 - Attachment description: Part 4: Update IPC protocols and implementation for setSmscAddress API → Part 4(rebase): Update IPC protocols and implementation for setSmscAddress API
Attachment #8597072 - Attachment is obsolete: true
Attachment #8597073 - Attachment description: Part 5: Add dummy implementation for Android backend → Part 5(rebase): Add dummy implementation for Android backend
Attachment #8597073 - Attachment is obsolete: true
Comment on attachment 8597071 [details] [diff] [review]
Part 3(v2): Add setSmscAddress implementation for Gonk backend

Please note nsISmsService.idl is moved to the new patch "Part 7" for better organization.
Attachment #8597071 - Attachment description: Part 3: Add setSmscAddress implementation for Gonk backend → Part 3(v2): Add setSmscAddress implementation for Gonk backend
Attachment #8597074 - Attachment description: Part 6: Add marionette and xpcshell test cases → Part 6(v3): Add marionette and xpcshell test cases
Attachment #8592634 - Attachment is obsolete: true
Attachment #8592636 - Attachment is obsolete: true
Attachment #8592643 - Attachment is obsolete: true
Attachment #8597069 - Flags: review?(btseng)
Attachment #8597071 - Flags: review?(btseng)
Attachment #8597074 - Flags: review?(btseng)
Attachment #8597075 - Flags: review?(btseng)
Hi Bevis,

Could you help to review the updated patch when you have time? Please note that I've moved nsISmsService.idl from "Part 4" to the new patch "Part 7", along with new changes in moz.build and Assertions.cpp.
Comment on attachment 8597069 [details] [diff] [review]
Part 1(v2): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class

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

Looks ok to me with the following comment addressed.

Since .webidl was modified in this patch, we need DOM peer's review as well.

Please address the comment and then have review request to Hsinyi.

Thanks!

::: dom/webidl/MozMobileMessageManager.webidl
@@ +201,5 @@
> +   * @param smscAddress SMSC address to use. It returns an error if
> +   * smscAddress.address does not present.
> +   * @param serviceId The ID of the RIL service which needs to be specified
> +   * under the multi-sim scenario.
> +   */

Let's has better alignment as followed:
/**
   * Set the SMSC address.
   * @param smscAddress 
   *        SMSC address to use.
   *        Reject if smscAddress.address is not presented.
   * @param serviceId (Optional)
   *        The ID of the RIL service which needs to be specified under 
   *        the multi-sim scenario.
   * @return a Promise
   *         Resolve if success. Otherwise, reject with error cause.
   */
Attachment #8597069 - Flags: review?(btseng) → review+
Comment on attachment 8597075 [details] [diff] [review]
Part 7: Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl

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

Please see my comment below.

Thanks!

::: dom/mobilemessage/interfaces/nsISmsService.idl
@@ +69,5 @@
>  
> +  /**
> +   * Get the information necessary to create a multi-part SMS for a given text.
> +   *
> +   * @param text The text message content.

Have the description started in 2nd line and aligned with the parameter name.

@@ +70,5 @@
> +  /**
> +   * Get the information necessary to create a multi-part SMS for a given text.
> +   *
> +   * @param text The text message content.
> +   * @paramName request The callback object to use. It invokes

Have the description started in 2nd line and aligned with the parameter name.
:s/paramName/param/g

@@ +80,5 @@
>  
> +  /**
> +   * Send a SMS.
> +   *
> +   * @param serviceId The ID of RIL service to use.

Have the description started in 2nd line and aligned with the parameter name.

@@ +81,5 @@
> +  /**
> +   * Send a SMS.
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Destination number in string.

ditto.
no need for "in string" :)

@@ +82,5 @@
> +   * Send a SMS.
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Destination number in string.
> +   * @param message The text message content.

ditto.

@@ +83,5 @@
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Destination number in string.
> +   * @param message The text message content.
> +   * @param silent |true| to send a silent message.

ditto.

@@ +84,5 @@
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Destination number in string.
> +   * @param message The text message content.
> +   * @param silent |true| to send a silent message.
> +   * @param request The callback object to use. It invokes |notifyMessageSent|

ditto.

@@ +93,5 @@
>              in DOMString message,
>              in boolean silent,
>              in nsIMobileMessageCallback request);
>  
>    void addSilentNumber(in DOMString number);

silent number is used by Payment & MobileIdentityManager for the certification.
SmsServie is help to maintain the list of the silent numbers.
Please help to have some documentation for this.

Thank!

https://dxr.mozilla.org/mozilla-central/search?q=addSilentNumber&case=false

@@ +99,5 @@
>  
> +  /**
> +   * Get the short message service center address of given |serviceId|.
> +   *
> +   * @param serviceId The ID of RIL service to use.

ditto.

@@ +100,5 @@
> +  /**
> +   * Get the short message service center address of given |serviceId|.
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param request The callback object to use. It invokes

ditto.

@@ +110,5 @@
> +
> +  /**
> +   * Set the short message service center address of given |serviceId|.
> +   *
> +   * @param serviceId The ID of RIL service to use.

ditto

@@ +111,5 @@
> +  /**
> +   * Set the short message service center address of given |serviceId|.
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Number part of the SMSC address.

ditto

@@ +112,5 @@
> +   * Set the short message service center address of given |serviceId|.
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Number part of the SMSC address.
> +   * @param typeOfNumber Type of number of the SMSC address.

ditto

@@ +113,5 @@
> +   *
> +   * @param serviceId The ID of RIL service to use.
> +   * @param number Number part of the SMSC address.
> +   * @param typeOfNumber Type of number of the SMSC address.
> +   * @param numberPlanIdentification Number plan identification of the SMSC

ditto

@@ +115,5 @@
> +   * @param number Number part of the SMSC address.
> +   * @param typeOfNumber Type of number of the SMSC address.
> +   * @param numberPlanIdentification Number plan identification of the SMSC
> +   *   address.
> +   * @param request The callback object to use. It invokes

ditto
Attachment #8597075 - Flags: review?(btseng)
Comment on attachment 8597071 [details] [diff] [review]
Part 3(v2): Add setSmscAddress implementation for Gonk backend

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

Please check my comment below.

Thanks!

::: dom/mobilemessage/gonk/SmsService.js
@@ +790,5 @@
>        }
>      }
>    },
>  
> +  // An array of silent numbers.

Thanks for correcting this. :)

::: dom/system/gonk/ril_worker.js
@@ +1745,5 @@
> +    let npi = options.numberPlanIdentification;
> +
> +    // If any of the mandatory arguments is not available, return an error
> +    // immediately.
> +    if (!ton || !npi || !options.smscAddress) {

0 is valid for both ton and npi.
Please check precisely with !== undefined.

@@ +1766,5 @@
> +      return;
> +    }
> +
> +    // Init parcel.
> +    this.SMSC = null;

No need for this because 
1. you don't know if the request is success or not at this stage.
2. SMSC will always be get from modem.

I think we can remove 'this.SMSC' when rewriting the getSmscAddress().

@@ +1795,5 @@
> +                             .replace(/c/ig, "e")
> +                             .replace(/b/ig, "d")
> +                             .replace(/a/ig, "c")
> +                             .replace(/\#/g, "b")
> +                             .replace(/\*/g, "a");

Please have these "replace()" with the same order of the BCD number (*, #, a, b, c) as the explanation for better reading.

@@ +1801,5 @@
> +      // address length and string length
> +      let length = Math.ceil(pureNumber.length / 2) + 1; // +1 octet for TOA
> +      let strlen = length * 2 + 2; // +2 semi-octets for length octet
> +
> +      Buf.writeInt32(strlen);

No need for this.
the data structure of RIL_REQUEST_SET_SMSC_ADDRESS is a char*.
What we have to do is to write all the data with GsmPDUHelper for the PDU of the SMSC:
 - Length
 - TOA
 - BCD Numbers.
Then, end of string with 
Buf.writeStringDelimiter(0);

For PDU mode,
Please try to have this tested in Nexus 4 running with your solution to see if everything works fine.
Attachment #8597071 - Flags: review?(btseng)
Hi Bevis,

I'm still a bit confused about the silent number. Is the following description correct?

>  /**
>   * Add a number as a silent message originator. When receiving a SMS sent from
>   * the originator, the message is treated as a silent message and observers
>   * are notified through the topic "silent-sms-received".
>   *
>   * @param number
>   *        Originator number in string.
>   * @throw NS_ERROR_UNEXPECTED
>   *        If the given number has already added before.
>   */
>  void addSilentNumber(in DOMString number);
Comment on attachment 8597074 [details] [diff] [review]
Part 6(v3): Add marionette and xpcshell test cases

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

I like to review this again after the problem of PDU mode in patch part 3 is resolved.
Attachment #8597074 - Flags: review?(btseng)
Comment on attachment 8597211 [details] [diff] [review]
Part 1(v3): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class. r=htsai

Hi HsinYi,

May I have your review on the WebIDL change? Bevis has reviewed this change.
Attachment #8597211 - Attachment description: Part 1: Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class → Part 1(v3): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class
Attachment #8597211 - Flags: review?(htsai)
Attachment #8597069 - Attachment is obsolete: true
Attachment #8597212 - Attachment description: Part 3: Add setSmscAddress implementation for Gonk backend → Part 3(v3): Add setSmscAddress implementation for Gonk backend
Attachment #8597212 - Flags: review?(btseng)
Attachment #8597071 - Attachment is obsolete: true
Attachment #8597214 - Attachment description: Part 6: Add marionette and xpcshell test cases → Part 6(v4): Add marionette and xpcshell test cases
Attachment #8597214 - Flags: review?(btseng)
Attachment #8597074 - Attachment is obsolete: true
Attachment #8597215 - Attachment description: Part 7: Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl → Part 7(v2): Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl
Attachment #8597215 - Flags: review?(btseng)
Attachment #8597075 - Attachment is obsolete: true
Comment on attachment 8597212 [details] [diff] [review]
Part 3(v3): Add setSmscAddress implementation for Gonk backend

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

Looks good to me.

Thanks!
Attachment #8597212 - Flags: review?(btseng) → review+
Comment on attachment 8597215 [details] [diff] [review]
Part 7(v2): Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl. r=btseng

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

Looks good to me.

Thanks!
Attachment #8597215 - Flags: review?(btseng) → review+
Comment on attachment 8597214 [details] [diff] [review]
Part 6(v4): Add marionette and xpcshell test cases. r=btseng

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

Looks good to me.

Thanks!
Attachment #8597214 - Flags: review?(btseng) → review+
Comment on attachment 8597211 [details] [diff] [review]
Part 1(v3): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class. r=htsai

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

It looks really good. Thanks for the efforts :)
Attachment #8597211 - Flags: review?(htsai) → review+
Comment on attachment 8597884 [details] [diff] [review]
Part 3(v4): Add setSmscAddress implementation for Gonk backend. r=btseng

Rebase on latest change and add NPI enumeration / value mappings.
Attachment #8597884 - Attachment description: Part 3: Add setSmscAddress implementation for Gonk backend → Part 3(v4): Add setSmscAddress implementation for Gonk backend
Attachment #8597884 - Flags: review+
Attachment #8597212 - Attachment is obsolete: true
Attachment #8597211 - Attachment description: Part 1(v3): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class → Part 1(v3): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class. r=htsai
Attachment #8592635 - Attachment description: Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress → Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress. r=btseng
Attachment #8597884 - Attachment description: Part 3(v4): Add setSmscAddress implementation for Gonk backend → Part 3(v4): Add setSmscAddress implementation for Gonk backend. r=btseng
Attachment #8593193 - Attachment description: Part 4(v2): Update IPC protocols and implementation for setSmscAddress API → Part 4(v2): Update IPC protocols and implementation for setSmscAddress API. r=btseng
Attachment #8593211 - Attachment description: Part 5(v3): Add dummy implementation for Android backend → Part 5(v3): Add dummy implementation for Android backend. r=btseng
Attachment #8597214 - Attachment description: Part 6(v4): Add marionette and xpcshell test cases → Part 6(v4): Add marionette and xpcshell test cases. r=btseng
Attachment #8597215 - Attachment description: Part 7(v2): Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl → Part 7(v2): Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl. r=btseng
Part 2 needs rebasing.
Keywords: checkin-needed
Attachment #8599051 - Attachment description: Part 1: Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class → Part 1(v3-rebase): Add setSmscAddress API in MozMobileMessageManager Web IDL interface, and corresponding implementation in MobileMessageManager class. r=htsai, btseng
Attachment #8599051 - Flags: review+
Attachment #8597211 - Attachment is obsolete: true
Attachment #8599052 - Attachment description: Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress → Part 2(rebase): Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress. r=btseng
Attachment #8599052 - Flags: review+
Attachment #8592635 - Attachment is obsolete: true
Attachment #8599053 - Attachment description: Part 3: Add setSmscAddress implementation for Gonk backend → Part 3(v4-rebase): Add setSmscAddress implementation for Gonk backend. r=btseng
Attachment #8599053 - Flags: review+
Attachment #8597884 - Attachment is obsolete: true
Attachment #8599054 - Attachment description: Part 4: Update IPC protocols and implementation for setSmscAddress API → Part 4(v4-rebase): Update IPC protocols and implementation for setSmscAddress API. r=btseng
Attachment #8599054 - Flags: review+
Attachment #8593193 - Attachment is obsolete: true
Attachment #8599055 - Attachment description: Part 5: Add dummy implementation for Android backend → Part 5(v3-rebase): Add dummy implementation for Android backend. r=btseng
Attachment #8599055 - Flags: review+
Attachment #8593211 - Attachment is obsolete: true
Attachment #8599056 - Attachment description: Part 6: Add marionette and xpcshell test cases → Part 6(v4-rebase): Add marionette and xpcshell test cases. r=btseng
Attachment #8599056 - Flags: review+
Attachment #8599057 - Attachment description: Part 7: Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl → Part 7(v2-rebase): Update nsISmsService.idl and ensure the type-of-number and number-plan-identification constants match the corresponding enum in MozMobileMessageManager.webidl. r=btseng
Attachment #8599057 - Flags: review+
Attachment #8597214 - Attachment is obsolete: true
Attachment #8597215 - Attachment is obsolete: true
Attachment #8599061 - Attachment description: Part 2: Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress → Part 2(rebase2): Update nsIMobileMessageCallback interface and implementation to support Promise and setSmscAddress. r=btseng
Attachment #8599061 - Flags: review+
Attachment #8599052 - Attachment is obsolete: true
All patches are rebased. Thx.
Flags: in-testsuite+
Blocks: 1177540
See Also: → 1207470
feature-b2g: --- → 2.2r+
Comment on attachment 8668847 [details] [diff] [review]
Add setSmscAddress web API in MozMobileMessageManager.  r=htsai,btseng

Patch for v2.2r uplift. Waiting for try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea58ca5b8e6
Attachment #8668847 - Attachment description: Add setSmscAddress web API in MozMobileMessageManager. btseng → Add setSmscAddress web API in MozMobileMessageManager. r=htsai,btseng
Attachment #8668847 - Attachment is obsolete: true
Comment on attachment 8668862 [details] [diff] [review]
[v2.2r] Add setSmscAddress web API in MozMobileMessageManager. r=htsai, btseng

Update v2.2r patch.
Attachment #8668862 - Attachment description: Add setSmscAddress web API in MozMobileMessageManager → [v2.2r] Add setSmscAddress web API in MozMobileMessageManager
Comment on attachment 8668862 [details] [diff] [review]
[v2.2r] Add setSmscAddress web API in MozMobileMessageManager. r=htsai, btseng

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa338b7bd692
Attachment #8668862 - Attachment description: [v2.2r] Add setSmscAddress web API in MozMobileMessageManager → [v2.2r] Add setSmscAddress web API in MozMobileMessageManager. r=htsai, btseng
Attachment #8668862 - Flags: review+
Need to phase-in the v2.2r patch.
Keywords: checkin-needed
(In reply to Samael Wang [:freesamael][:sawang] from comment #96)
> Need to phase-in the v2.2r patch.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/15f793b67b1a
Keywords: checkin-needed
Duplicate of this bug: 1207470
You need to log in before you can comment on or make changes to this bug.