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

RESOLVED FIXED in 2.2 S3 (9jan)

Status

RESOLVED FIXED
4 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)

3.70 KB, patch
edgar
: review+
Details | Diff | Splinter Review
6.99 KB, patch
edgar
: review+
Details | Diff | Splinter Review
1.68 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
5.96 KB, patch
edgar
: review+
Details | Diff | Splinter Review
+++ 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.
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)
Thanks for the feedback, Arthur and Hsinyi. I will use `promise` for it.
Assignee: nobody → echen
Target Milestone: --- → 2.2 S3 (9jan)
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: echen → nobody
Mentor: echen
Just a reminder. 
It will be good to put contributor's name as assignee if confirmed they are taking it.
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)
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

4 years ago
status-b2g-v2.0: --- → wontfix
status-b2g-v2.0M: --- → wontfix
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
Flags: needinfo?(jocheng)
(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: xingming.yin.hz → echen
Mentor: echen
Created attachment 8542030 [details] [diff] [review]
Part 1: Interface changes, v1
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)
Created attachment 8542033 [details] [diff] [review]
Part 2: DOM changes, v1
Created attachment 8542037 [details] [diff] [review]
Part 3: RIL changes, v1
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+
Created attachment 8543750 [details] [diff] [review]
Part 1: Interface changes, v2, r=hsinyi

Address review comment #16,
- [NewObject, Throws]
Attachment #8542030 - Attachment is obsolete: true
Attachment #8543750 - Flags: review+
Created attachment 8543765 [details] [diff] [review]
Part 2: DOM changes, v2

Would you mind reviewing this patch, Olli? Thank you.
Attachment #8542033 - Attachment is obsolete: true
Attachment #8543765 - Flags: review?(bugs)
Keywords: checkin-needed
Keywords: checkin-needed
Created attachment 8543768 [details] [diff] [review]
Part 3: RIL changes, v2
Attachment #8542037 - Attachment is obsolete: true
Attachment #8543768 - Flags: review?(htsai)
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!
(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-
(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.
Created attachment 8544461 [details] [diff] [review]
Part 1: Interface changes, v3, r=hsinyi

Rebase
Attachment #8544461 - Flags: review+
Attachment #8543750 - Attachment is obsolete: true
Created attachment 8544463 [details] [diff] [review]
Part 2: DOM changes, v3

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)
Created attachment 8544464 [details] [diff] [review]
Part 3: RIL changes, v3, r=hsinyi

Rebase
Attachment #8543768 - Attachment is obsolete: true
Attachment #8544464 - Flags: review+
Created attachment 8544465 [details] [diff] [review]
Part 4: Test case, v2

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+
Created attachment 8545180 [details] [diff] [review]
Part 2: DOM changes, v4, r=smaug

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!
status-b2g-v2.2: affected → fixed
You need to log in before you can comment on or make changes to this bug.