Closed Bug 1085307 Opened 5 years ago Closed 5 years ago

[Icc] Deprecate IccCardLockError.lockType

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(1 file)

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.
Depends on: 1085313
Attached patch Patch, v1Splinter Review
Assignee: nobody → echen
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.
Attachment #8510895 - Flags: review?(htsai)
Attachment #8510895 - Flags: review?(bugs)
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).
Attachment #8510895 - Flags: review?(bugs) → review-
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
Attachment #8510895 - Flags: review- → review?(bugs)
(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.
Attachment #8510895 - Flags: review?(bugs) → review+
Depends on: 873380
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.
Attachment #8510895 - Flags: review?(htsai) → review+
Thanks for the review, Hsinyi and Olli.

Try result: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29bef366dd77
https://hg.mozilla.org/mozilla-central/rev/9cae12dcdd4a
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
You need to log in before you can comment on or make changes to this bug.