929701 - Don't use an attribute with type |any| in DOMMMIError or don't write DOMMMIError in JS implemented WebIDL

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Andrew McCreight [:mccr8]]

|any| can be a dangerous thing to use in JS-implemented WebIDL, so we're trying to get rid of its use in attributes.  DOMMMIError looks like a very simple class that just uses the |any| field, |additionalInformation| to pass through data from another class, so maybe it could just be converted to C++ or something.

Comment 1

[Edgar Chen [:edgar]]

The |additionalInformation| of DOMMMIError is only used for the retryCount of card lock [1][2], we could use |short| instead. And in this bug I would like to convert DOMMMIError to C++, this can benefit bug 843452.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/ril_worker.js?from=ril_worker.js&case=true#3309-3311
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/mmi.js#L239-L255

Comment 2

[Edgar Chen [:edgar]]

Attachment: Part 1: Don't use an attribute with type |any| in DOMMMIError, v1

Comment 3

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement DOMMMIError by C++, v1

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 3: Remove JS-implemented DOMMMIError, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 4-1: Convert test_mobile_mmi.js to Promise, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 4-2: Add marionette test for changing pin via mmi, v1

Comment 7

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

Comment on attachment 8410875 [details] [diff] [review]
Part 2: Implement DOMMMIError by C++, v1

>+class DOMMMIError MOZ_FINAL : public DOMError
>+{
>+public:
>+  NS_DECL_ISUPPORTS_INHERITED
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(DOMMMIError, DOMError)
You don't need these macros, nor the same stuff in the .cpp since you don't add anything new to cycle collection.
But I guess it helps with possible leak analyzing, so let's keep the macros.

Comment 8

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

Comment on attachment 8410885 [details] [diff] [review]
Part 4-1: Convert test_mobile_mmi.js to Promise, v1

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

Looks good!

Comment 9

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

Comment on attachment 8410931 [details] [diff] [review]
Part 4-2: Add marionette test for changing pin via mmi, v1

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

It looks great, but I am greedy... so, could we have one more case for "emMmiErrorInvalidPin"? :P

Comment 10

[Edgar Chen [:edgar]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #9)
> Comment on attachment 8410931 [details] [diff] [review]
> Part 4-2: Add marionette test for changing pin via mmi, v1
> 
> Review of attachment 8410931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks great, but I am greedy... so, could we have one more case for
> "emMmiErrorInvalidPin"? :P

Sure ;)

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 4-2: Add marionette test for changing pin via mmi, v2

Add test cases for "emMmiErrorInvalidPin".

Comment 12

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

Comment on attachment 8414360 [details] [diff] [review]
Part 4-2: Add marionette test for changing pin via mmi, v2

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

ya, thank you ~

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 1-2: Protect DOMMMIError with MOZ_B2G_RIL, v1

Oops, I found we cannot build pass in non-b2g platform [1].

--
./DOMMMIErrorBinding.cpp:9:10: fatal error: 'mozilla/dom/DOMMMIError.h' file not found
--

It is because the files under dom/mobileconnection/* are protected by 'MOZ_B2G_RIL' [2] and the webidl of MobileConnection has the same configuration [3]. DOMMMIError is only used for MobileConnection, so we should just follow the same rule.

((Will squash with part1 for landed))

[1] https://tbpl.mozilla.org/?tree=Try&rev=59baf5e4d2d9
[2] http://dxr.mozilla.org/mozilla-central/source/dom/moz.build#95-102
[3] http://dxr.mozilla.org/mozilla-central/source/dom/webidl/moz.build#545-555

Comment 14

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

Comment on attachment 8414401 [details] [diff] [review]
Part 1-2: Protect DOMMMIError with MOZ_B2G_RIL, v1

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

It looks good to me though I can't really do the config/build review. :)

Comment 15

[Edgar Chen [:edgar]]

Full try result: https://tbpl.mozilla.org/?tree=Try&rev=ac9c0f94578d

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 1: Don't use an attribute with type |any| in DOMMMIError, v2, r=hsinyi,smaug

Squash original part1(attachment 8410869 [details] [diff] [review]) and part1-2(attachment 8414401 [details] [diff] [review]).

Comment 17

[Edgar Chen [:edgar]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f8d0998c7ec7
https://hg.mozilla.org/integration/b2g-inbound/rev/9bff5184e8b6
https://hg.mozilla.org/integration/b2g-inbound/rev/1c1e3bae34ba
https://hg.mozilla.org/integration/b2g-inbound/rev/e77760fd7332
https://hg.mozilla.org/integration/b2g-inbound/rev/f8e0340f374a

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8d0998c7ec7
https://hg.mozilla.org/mozilla-central/rev/9bff5184e8b6
https://hg.mozilla.org/mozilla-central/rev/1c1e3bae34ba
https://hg.mozilla.org/mozilla-central/rev/e77760fd7332
https://hg.mozilla.org/mozilla-central/rev/f8e0340f374a