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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

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

Comment 1

[Edgar Chen [:edgar]]

(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

Comment 2

[Edgar Chen [:edgar]]

(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

Comment 3

[Edgar Chen [:edgar]]

Attachment: Patch, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 1: Having different IPC message for getting and setting/unlocking request, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 2: __exposedProps__ has been deprecated, use Cu.cloneInto() instead, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 2: __exposedProps__ has been deprecated, use Cu.cloneInto() instead, v2

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 3: Remove the redundant information carried in request result of icc.{set|get|unlock}CardLock API, v1

Comment 8

[Edgar Chen [:edgar]]

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.

Comment 9

[Edgar Chen [:edgar]]

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

Comment 10

[Edgar Chen [:edgar]]

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.

Comment 11

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

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 :)

Comment 12

[Edgar Chen [:edgar]]

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

Comment 13

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a66e234e462a
https://hg.mozilla.org/integration/b2g-inbound/rev/16254a653e65
https://hg.mozilla.org/integration/b2g-inbound/rev/fb852e92beb1

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a66e234e462a
https://hg.mozilla.org/mozilla-central/rev/16254a653e65
https://hg.mozilla.org/mozilla-central/rev/fb852e92beb1

Comment 15

[Edgar Chen [:edgar]]

Reopen given that we need to do corresponding changes in test_icc_card_lock.js as well.
Will provide a patch for this.

Comment 16

[Edgar Chen [:edgar]]

Attachment: (follow-up) Part 4: Need to do corresponding changes in test_icc_card_lock.js, v1

Comment 17

[Edgar Chen [:edgar]]

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.

Comment 18

[Edgar Chen [:edgar]]

Try result for part 4: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e1a597394db8

Comment 19

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/6837b1c1a9f1

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6837b1c1a9f1