788433 - B2G RIL: DOM request didn't return anything when 'onsuccess'

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Evelyn Hung]

http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMMobileConnection.idl
Now I can't get `result.enabled` in onsuccess callback of getCardLock(), and I can't get `result.success` when calling setCardLock().

Comment 1

[Evelyn Hung]

my last gecko commit: 10db82df0078edb8ca12b034fb8e6b73b6db85da

Comment 2

[Shian-Yow Wu [:swu]]

this should be blocking-basecamp.

Comment 3

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

What I investigated so far...

The code from ril_worker.js to RILContentHelper.js, right before DOMRequest.fireSuccess, looks right. Every parameter is passed correctly. 

I have no idea why app receives nothing in domRequest.result at this moment. Should keep investigating...

Comment 4

[Evelyn Hung]

Hi Hsin-yi,
Thanks. 

I guess the API description in idl isn't align with real returned object, at least on `setCardLock` method, the returned property `result` has been changed to `success`, and the domRequest also changed a couple of weeks ago. [:jaoo] submit a pull request in Gaia for this change (https://github.com/mozilla-b2g/gaia/pull/3507), a relevant bug found along with it (https://bugzilla.mozilla.org/show_bug.cgi?id=783525)

That's all I know. Thanks.

Comment 5

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #1)
> my last gecko commit: 10db82df0078edb8ca12b034fb8e6b73b6db85da

Sorry this commit number is wrong, please ignore this.

Comment 6

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

(In reply to Evelyn Hung [:evelyn] from comment #4)
> Hi Hsin-yi,
> Thanks. 
> 
> I guess the API description in idl isn't align with real returned object, at
> least on `setCardLock` method, the returned property `result` has been
> changed to `success`, and the domRequest also changed a couple of weeks ago.
> [:jaoo] submit a pull request in Gaia for this change
> (https://github.com/mozilla-b2g/gaia/pull/3507), a relevant bug found along
> with it (https://bugzilla.mozilla.org/show_bug.cgi?id=783525)
> 
> That's all I know. Thanks.

I checked the attributes described in .idl and sent from RIL. They are consistent now. However, nothing gained when I try to parse 'request.result' in gaia. There should be another problem.

Per discussion with Vicamo and Yoshi, we might need to create .idl for card lock info. I will try this solution and hope it works. :)

Comment 7

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #6)
> (In reply to Evelyn Hung [:evelyn] from comment #4)
> > Hi Hsin-yi,
> > Thanks. 
> > 
> > I guess the API description in idl isn't align with real returned object, at
> > least on `setCardLock` method, the returned property `result` has been
> > changed to `success`, and the domRequest also changed a couple of weeks ago.
> > [:jaoo] submit a pull request in Gaia for this change
> > (https://github.com/mozilla-b2g/gaia/pull/3507), a relevant bug found along
> > with it (https://bugzilla.mozilla.org/show_bug.cgi?id=783525)
> > 
> > That's all I know. Thanks.
> 
> I checked the attributes described in .idl and sent from RIL. They are
> consistent now. 
Sorry, I am dazzling... What I meant here is the attribute you use in gaia is consistent with the code (not the comment in .idl) ^^" 
> However, nothing gained when I try to parse 'request.result'
> in gaia. There should be another problem.
> 
> Per discussion with Vicamo and Yoshi, we might need to create .idl for card
> lock info. I will try this solution and hope it works. :)

Comment 8

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

Good, I think I found where the problem is and also what the solution. :D 
We need to specify the property "__exposedProps__" of a chrome object if we want to share the object with content. 

Patch ongoing.

Reference: https://developer.mozilla.org/en-US/docs/XPConnect_wrappers#__exposedProps__

Comment 9

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

Attachment: Patch

1) add "__exposedProps__" to share specified attributes with content
2) correct the type of a parameter which is assigned to DOMRequest.fireError()

Comment 10

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #9)
> Created attachment 659185 [details] [diff] [review]
> Patch
> 
> 1) add "__exposedProps__" to share specified attributes with content
> 2) correct the type of a parameter which is assigned to
> DOMRequest.fireError()
Also, I correct the commented examples in .idl.

Comment 11

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

Comment on attachment 659185 [details] [diff] [review]
Patch

Covered what Bug 783504 & Bug 783523 are doing.

Comment 12

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #11)
> Comment on attachment 659185 [details] [diff] [review]
> Patch
> 
> Covered what Bug 783504 & Bug 783523 are doing.
Oops, typo: Bug 783504 & Bug 783525

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

We have the same problem for MobileNetworkInfo at running test case dom/network/tests/marionette/test_mobile_networks.js, but following test cases are immune:

1. dom/network/tests/marionette/test_mobile_iccinfo.js (MobileICCInfo, through observer)
2. dom/network/tests/marionette/test_mobile_voice_state.js (MobileConnectionInfo, MobileCellInfo, through observer)
3. dom/telephony/test/marionette/test_voicemail_number.js ()
4. dom/telephony/test/marionette/test_voicemail_statuschanged.js (VoicemailStatus, through callback)

Comment 14

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

Attachment: Patch: add __exposedProps__

add __exposedProps__ to share attributes with content

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 659200 [details] [diff] [review]
Patch: add __exposedProps__

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

::: dom/system/gonk/RILContentHelper.js
@@ +79,5 @@
>  XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
>                                     "@mozilla.org/uuid-generator;1",
>                                     "nsIUUIDGenerator");
>  
> +function MobileICCCardLockResult(aLockType, aLockEnabled, aRetryCount, aSuccess) {

I'd rather we take an object here than positional arguments.

@@ +87,5 @@
> +  this.success = aSuccess;
> +};
> +MobileICCCardLockResult.prototype = {
> +  __exposedProps__ : {
> +                       lockType: 'r',

Coding style nit: bad indention. Either do

  __exposedProps__ : {lockType: 'r',
                      ...}

or

  __exposedProps__ : {
    lockType: 'r',
    ...
  }

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > +function MobileICCCardLockResult(aLockType, aLockEnabled, aRetryCount, aSuccess) {
> 
> I'd rather we take an object here than positional arguments.

This structure is meant to be passed to content but has no interface definition for now. It sounds strange for me. Is this acceptable?

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Philipp von Weitershausen [:philikon] from comment #15)
> > > +function MobileICCCardLockResult(aLockType, aLockEnabled, aRetryCount, aSuccess) {
> > 
> > I'd rather we take an object here than positional arguments.
> 
> This structure is meant to be passed to content but has no interface
> definition for now. It sounds strange for me. Is this acceptable?

We can pass anything to content so long as it has __exposedProps__.

Comment 18

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

Attachment: Patch (v2): add __exposedProps__ . r=philikon

Comment #15 addressed.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3af6abe746de

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3af6abe746de