Closed Bug 929701 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RIL, defect)

defect
Not set
normal

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S1 (9may)
tracking-b2g backlog

People

(Reporter: mccr8, Assigned: edgar)

References

Details

(Whiteboard: [p=3])

Attachments

(5 files, 3 obsolete files)

|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.
Assignee: nobody → echen
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Depends on: 883178
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
Blocks: 843452
Attachment #8410869 - Flags: review?(htsai)
Attachment #8410869 - Flags: review?(bugs)
Attachment #8410875 - Flags: review?(bugs)
Attachment #8410877 - Flags: review?(htsai)
Attachment #8410885 - Flags: review?(htsai)
Attachment #8410931 - Flags: review?(htsai)
Attachment #8410869 - Flags: review?(bugs) → review+
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.
Attachment #8410875 - Flags: review?(bugs) → review+
Whiteboard: [p=3]
Target Milestone: --- → 2.0 S1 (9may)
Attachment #8410869 - Flags: review?(htsai) → review+
Attachment #8410877 - Flags: review?(htsai) → review+
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!
Attachment #8410885 - Flags: review?(htsai) → review+
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
Attachment #8410931 - Flags: review?(htsai)
(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 ;)
Add test cases for "emMmiErrorInvalidPin".
Attachment #8410931 - Attachment is obsolete: true
Attachment #8414360 - Flags: review?(htsai)
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 ~
Attachment #8414360 - Flags: review?(htsai) → review+
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
Attachment #8414401 - Flags: review?(htsai)
Attachment #8414401 - Flags: review?(bugs)
Attachment #8414401 - Flags: review?(bugs) → review+
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. :)
Attachment #8414401 - Flags: review?(htsai) → review+
blocking-b2g: --- → backlog
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.