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

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.

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

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.

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.

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.

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.

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

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

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.

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

Add r=echou after r+ by Eric.

Address review comment #12.

BTW, Gaia needs to do corresponding changes, I will file a bug for that.

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

Address comment #23. Thanks, Yoshi.

try server: https://tbpl.mozilla.org/?tree=Try&rev=37182d9d5ab7 https://tbpl.mozilla.org/?tree=Try&rev=e25bcbb5463b

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.

Rebase only.

Rebase only.

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

All green in try server: https://tbpl.mozilla.org/?tree=Try&rev=04b6c33fa189 https://tbpl.mozilla.org/?tree=Try&rev=282e31040c98

https://hg.mozilla.org/integration/b2g-inbound/rev/09057d7a7c7f https://hg.mozilla.org/integration/b2g-inbound/rev/1f391cd3728e https://hg.mozilla.org/integration/b2g-inbound/rev/365bc42758cd https://hg.mozilla.org/integration/b2g-inbound/rev/7e6ef5e5ec52

https://hg.mozilla.org/mozilla-central/rev/09057d7a7c7f https://hg.mozilla.org/mozilla-central/rev/1f391cd3728e https://hg.mozilla.org/mozilla-central/rev/365bc42758cd https://hg.mozilla.org/mozilla-central/rev/7e6ef5e5ec52