1085307 - [Icc] Deprecate IccCardLockError.lockType

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

IMO, IccCardLockError contains `lockType` seems unnecessary, because callers know which lockType they are operating with when calling {set|unlock}CardLock API. Gecko doesn't have to carry this information back to the caller. And if callers need the lockType in onerror callback, they could just bind it with callback function.

now:

let request = icc.unlockCardLock(lockType);
request.onerror = function(evt) {
  lockType = evt.target.error.lockType;
  ....
};

could just bind lockType with callback function:

let request = icc.unlockCardLock(lockType);
request.onerror = function(lockType, evt) {
  .....
}.bind(this, lockType);

There are some place referencing IccCardLockError.lockType in Gaia now, like
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/simcard_dialog.js#L95-L128. We need to modify them first. Will file a separated bug for Gaia. Thank you.

Comment 1

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 2

[Edgar Chen [:edgar]]

Comment on attachment 8510895 [details] [diff] [review]
Patch, v1

All the IccCardLockError.lockType usage in Gaia was removed in bug 1085313. It is safe to deprecate it. May I have your review, Olli and Hsinyi? Thank you.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8510895 [details] [diff] [review]
Patch, v1

Not sure this makes the API any better (since that .lockType could be handy)
and the example using .bind(this, lockType) certainly looks uglier to me.


In Gaia patch I see
 req.onerror = (function sm_unlockError() {
- this.handleUnlockError(req.error);
+ this.handleUnlockError(options.lockType, req.error.retryCount);
}).bind(this);

That certainly seems like making the API usage worse. One needs to
get the relevant data from two different objects, from options and from req.error.


Why do we want this change? Storing mLockType for a short time in the error
object shouldn't really cause too much memory usage.




I see this a bit similar to event handling.
Event listeners are added for certain event type, so they know which events they
are listening for. Yet event objects have event.type.



Please clarify why we want to get rid of .lockType (and re-ask review).

Comment 4

[Edgar Chen [:edgar]]

Comment on attachment 8510895 [details] [diff] [review]
Patch, v1

In bug 1052825, we would like use enum in webidl for lockType and also define the consts in idl [1]. Since the error event is filed in js code now [2], it seems we have to implement idl consts <-> enum string mapping in js code which is I am trying to avoid if this information is not really necessary to be carried in error event. 

And although we won't have above concern after bug 864489 (the event will be filed in C++, we can have better way to handle it). But we still need putting it in ipdl protocol, request callback and have a way to bind it with DomRequest. I would say I prefer to make implementation easier if possible.

Furthermore, this change makes cardlock API aligned with other mozIcc API's behaviour.
Other mozIcc API (like readContacts()) doesn't carry the caller's input (for example, contactType) back to error/success event, either. 

How do you think? Thank you. :)

[1] A feedback from partner, just like bug 937485
[2] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Edgar Chen [:edgar][:echen] from comment #4) 
> Furthermore, this change makes cardlock API aligned with other mozIcc API's
> behaviour.
> Other mozIcc API (like readContacts()) doesn't carry the caller's input (for
> example, contactType) back to error/success event, either. 

Ok, this sounds pretty convincing.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8510895 [details] [diff] [review]
Patch, v1

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

Considering the implementation details and API usage (talked with gaia developers), removing this unnecessary attribute looks good to me.

Comment 7

[Edgar Chen [:edgar]]

Thanks for the review, Hsinyi and Olli.

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

Comment 8

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/9cae12dcdd4a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cae12dcdd4a