[B2G][RIL] Provide the availability of FDN to Gaia

RESOLVED FIXED in 2.2 S3 (9jan)

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

unspecified
2.2 S3 (9jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(feature-b2g:2.2+, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 affected, b2g-v2.2 fixed)

Details

Attachments

(4 attachments, 8 obsolete attachments)

Assignee

Description

5 years ago
+++ This bug was initially created as a clone of Bug #1096815 +++

[quote]:
The phone should show or hide the FDN settings according to the value of EF 0x6F38.

Gecko needs to provide the availability of FDN to Gaia.
Assignee

Updated

5 years ago
Blocks: 1096815
Assignee

Comment 1

5 years ago
According to TS 31.102 clause 4.2.24, EF_FDN is presented only if service n° 2[1] and/or service n° 89 [2] is available. We could provide the service information [3] to Gaia and Gaia can get FDN availability from it. Also it could be extended if Gaia needs to know more service status, e.g. 'SDN' .. etc.

Proposed API:

enum IccService {
 "fdn"
};

partial interface MozIcc {
  /**
   * Retrieve the the availability of an service.
   *
   * @param service
   *        Identifies the service type.
   *
   * @return a DOMRequest.
   *         The request's result will be a boolean indicating the availability
   *         of the service.
 DOMRequest getServiceState(IccService service);
};

Hi Hsinyi, Arthur, may I have your opinion on this? Thank you

[1] Fixed Dialling Numbers (FDN) service, see TS 31.102 clause 4.2.8
[2] eCall Data service, see TS 31.102 clause 4.2.8
[3] See TS 31.102 clause 4.2.8, EF UST (USIM Service Table) for USIM.
    And TS 51.011 clause 10.3.7, EF SST (SIM service table) for SIM.
Flags: needinfo?(htsai)
Flags: needinfo?(arthur.chen)
The interface looks good to me. Is it possible returning a promise as newly added APIs seem to do so. It's just a question and I don't have strong opinion on it.
Flags: needinfo?(arthur.chen)
(In reply to Edgar Chen [:edgar][:echen] from comment #1)
> According to TS 31.102 clause 4.2.24, EF_FDN is presented only if service n°
> 2[1] and/or service n° 89 [2] is available. We could provide the service
> information [3] to Gaia and Gaia can get FDN availability from it. Also it
> could be extended if Gaia needs to know more service status, e.g. 'SDN' ..
> etc.
> 
> Proposed API:
> 
> enum IccService {
>  "fdn"
> };
> 
> partial interface MozIcc {
>   /**
>    * Retrieve the the availability of an service.
>    *
>    * @param service
>    *        Identifies the service type.
>    *
>    * @return a DOMRequest.
>    *         The request's result will be a boolean indicating the
> availability
>    *         of the service.
>  DOMRequest getServiceState(IccService service);
> };
> 
> Hi Hsinyi, Arthur, may I have your opinion on this? Thank you
> 
> [1] Fixed Dialling Numbers (FDN) service, see TS 31.102 clause 4.2.8
> [2] eCall Data service, see TS 31.102 clause 4.2.8
> [3] See TS 31.102 clause 4.2.8, EF UST (USIM Service Table) for USIM.
>     And TS 51.011 clause 10.3.7, EF SST (SIM service table) for SIM.

As Arthur pointed out, we should turn to use "promise" for a new API. 
Other than that, this proposal looks good to me, too.
Flags: needinfo?(htsai)
Assignee

Comment 4

5 years ago
Thanks for the feedback, Arthur and Hsinyi. I will use `promise` for it.
Assignee

Updated

5 years ago
Assignee: nobody → echen
Assignee

Updated

5 years ago
Target Milestone: --- → 2.2 S3 (9jan)
Assignee

Updated

5 years ago
Setting feature-b2g:2.2+ as this blocks feature item in new 2.2 scope.
However, from bug 1096815 it seems partner is willing to contribute.
If that's the case, Edgar and telephony platform team can mentor and review.

ni? to Josh for status checking from partner.
feature-b2g: --- → 2.2+
Flags: needinfo?(jocheng)
Assignee

Updated

5 years ago
Assignee: echen → nobody
Mentor: echen
Just a reminder. 
It will be good to put contributor's name as assignee if confirmed they are taking it.
Assignee

Comment 7

5 years ago
Hi XingMing,

I assume you will contribute this, otherwise feel free to kick it back to me

Thank you. :)
Assignee: nobody → xingming.yin.hz
Flags: needinfo?(xingming.yin.hz)
Assignee

Updated

5 years ago
Duplicate of this bug: 1100244
Hi XingMing,
We are sorry that because we have tight schedule to fix this bug (before 1/9). We have to take this bug back. You are very welcome to contribute later for other bugs. Thank you very much!

Hi Edgar,
Do you still have capacity to take this bug back? Thank you!
Flags: needinfo?(xingming.yin.hz)

Updated

5 years ago
Flags: needinfo?(jocheng)
Assignee

Comment 10

5 years ago
(In reply to Josh Cheng [:josh] from comment #9)
> Hi XingMing,
> We are sorry that because we have tight schedule to fix this bug (before
> 1/9). We have to take this bug back. You are very welcome to contribute
> later for other bugs. Thank you very much!
> 
> Hi Edgar,
> Do you still have capacity to take this bug back? Thank you!

Sure, I will take this.
Assignee

Updated

5 years ago
Assignee: xingming.yin.hz → echen
Mentor: echen
Assignee

Comment 11

5 years ago
Posted patch Part 1: Interface changes, v1 (obsolete) — Splinter Review
Assignee

Comment 12

5 years ago
Comment on attachment 8542030 [details] [diff] [review]
Part 1: Interface changes, v1

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

Hi Hsinyi,

In this patch, I follow the proposal in comment #1 to add a new API in MozIcc for getting icc service state.
And I also address your comment about using `Promise` for new API.

May I have your review? Thank you.
Attachment #8542030 - Flags: review?(htsai)
Assignee

Comment 13

5 years ago
Posted patch Part 2: DOM changes, v1 (obsolete) — Splinter Review
Assignee

Comment 14

5 years ago
Posted patch Part 3: RIL changes, v1 (obsolete) — Splinter Review
Assignee

Comment 15

5 years ago
Posted patch Part 4: Test case, v1 (obsolete) — Splinter Review
Comment on attachment 8542030 [details] [diff] [review]
Part 1: Interface changes, v1

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

r=me with comment addressed, thank you edgar!

::: dom/icc/interfaces/nsIIccProvider.idl
@@ +192,5 @@
>                               in nsIDOMWindow window,
>                               in unsigned long mvnoType,
>                               in DOMString mvnoData);
> +
> +  nsISupports getServiceState(in unsigned long clientId,

I am fine with this now. But we all know that in bug 864489 nsIIccProvider.idl will be revised again anyway.

::: dom/webidl/MozIcc.webidl
@@ +346,5 @@
> +   * @return a Promise
> +   *         If succeeds, the promise is resolved with boolean indicating the
> +   *         availability of the service. Otherwise, rejected with a DOMError.
> +   */
> +  [Throws]

This API will return a new promise object every query. So, please have [NewObject, Throws]
Attachment #8542030 - Flags: review?(htsai) → review+
Assignee

Comment 17

5 years ago
Address review comment #16,
- [NewObject, Throws]
Attachment #8542030 - Attachment is obsolete: true
Attachment #8543750 - Flags: review+
Assignee

Comment 18

5 years ago
Posted patch Part 2: DOM changes, v2 (obsolete) — Splinter Review
Would you mind reviewing this patch, Olli? Thank you.
Attachment #8542033 - Attachment is obsolete: true
Attachment #8543765 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 19

5 years ago
Posted patch Part 3: RIL changes, v2 (obsolete) — Splinter Review
Attachment #8542037 - Attachment is obsolete: true
Attachment #8543768 - Flags: review?(htsai)
Assignee

Updated

5 years ago
Attachment #8542038 - Flags: review?(htsai)
Comment on attachment 8543768 [details] [diff] [review]
Part 3: RIL changes, v2

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

Looks good!
Attachment #8543768 - Flags: review?(htsai) → review+
Comment on attachment 8542038 [details] [diff] [review]
Part 4: Test case, v1

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

::: dom/icc/tests/marionette/test_icc_service_state.js
@@ +10,5 @@
> +
> +  // Check fdn service state
> +  return icc.getServiceState("fdn")
> +    .then((aResult) => {
> +      is(aResult, true, "check fdn service state");

Can we also have a test for a non-supported service?
Attachment #8542038 - Flags: review?(htsai)
Hi Anshul,
This is from a partner's request that needs ril interface change. Thank you!
Assignee

Comment 23

5 years ago
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #21)
> Comment on attachment 8542038 [details] [diff] [review]
> Part 4: Test case, v1
> 
> Review of attachment 8542038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/icc/tests/marionette/test_icc_service_state.js
> @@ +10,5 @@
> > +
> > +  // Check fdn service state
> > +  return icc.getServiceState("fdn")
> > +    .then((aResult) => {
> > +      is(aResult, true, "check fdn service state");
> 
> Can we also have a test for a non-supported service?

Will add in next version, thank you.
Comment on attachment 8543765 [details] [diff] [review]
Part 2: DOM changes, v2

>+Icc::GetServiceState(IccService aService, ErrorResult& aRv)
>+{
>+  if (!mProvider) {
>+    aRv.Throw(NS_ERROR_FAILURE);
>+    return nullptr;
>+  }
>+
>+  nsCOMPtr<nsISupports> promise;
>+  nsresult rv = mProvider->GetServiceState(mClientId, GetOwner(),
>+                                           static_cast<uint32_t>(aService),
>+                                           getter_AddRefs(promise));
>+  if (NS_FAILED(rv)) {
>+    aRv.Throw(rv);
>+    return nullptr;
>+  }
>+
>+  return promise.forget().downcast<Promise>();
This is a tad too scary cast. JS could return non-Promise objects. or some kind of wrappers or whatever.
We need some idl interface for promises, or just add IID for Promise class. Maybe that.
So, add something like
#define NS_PROMISE_IID \
{ 0xad013674, 0x5f71, 0x4027, \
  { 0x80, 0xee, 0x77, 0xcb, 0xd6, 0xe5, 0xf3, 0x1d } }
and NS_DECLARE_STATIC_IID_ACCESSOR(NS_PROMISE_IID)
and NS_DEFINE_STATIC_IID_ACCESSOR(Promise, NS_PROMISE_IID)
to Promise.h
(see how those macros are used elsewhere, like in Element.h)
and then add entry for Promise to
NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Promise)
in Promise.cpp

and then
nsCOMPtr<Promise> p = do_QueryInterface(promise);
...
return p.forget();
Attachment #8543765 - Flags: review?(bugs) → review-
Assignee

Comment 25

5 years ago
(In reply to Olli Pettay [:smaug] from comment #24)
> Comment on attachment 8543765 [details] [diff] [review]
> Part 2: DOM changes, v2
> 
> >+Icc::GetServiceState(IccService aService, ErrorResult& aRv)
> >+{
> >+  if (!mProvider) {
> >+    aRv.Throw(NS_ERROR_FAILURE);
> >+    return nullptr;
> >+  }
> >+
> >+  nsCOMPtr<nsISupports> promise;
> >+  nsresult rv = mProvider->GetServiceState(mClientId, GetOwner(),
> >+                                           static_cast<uint32_t>(aService),
> >+                                           getter_AddRefs(promise));
> >+  if (NS_FAILED(rv)) {
> >+    aRv.Throw(rv);
> >+    return nullptr;
> >+  }
> >+
> >+  return promise.forget().downcast<Promise>();
> This is a tad too scary cast. JS could return non-Promise objects. or some
> kind of wrappers or whatever.
> We need some idl interface for promises, or just add IID for Promise class.
> Maybe that.
> So, add something like
> #define NS_PROMISE_IID \
> { 0xad013674, 0x5f71, 0x4027, \
>   { 0x80, 0xee, 0x77, 0xcb, 0xd6, 0xe5, 0xf3, 0x1d } }
> and NS_DECLARE_STATIC_IID_ACCESSOR(NS_PROMISE_IID)
> and NS_DEFINE_STATIC_IID_ACCESSOR(Promise, NS_PROMISE_IID)
> to Promise.h
> (see how those macros are used elsewhere, like in Element.h)
> and then add entry for Promise to
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Promise)
> in Promise.cpp
> 
> and then
> nsCOMPtr<Promise> p = do_QueryInterface(promise);
> ...
> return p.forget();

Will do this, thanks for the review.
Assignee

Comment 26

5 years ago
Rebase
Attachment #8544461 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8543750 - Attachment is obsolete: true
Assignee

Comment 27

5 years ago
Posted patch Part 2: DOM changes, v3 (obsolete) — Splinter Review
Address review comment #24,
- Add IID for Promise class
- Use do_QueryInterface

Hi Olli, mind reviewing again? Thank you.
Attachment #8543765 - Attachment is obsolete: true
Attachment #8544463 - Flags: review?(bugs)
Assignee

Comment 28

5 years ago
Rebase
Attachment #8543768 - Attachment is obsolete: true
Attachment #8544464 - Flags: review+
Assignee

Comment 29

5 years ago
Address review comment #21,
- Add test for a non-supported service.
Attachment #8542038 - Attachment is obsolete: true
Attachment #8544465 - Flags: review?(htsai)
Comment on attachment 8544463 [details] [diff] [review]
Part 2: DOM changes, v3


> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(Promise)
>   NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY
>+  NS_INTERFACE_MAP_ENTRY(Promise)
>   NS_INTERFACE_MAP_ENTRY(nsISupports)
> NS_INTERFACE_MAP_END
Nit, could you move 
NS_INTERFACE_MAP_ENTRY(Promise) after 
NS_INTERFACE_MAP_ENTRY(nsISupports)
Attachment #8544463 - Flags: review?(bugs) → review+
Comment on attachment 8544465 [details] [diff] [review]
Part 4: Test case, v2

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

Perfect!
Attachment #8544465 - Flags: review?(htsai) → review+
Assignee

Comment 32

5 years ago
Address review comment #30,
- move NS_INTERFACE_MAP_ENTRY(Promise) after NS_INTERFACE_MAP_ENTRY(nsISupports)
Attachment #8544463 - Attachment is obsolete: true
Attachment #8545180 - Flags: review+
Thank you Edgar!
Assignee

Updated

4 years ago
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.