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)
Firefox OS Graveyard
RIL
Tracking
(tracking-b2g:backlog)
People
(Reporter: mccr8, Assigned: edgar)
References
Details
(Whiteboard: [p=3])
Attachments
(5 files, 3 obsolete files)
5.86 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
4.63 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
5.85 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
4.43 KB,
patch
|
hsinyi
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
|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 | ||
Updated•10 years ago
|
Assignee: nobody → echen
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Assignee | ||
Comment 1•10 years ago
|
||
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
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8410869 -
Flags: review?(htsai)
Attachment #8410869 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8410875 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8410877 -
Flags: review?(htsai)
Assignee | ||
Updated•10 years ago
|
Attachment #8410885 -
Flags: review?(htsai)
Assignee | ||
Updated•10 years ago
|
Attachment #8410931 -
Flags: review?(htsai)
Updated•10 years ago
|
Attachment #8410869 -
Flags: review?(bugs) → review+
Comment 7•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [p=3]
Target Milestone: --- → 2.0 S1 (9may)
Updated•10 years ago
|
Attachment #8410869 -
Flags: review?(htsai) → review+
Updated•10 years ago
|
Attachment #8410877 -
Flags: review?(htsai) → review+
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
(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 ;)
Assignee | ||
Updated•10 years ago
|
Blocks: b2g-ril-interface
Assignee | ||
Comment 11•10 years ago
|
||
Add test cases for "emMmiErrorInvalidPin".
Attachment #8410931 -
Attachment is obsolete: true
Attachment #8414360 -
Flags: review?(htsai)
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8414401 -
Flags: review?(bugs) → review+
Comment 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
Full try result: https://tbpl.mozilla.org/?tree=Try&rev=ac9c0f94578d
Assignee | ||
Comment 16•10 years ago
|
||
Squash original part1(attachment 8410869 [details] [diff] [review]) and part1-2(attachment 8414401 [details] [diff] [review]).
Attachment #8410869 -
Attachment is obsolete: true
Attachment #8414401 -
Attachment is obsolete: true
Attachment #8414958 -
Flags: review+
Assignee | ||
Comment 17•10 years ago
|
||
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
Updated•10 years ago
|
blocking-b2g: --- → backlog
Comment 18•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•