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

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: edgar, Assigned: edgar)

Tracking

(Blocks: 2 bugs)

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

3 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.
Blocks: 1112484
(Assignee)

Updated

3 years ago
Blocks: 1096815
(Assignee)

Comment 1

3 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

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

Updated

3 years ago
Assignee: nobody → echen
(Assignee)

Updated

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

Updated

3 years ago
Blocks: 959978
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

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

Comment 7

3 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

3 years ago
Duplicate of this bug: 1100244
See Also: → bug 1020757

Comment 9

3 years ago
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

3 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)
(Assignee)

Comment 10

3 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

3 years ago
Assignee: xingming.yin.hz → echen
Mentor: echen@mozilla.com
(Assignee)

Comment 11

3 years ago
Created attachment 8542030 [details] [diff] [review]
Part 1: Interface changes, v1
(Assignee)

Comment 12

3 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

3 years ago
Created attachment 8542033 [details] [diff] [review]
Part 2: DOM changes, v1
(Assignee)

Comment 14

3 years ago
Created attachment 8542037 [details] [diff] [review]
Part 3: RIL changes, v1
(Assignee)

Comment 15

3 years ago
Created attachment 8542038 [details] [diff] [review]
Part 4: Test case, 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+
(Assignee)

Comment 17

3 years ago
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+
(Assignee)

Comment 18

3 years ago
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)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

3 years ago
Created attachment 8543768 [details] [diff] [review]
Part 3: RIL changes, v2
Attachment #8542037 - Attachment is obsolete: true
Attachment #8543768 - Flags: review?(htsai)
(Assignee)

Updated

3 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

3 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

3 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

3 years ago
Created attachment 8544461 [details] [diff] [review]
Part 1: Interface changes, v3, r=hsinyi

Rebase
Attachment #8544461 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8543750 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
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)
(Assignee)

Comment 28

3 years ago
Created attachment 8544464 [details] [diff] [review]
Part 3: RIL changes, v3, r=hsinyi

Rebase
Attachment #8543768 - Attachment is obsolete: true
Attachment #8544464 - Flags: review+
(Assignee)

Comment 29

3 years ago
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+
(Assignee)

Comment 32

3 years ago
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+
(Assignee)

Comment 33

3 years ago
Try result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29f1208ce2c7
(Assignee)

Comment 34

3 years ago
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 36

3 years ago
Thank you Edgar!
status-b2g-v2.2: affected → fixed
(Assignee)

Updated

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