+++ 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.

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.

(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.

Thanks for the feedback, Arthur and Hsinyi. I will use `promise` for it.

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.

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. :)

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!

(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.

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.

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]

Address review comment #16, - [NewObject, Throws]

Would you mind reviewing this patch, Olli? Thank you.

Comment on attachment 8543768 [details] [diff] [review] Part 3: RIL changes, v2 Review of attachment 8543768 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!

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?

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();

(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

Address review comment #24, - Add IID for Promise class - Use do_QueryInterface Hi Olli, mind reviewing again? Thank you.

Rebase

Address review comment #21, - Add test for a non-supported service.

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)

Comment on attachment 8544465 [details] [diff] [review] Part 4: Test case, v2 Review of attachment 8544465 [details] [diff] [review]: ----------------------------------------------------------------- Perfect!

Address review comment #30, - move NS_INTERFACE_MAP_ENTRY(Promise) after NS_INTERFACE_MAP_ENTRY(nsISupports)

Try result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29f1208ce2c7

https://hg.mozilla.org/integration/b2g-inbound/rev/d54440baa318 https://hg.mozilla.org/integration/b2g-inbound/rev/a07deec1d272 https://hg.mozilla.org/integration/b2g-inbound/rev/77324fcf60a0 https://hg.mozilla.org/integration/b2g-inbound/rev/f1997f0cf24b

https://hg.mozilla.org/mozilla-central/rev/d54440baa318 https://hg.mozilla.org/mozilla-central/rev/a07deec1d272 https://hg.mozilla.org/mozilla-central/rev/77324fcf60a0 https://hg.mozilla.org/mozilla-central/rev/f1997f0cf24b

Thank you Edgar!