Closed Bug 1083745 Opened 10 years ago Closed 10 years ago

[Icc] Remove the redundant information carried in request result of {set|get|unlock}CardLock API.

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S8 (7Nov)

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(4 files, 2 obsolete files)

Now if the request of {set|get|unlock}CardLock is success, |MobileIccCardLockResult|[1] will be dispatched in success callback. But not every attribute is useful. For example,
- We don't need a flag for `success` if we just dispatch it in `success` callback.
- |enabled| is useless for unlockCardLock.

I have quick checked the Gaia, in above two cases, Gaia doesn't use them, either. I will list what information we should really need to expose for each API and also check Gaia again to make sure we won't break anything. 

Thank you.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#77-82
(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> Now if the request of {set|get|unlock}CardLock is success,
> |MobileIccCardLockResult|[1] will be dispatched in success callback. But not
> every attribute is useful. For example,
> - We don't need a flag for `success` if we just dispatch it in `success`
> callback.
> - |enabled| is useless for unlockCardLock.
- |lockType| is also unnecessary to me, callers know which lockType they are operating with. If they need to access lockType in callback, they could just bind it with callback function.

> 
> I have quick checked the Gaia, in above two cases, Gaia doesn't use them,
> either. I will list what information we should really need to expose for
> each API and also check Gaia again to make sure we won't break anything. 
> 
> Thank you.
> 
> [1]
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> RILContentHelper.js#77-82
Blocks: 1052825
(In reply to Edgar Chen [:edgar][:echen] from comment #0)
> I will list what information we should really need to expose for
> each API and also check Gaia again to make sure we won't break anything. 

1. It seems we don't need to carry any additional information in success event for setCardLock and unlockCardLock request [1][2].
2. Only |enabled| is needed for getCardLock [3].
3. And only |retryCount| is needed for getCardLockRetryCount [4].

[1] https://github.com/mozilla-b2g/gaia/blob/64f4a6fa02e16140e005b3ec1195c90a6a698aeb/apps/settings/js/simcard_dialog.js#L212-L221
[2] https://github.com/mozilla-b2g/gaia/blob/64f4a6fa02e16140e005b3ec1195c90a6a698aeb/apps/system/js/simcard_dialog.js#L225-L231
[3] https://github.com/mozilla-b2g/gaia/blob/7afdc83937dfe4a5d9465b48e0aab7277e59b3eb/apps/settings/js/simcard_lock.js#L94-L101
[4] https://github.com/mozilla-b2g/gaia/blob/64f4a6fa02e16140e005b3ec1195c90a6a698aeb/apps/system/js/simcard_dialog.js#L97-L105
Attached patch Patch, v1 (obsolete) — Splinter Review
Comment on attachment 8513271 [details] [diff] [review]
Part 1: Having different IPC message for getting and setting/unlocking request, v1

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

In current code, RILContentHelper use |rilMessageType| to distinguish the request in order to carry different data in error event.
But |rilMessageType| is a message protocol used between ril_worker and RadioInterfaceLayer, it is not proper to use it in RILContentHelper.
To distinguish the request, we can use different IPC message instead.

Hi Hsinyi, may I have your review? Thank you.
Attachment #8513271 - Flags: review?(htsai)
Comment on attachment 8513284 [details] [diff] [review]
Part 2: __exposedProps__ has been deprecated, use Cu.cloneInto() instead, v2

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

__exposedProps__ has been deprecated [1], move them to Cu.cloneInto().
Hi Hinsyi, may I have your review? Thank you.

[1] https://groups.google.com/forum/#!msg/mozilla.dev.platform/yNKS1Z6UYQo/qloD9G0AdkcJ
Attachment #8513284 - Flags: review?(htsai)
Comment on attachment 8513285 [details] [diff] [review]
Part 3: Remove the redundant information carried in request result of icc.{set|get|unlock}CardLock API, v1

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

Remove the redundant information carried in success event, please see the summary in comment #2.
I have tested this in flame and also ran the marionette tests with bug 1084233 and bug 1083650 locally, all of them works good.

Hi Hsinyi, may I have your review? Thank you.
Attachment #8513285 - Flags: review?(htsai)
Attachment #8513271 - Flags: review?(htsai) → review+
Attachment #8513284 - Flags: review?(htsai) → review+
Comment on attachment 8513285 [details] [diff] [review]
Part 3: Remove the redundant information carried in request result of icc.{set|get|unlock}CardLock API, v1

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

I like this clean-up :)
Attachment #8513285 - Flags: review?(htsai) → review+
Reopen given that we need to do corresponding changes in test_icc_card_lock.js as well.
Will provide a patch for this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8516433 [details] [diff] [review]
(follow-up) Part 4: Need to do corresponding changes in test_icc_card_lock.js, v1

Hi Hinsyi, may I have your review? Thank you.
Attachment #8516433 - Flags: review?(htsai)
Attachment #8516433 - Flags: review?(htsai) → review+
https://hg.mozilla.org/mozilla-central/rev/6837b1c1a9f1
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S8 (7Nov)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: