Closed Bug 873380 Opened 7 years ago Closed 6 years ago

B2G RIL: Refined the error report policy for card lock functionality

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(4 files, 7 obsolete files)

860 bytes, patch
edgar
: review+
Details | Diff | Splinter Review
13.62 KB, patch
edgar
: review+
Details | Diff | Splinter Review
10.10 KB, patch
edgar
: review+
Details | Diff | Splinter Review
974 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
Current error report policy of card lock is as below:
- Dispatch an error event on DOMRequest with an error message.
- Dispatch an 'icccardlockerror' event with more information, like remaining retires ... etc.

In this way we are dispatching two error event. Maybe we could consider having a customized DOMError which contains more information and dispatch it on DOMRequest.

Please see [1] [2] [3] [4] for more detailed information.

Thanks

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c3
[3] https://bugzilla.mozilla.org/show_bug.cgi?id=783525#c0
[4] https://bugzilla.mozilla.org/show_bug.cgi?id=783525#c4
(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> Current error report policy of card lock is as below:
> - Dispatch an error event on DOMRequest with an error message.
> - Dispatch an 'icccardlockerror' event with more information, like remaining
> retires ... etc.
> 
> In this way we are dispatching two error event. Maybe we could consider
> having a customized DOMError which contains more information and dispatch it
> on DOMRequest.

Agree! 
I think we should discuss the change in nsIDOMDOMRequest API first.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/base/nsIDOMDOMRequest.idl#39

> 
> Please see [1] [2] [3] [4] for more detailed information.
> 
> Thanks
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c2
> [2] https://bugzilla.mozilla.org/show_bug.cgi?id=772357#c3
> [3] https://bugzilla.mozilla.org/show_bug.cgi?id=783525#c0
> [4] https://bugzilla.mozilla.org/show_bug.cgi?id=783525#c4
Depends on: 860585
Depends on: 885701
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Version: Trunk → unspecified
In bug 873380, we add fireDetailedError() in DOMRequest to fire an error with more detailed information. We can use this for card lock related error.
Assignee: nobody → echen
(In reply to Edgar Chen [:edgar][:echen] from comment #2)
> In bug 873380, we add fireDetailedError() in DOMRequest to fire an error
This bug is Bug 873380 O.O
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #3)
> (In reply to Edgar Chen [:edgar][:echen] from comment #2)
> > In bug 873380, we add fireDetailedError() in DOMRequest to fire an error
       ^^^^^^^^^^
Ha, typo, it should be bug 885701. 
> This bug is Bug 873380 O.O
1). Remove icccardlockerror event.
2). Add DOMIccCardLockError which inherit from DOMError.
Changes for RIL implementation and test case.
Because patch part1 touches the interface, so change the component to "DOM: Device Interfaces".
Component: RIL → DOM: Device Interfaces
Product: Boot2Gecko → Core
Comment on attachment 810422 [details] [diff] [review]
Part 1: Refined the error report policy for card lock functionality, v1

Hi smaug, jonas, in this bug, we would like to improve error reporting handler of card lock functionality (Please see comment #0 for detailed information). In this patch, I remove the icccardlockerror event and add DOMIccCardError which is used for dispatching the detailed error through DOMRequest. Thanks.
Attachment #810422 - Flags: superreview?(jonas)
Attachment #810422 - Flags: review?(bugs)
Comment on attachment 810423 [details] [diff] [review]
Part 2: Bluetooth changes for iccCardLockError, v1

Eric, we are going to remove |notifyIccCardLockError()| in IccListener, please help to review the corresponding changes in Bluetooth, thanks.
Attachment #810423 - Flags: review?(echou)
Comment on attachment 810425 [details] [diff] [review]
Part 3: RIL changes for icc card lock error, v1

Yoshi, in this patch, I implement the DOMIccCardError in RILContentHelper and dispatch it via fireDetailedError of DOMRequest. And I also correct the marionette test in this patch, thanks.
Attachment #810425 - Flags: review?(allstars.chh)
Comment on attachment 810425 [details] [diff] [review]
Part 3: RIL changes for icc card lock error, v1

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

::: dom/system/gonk/RILContentHelper.js
@@ +426,5 @@
> +  classDescription: "DOMIccCardLockError",
> +  classID:          DOMICCCARDLOCKERROR_CID,
> +  contractID:       "@mozilla.org/dom/icccardlock-error;1",
> +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsISupports]),
> +  __init: function(name, lockType, retryCount) {

lockType should be the 1st argument,
at least it should come before errorMsg.

And the name here should be errrMsg.
Or you mixed name and message from DOMError?

@@ +1456,5 @@
> +
> +    if (DEBUG) {
> +      debug("fire request detailed error, id: " + requestId +
> +            ", detailedError: " + JSON.stringify(detailedError));
> +    }

Remove this debug

@@ +1520,5 @@
>          break;
>        case "RIL:CardLockResult":
>          if (msg.json.success) {
>            let result = new MobileIccCardLockResult(msg.json);
>            this.fireRequestSuccess(msg.json.requestId, result);

You didn't delete this._windowsMap[requestId] ?

@@ +1523,5 @@
>            let result = new MobileIccCardLockResult(msg.json);
>            this.fireRequestSuccess(msg.json.requestId, result);
>          } else {
>            if (msg.json.rilMessageType == "iccSetCardLock" ||
>                msg.json.rilMessageType == "iccUnlockCardLock") {

Why don't we add error for iccGetCardLock also?

@@ +1525,5 @@
>          } else {
>            if (msg.json.rilMessageType == "iccSetCardLock" ||
>                msg.json.rilMessageType == "iccUnlockCardLock") {
> +            let requestId = msg.json.requestId;
> +            let window = this._windowsMap[requestId];

window is a common global object in content side,
also RILContentHelper doesn't have window in it global scope,
we should use another naming to prevent confusion.
Attachment #810425 - Flags: review?(allstars.chh)
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #12)
> Comment on attachment 810425 [details] [diff] [review]
> Part 3: RIL changes for icc card lock error, v1
> 
> Review of attachment 810425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +426,5 @@
> > +  classDescription: "DOMIccCardLockError",
> > +  classID:          DOMICCCARDLOCKERROR_CID,
> > +  contractID:       "@mozilla.org/dom/icccardlock-error;1",
> > +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsISupports]),
> > +  __init: function(name, lockType, retryCount) {
> 
> lockType should be the 1st argument,
> at least it should come before errorMsg.
> 
> And the name here should be errrMsg.
> Or you mixed name and message from DOMError?
Hmm, it is the name of DOMError.
It seems we also put errorMsg in name of DOMError in other place.

> 
> @@ +1456,5 @@
> > +
> > +    if (DEBUG) {
> > +      debug("fire request detailed error, id: " + requestId +
> > +            ", detailedError: " + JSON.stringify(detailedError));
> > +    }
> 
> Remove this debug
Okay.

> 
> @@ +1520,5 @@
> >          break;
> >        case "RIL:CardLockResult":
> >          if (msg.json.success) {
> >            let result = new MobileIccCardLockResult(msg.json);
> >            this.fireRequestSuccess(msg.json.requestId, result);
> 
> You didn't delete this._windowsMap[requestId] ?
I will address this in next version. Thanks.

> 
> @@ +1523,5 @@
> >            let result = new MobileIccCardLockResult(msg.json);
> >            this.fireRequestSuccess(msg.json.requestId, result);
> >          } else {
> >            if (msg.json.rilMessageType == "iccSetCardLock" ||
> >                msg.json.rilMessageType == "iccUnlockCardLock") {
> 
> Why don't we add error for iccGetCardLock also?
Because the error of iccGetCardLock doesn't have retryCount information, so we don't need to use DOMIccCardError to dispatch it, use DOMError is enough.

> 
> @@ +1525,5 @@
> >          } else {
> >            if (msg.json.rilMessageType == "iccSetCardLock" ||
> >                msg.json.rilMessageType == "iccUnlockCardLock") {
> > +            let requestId = msg.json.requestId;
> > +            let window = this._windowsMap[requestId];
> 
> window is a common global object in content side,
> also RILContentHelper doesn't have window in it global scope,
> we should use another naming to prevent confusion.

Sure, I will address this in next version.
Comment on attachment 810422 [details] [diff] [review]
Part 1: Refined the error report policy for card lock functionality, v1

Either the new interface should have a pref
or DOMIccCardLockError.webidl should be in if CONFIG['MOZ_B2G_RIL']

Also, I don't see reason for DOM prefix in the interface name.
Attachment #810422 - Flags: review?(bugs) → review+
(But I hope jonas comments on the API change)
Comment on attachment 810422 [details] [diff] [review]
Part 1: Refined the error report policy for card lock functionality, v1

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

If this is fine with the RIL team, then this is fine with me.
Attachment #810422 - Flags: superreview?(jonas) → superreview?(htsai)
Attachment #810423 - Flags: review?(echou) → review+
Comment on attachment 810422 [details] [diff] [review]
Part 1: Refined the error report policy for card lock functionality, v1

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

Please rename the interface IccCardLockError.webidl, and make sure it is put in 'if CONFIG['MOZ_B2G_RIL'].'
The rest changes look good to me. Thank you.
Attachment #810422 - Flags: superreview?(htsai) → superreview+
(In reply to Olli Pettay [:smaug] from comment #14)
> Comment on attachment 810422 [details] [diff] [review]
> Part 1: Refined the error report policy for card lock functionality, v1
> 
> Either the new interface should have a pref
> or DOMIccCardLockError.webidl should be in if CONFIG['MOZ_B2G_RIL']
> 
> Also, I don't see reason for DOM prefix in the interface name.

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #17)
> Comment on attachment 810422 [details] [diff] [review]
> Part 1: Refined the error report policy for card lock functionality, v1
> 
> Review of attachment 810422 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please rename the interface IccCardLockError.webidl, and make sure it is put
> in 'if CONFIG['MOZ_B2G_RIL'].'
> The rest changes look good to me. Thank you.

Thank you, smaug, hsinyi. I will address your comments in next version.
1). Address review comment #14 and comment #17.
2). Add r=smaug and sr=hsinyi after r+ by smaug and sr+ by hsinyi.
Attachment #810422 - Attachment is obsolete: true
Attachment #812554 - Flags: superreview+
Attachment #812554 - Flags: review+
Add r=echou after r+ by Eric.
Attachment #810423 - Attachment is obsolete: true
Attachment #812559 - Flags: review+
Address review comment #12.
Attachment #810425 - Attachment is obsolete: true
Attachment #812562 - Flags: review?(allstars.chh)
BTW, Gaia needs to do corresponding changes, I will file a bug for that.
Blocks: 922703
Comment on attachment 812562 [details] [diff] [review]
Part 3: RIL changes for icc card lock error, v2

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

::: dom/system/gonk/RILContentHelper.js
@@ +426,5 @@
> +  classDescription: "IccCardLockError",
> +  classID:          ICCCARDLOCKERROR_CID,
> +  contractID:       "@mozilla.org/dom/icccardlock-error;1",
> +  QueryInterface:   XPCOMUtils.generateQI([Ci.nsISupports]),
> +  __init: function(lockType, errorName, retryCount) {

errorMsg
Attachment #812562 - Flags: review?(allstars.chh) → review+
Address comment #23. Thanks, Yoshi.
Attachment #812562 - Attachment is obsolete: true
Attachment #812968 - Flags: review+
Comment on attachment 812968 [details] [diff] [review]
Part 3: RIL changes for icc card lock error, v3, r=allstars.chh

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

::: dom/system/gonk/RILContentHelper.js
@@ +775,5 @@
>                                    Cr.NS_ERROR_UNEXPECTED);
>      }
>      let request = Services.DOMRequest.createRequest(window);
>      let requestId = this.getRequestId(request);
> +    this._windowsMap[info.requestId] = window;

Oh, I found a typo here. It should be only 'requestId', no 'info'.
Address comment #26.
Attachment #812968 - Attachment is obsolete: true
Attachment #812985 - Flags: review+
Rebase only.
Attachment #812554 - Attachment is obsolete: true
Attachment #815196 - Flags: superreview+
Attachment #815196 - Flags: review+
Rebase only.
Attachment #812985 - Attachment is obsolete: true
Attachment #815198 - Flags: review+
(In reply to Edgar Chen [:edgar][:echen] from comment #25)
> try server:
> https://tbpl.mozilla.org/?tree=Try&rev=37182d9d5ab7

Meet the same fail as bug 897940. 
Apply fix and run try server again: https://tbpl.mozilla.org/?tree=Try&rev=a46710ced91c
(In reply to Edgar Chen [:edgar][:echen] from comment #30)
> (In reply to Edgar Chen [:edgar][:echen] from comment #25)
> > try server:
> > https://tbpl.mozilla.org/?tree=Try&rev=37182d9d5ab7
> 
> Meet the same fail as bug 897940. 
> Apply fix and run try server again:
> https://tbpl.mozilla.org/?tree=Try&rev=a46710ced91c

Hmm, still have one error, I forgot to add a test in test_interfaces.html, will provide a patch later.
Attachment #815244 - Flags: review?(bugs)
Attachment #815244 - Flags: review?(bugs) → review+
Blocks: 1085307
You need to log in before you can comment on or make changes to this bug.