Closed Bug 1081810 Opened 5 years ago Closed 5 years ago

Implement IccCardLockError in C++

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: edgar, Assigned: edgar)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch Patch, v1 (obsolete) — Splinter Review
Attached patch Patch, v2 (obsolete) — Splinter Review
forgot to `hg qrefresh`, upload correct version includes all changes.
Attachment #8503972 - Attachment is obsolete: true
Assignee: nobody → echen
Comment on attachment 8503978 [details] [diff] [review]
Patch, v2

Move JS-implemented IccCardLockError to C++.

Hi Olli, may I have your review? Thank you.
Attachment #8503978 - Flags: review?(bugs)
Comment on attachment 8503978 [details] [diff] [review]
Patch, v2

ahaa, so we don't currently use the .webidl on desktop because of 
MOZ_B2G_RIL check in moz.build. I was wondering why Pref="dom.icc.enabled".
Looks fine.


Why ChromeConstructor? DOMError has normal Constructor and I don't see
any reason to change Constructor to ChromeConstructor, at least not in this patch. So if you undo that change, r+.


Does anyone else still call DOMError.init ? Perhaps DOMMMI* stuff does still.
(Otherwise we could get rid of that ChromeOnly init)
Attachment #8503978 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8503978 [details] [diff] [review]
> Patch, v2
> 
> ahaa, so we don't currently use the .webidl on desktop because of 
> MOZ_B2G_RIL check in moz.build. I was wondering why Pref="dom.icc.enabled".
> Looks fine.
Yup, it is guarded by MOZ_B2G_RIL right now, but we plan to remove MOZ_B2G_RIL and use preference to control it in bug 947116. And all other Icc related webidls have Pref="dom.icc.enabled" now actually, so add it in this bug.

> 
> Why ChromeConstructor? DOMError has normal Constructor and I don't see
> any reason to change Constructor to ChromeConstructor, at least not in this
> patch. So if you undo that change, r+.
Okay, will undo the change for Constructor.

> 
> 
> Does anyone else still call DOMError.init ? Perhaps DOMMMI* stuff does still.
> (Otherwise we could get rid of that ChromeOnly init)

DOMMMIError was moved to C++ in Bug 929701. We could get rid of DOMError.init.
Will provide a patch for this. Thank you.
Address review comment #5.
- undo the change for Constructor.
Attachment #8503978 - Attachment is obsolete: true
Attachment #8504529 - Flags: review+
Attachment #8504530 - Flags: review?(bugs)
Comment on attachment 8504530 [details] [diff] [review]
Part 2: Get rid of DOMError.init, v1

r+ assuming you've checked there are no callers.
Attachment #8504530 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8504530 [details] [diff] [review]
> Part 2: Get rid of DOMError.init, v1
> 
> r+ assuming you've checked there are no callers.

Yes, I have checked there are no callers. Thank you.
https://hg.mozilla.org/mozilla-central/rev/547119c53310
https://hg.mozilla.org/mozilla-central/rev/511be093541f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
Duplicate of this bug: 947064
You need to log in before you can comment on or make changes to this bug.