Closed Bug 1112471 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.2 S3 (9jan)
feature-b2g 2.2+
Tracking Status
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- affected
b2g-v2.2 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(4 files, 8 obsolete files)

+++ 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)
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
Attached patch Part 1: Interface changes, v1 (obsolete) — Splinter Review
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)
Attached patch Part 2: DOM changes, v1 (obsolete) — Splinter Review
Attached patch Part 3: RIL changes, v1 (obsolete) — Splinter Review
Attached 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+
Address review comment #16,
- [NewObject, Throws]
Attachment #8542030 - Attachment is obsolete: true
Attachment #8543750 - Flags: review+
Attached 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)
Keywords: checkin-needed
Keywords: checkin-needed
Attached patch Part 3: RIL changes, v2 (obsolete) — Splinter Review
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.
Rebase
Attachment #8544461 - Flags: review+
Attachment #8543750 - Attachment is obsolete: true
Attached 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)
Rebase
Attachment #8543768 - Attachment is obsolete: true
Attachment #8544464 - Flags: review+
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+
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!
Blocks: 1157082
You need to log in before you can comment on or make changes to this bug.