Closed
Bug 994461
Opened 10 years ago
Closed 10 years ago
B2G RIL: Move MozIccInfo to Webidl
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(tracking-b2g:backlog)
People
(Reporter: edgar, Assigned: edgar)
References
Details
(Whiteboard: [priority])
Attachments
(4 files, 9 obsolete files)
2.25 KB,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
echou
:
review+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
39.20 KB,
patch
|
edgar
:
review+
|
Details | Diff | Splinter Review |
Move MozIccInfo/MozGsmIccInfo/MozCdmaIccInfo to Webidl
Comment 1•10 years ago
|
||
(In reply to Edgar Chen [:edgar][:echen] from comment #0) > Move MozIccInfo/MozGsmIccInfo/MozCdmaIccInfo to Webidl Aha, I was about to filing this bug. Thank you :p
Comment 3•10 years ago
|
||
Hsin-Yi, can you find an owner that can take this within the next 2 weeks or so? We'd like to get this done by the end of august.
Flags: needinfo?(htsai)
Comment 4•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #3) > Hsin-Yi, can you find an owner that can take this within the next 2 weeks or > so? We'd like to get this done by the end of august. Again, Bobby, sorry that we need to work on 2.1 features and have no enough bandwidth for it now. I think we need to talk with our PM and prioritize this in coming release.
Flags: needinfo?(htsai)
Comment 5•10 years ago
|
||
See also bug 906398 comment 5.
Updated•10 years ago
|
Whiteboard: [priority]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → echen
Assignee | ||
Updated•10 years ago
|
Blocks: b2g-ril-interface
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
Use `hg mv` for file renaming.
Attachment #8492043 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
I uploaded a wrong file, attach correct one. :(
Attachment #8492045 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8492051 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v2 Changes in this patch: 1). Move Moz*IccInfo to WebIDL and implement them in C++. 2). Rename nsIDOMOZ*IccInfo to nsI*IccInfo given that we don't expose them to web any more, but use for internal interface. May I have your review? thank you.
Attachment #8492051 -
Flags: review?(vyang)
Attachment #8492051 -
Flags: review?(bugs)
Comment 13•10 years ago
|
||
Comment on attachment 8492051 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v2 Review of attachment 8492051 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/Icc.cpp @@ +101,5 @@ > + if (gsmIccInfo) { > + if (mIccInfo.IsNull()) { > + mIccInfo.SetValue().SetAsMozGsmIccInfo() = new GsmIccInfo(GetOwner()); > + } > + mIccInfo.Value().GetAsMozGsmIccInfo().get()->Update(gsmIccInfo); How about keeping a reference to nsIIccInfo in GsmIccInfo/CdmaIccInfo/IccInfo instead? Then we don't have to dup its attributes and |UpdateIccInfo| here means updating the underlying reference only. virtual void IccInfo::Update(nsIIccInfo* aIccInfo) { MOZ_ASSERT(aIccInfo); mInfo = aIccInfo; } virtual void GsmIccInfo::Update(nsIIccInfo* aIccInfo) { IccInfo::Update(aIccInfo); mGsmInfo = do_QueryInterface(aIccInfo); MOZ_ASSERT(mGsmInfo) }
Attachment #8492051 -
Flags: review?(vyang)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8492051 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v2 I would like to address comment #13 first, so cancel the review request. Thank you.
Attachment #8492051 -
Flags: review?(bugs)
Assignee | ||
Comment 15•10 years ago
|
||
Address comment #13.
Attachment #8492051 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8495870 -
Flags: review?(vyang)
Attachment #8495870 -
Flags: review?(bugs)
Comment 17•10 years ago
|
||
Comment on attachment 8495870 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v3 >+class IccInfo : public nsISupports >+ , public nsWrapperCache So we have this class implementing IccInfo > function IccInfo() {} > IccInfo.prototype = { >- QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMMozIccInfo]), >- classID: ICCINFO_CID, >- classInfo: XPCOMUtils.generateCI({ >- classID: ICCINFO_CID, >- classDescription: "MozIccInfo", >- flags: Ci.nsIClassInfo.DOM_OBJECT, >- interfaces: [Ci.nsIDOMMozIccInfo] >- }), >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIIccInfo]), And this JS implementation implements nsIIccInfo. Why do we need them both? Couldn't the C++ implementation just implement nsIIccInfo too. That way internal stuff can use nsIIccInfo if needed, and all the JS stuff would end up using webidl bindings. (Same with other interfaces too)
Attachment #8495870 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 18•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #17) > Comment on attachment 8495870 [details] [diff] [review] > Part 1: Move IccInfo to WebIDL, v3 > > >+class IccInfo : public nsISupports > >+ , public nsWrapperCache > So we have this class implementing IccInfo > > > function IccInfo() {} > > IccInfo.prototype = { > >- QueryInterface: XPCOMUtils.generateQI([Ci.nsIDOMMozIccInfo]), > >- classID: ICCINFO_CID, > >- classInfo: XPCOMUtils.generateCI({ > >- classID: ICCINFO_CID, > >- classDescription: "MozIccInfo", > >- flags: Ci.nsIClassInfo.DOM_OBJECT, > >- interfaces: [Ci.nsIDOMMozIccInfo] > >- }), > >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIIccInfo]), > > And this JS implementation implements nsIIccInfo. > Why do we need them both? Couldn't the C++ implementation just implement > nsIIccInfo too. > That way internal stuff can use nsIIccInfo if needed, and all the JS stuff > would end up using > webidl bindings. > (Same with other interfaces too) The internal xpcom service (JS-implemented) doesn't hold a window, we are unable to create a C++-implemented nsIIccInfo without a window. So we implement nsIIccInfo with JS and wrap it to IccInfo (C++-implemented webidl) when we export it to the web.
Comment 19•10 years ago
|
||
Ah. It is still a bit annoying to have pretty much the same implementation twice, one written in JS, one in C++. Could we perhaps add a [ChromeOnly] IccInfo cloneFor(Window aWindow); to the webidl interfaces and use IccInfo with mWindow pointing to null on chrome side?
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #19) > Ah. It is still a bit annoying to have pretty much the same implementation > twice, one written in JS, one in C++. > Could we perhaps add a > [ChromeOnly] IccInfo cloneFor(Window aWindow); > to the webidl interfaces and use IccInfo with mWindow pointing to null on > chrome side? But how to instantiate the object in JS component?
Comment 21•10 years ago
|
||
createInstance?
Comment 22•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #21) > createInstance? Hi, if I told you this nsIIccInfo is to be removed and all its attributes moved into nsIIcc itself in bug 864489, would that solve this stalemate? Then we'll have an IccService which manages may nsIIcc instances. All the informational attributes in regard to an ICC, as well as available methods, can be found directly on the interface. How do you think? This bug is just a precondition for that and was created to move IccInfo object to WebIDL.
Comment 23•10 years ago
|
||
Comment on attachment 8495870 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v3 Review of attachment 8495870 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/icc/IccInfo.h @@ +22,5 @@ > +public: > + NS_DECL_CYCLE_COLLECTING_ISUPPORTS > + NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(IccInfo) > + > + explicit IccInfo(nsPIDOMWindow* aWindow); We probably need a non-public dtor to pass build process on other platforms because it's ref-counted? @@ +70,5 @@ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(GsmIccInfo, IccInfo) > + > + explicit GsmIccInfo(nsPIDOMWindow* aWindow); ditto. @@ +93,5 @@ > +public: > + NS_DECL_ISUPPORTS_INHERITED > + NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(CdmaIccInfo, IccInfo) > + > + explicit CdmaIccInfo(nsPIDOMWindow* aWindow); ditto. ::: dom/system/gonk/RILContentHelper.js @@ +46,5 @@ > Components.ID("{5467f2eb-e214-43ea-9b89-67711241ec8e}"); > const CELLBROADCASTMESSAGE_CID = > Components.ID("{29474c96-3099-486f-bb4a-3c9a1da834e4}"); > const CELLBROADCASTETWSINFO_CID = > Components.ID("{59f176ee-9dcd-4005-9d47-f6be0cd08e17}"); These have been removed. Need a rebase.
Attachment #8495870 -
Flags: review?(vicamo) → review+
Comment 24•10 years ago
|
||
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #22) > (In reply to Olli Pettay [:smaug] from comment #21) > > createInstance? > > Hi, if I told you this nsIIccInfo is to be removed and all its attributes > moved into nsIIcc itself in bug 864489, would that solve this stalemate? > Then we'll have an IccService which manages may nsIIcc instances. All the > informational attributes in regard to an ICC, as well as available methods, > can be found directly on the interface. How do you think? This bug is just a > precondition for that and was created to move IccInfo object to WebIDL. Ah, that should make this all simpler and we can have the js implementation of nsIIccInfo for now.
Assignee | ||
Comment 25•10 years ago
|
||
- Rebase. - Address comment #23 Hi Olli, may I have your review again? Thank you.
Attachment #8498600 -
Flags: review?(bugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8495870 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
Comment on attachment 8492052 [details] [diff] [review] Part 2: GPS changes, v1 We rename nsIDOMMoz*IccInfo to nsI*IccInfo in part 1 given that it's no longer being exported to the web. This patch is for the corresponding changes in GPS. Hi Kan-Ru, may I have your review? Thank you.
Attachment #8492052 -
Flags: review?(kchen)
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8498602 [details] [diff] [review] Part 3: BT changes, v3 We rename nsIDOMMoz*IccInfo to nsI*IccInfo in part 1 given that it's no longer being exported to the web. This patch is for the corresponding changes in BT. Hi Eric, may I have your review? Thank you.
Attachment #8498602 -
Flags: review?(echou)
Assignee | ||
Comment 29•10 years ago
|
||
Comment on attachment 8492059 [details] [diff] [review] Part 4: MMS changes, v1 Hi Vicamo, may I have your review for the changes in MMS? Thank you.
Attachment #8492059 -
Flags: review?(vicamo)
Comment 30•10 years ago
|
||
Comment on attachment 8498600 [details] [diff] [review] Part 1: Move IccInfo to WebIDL, v4, r=vicamo +GsmIccInfo::GetMsisdn(nsAString &aMsisdn) const Nit, nsAString& aMsisdn >+class IccInfo : public nsISupports >+ , public nsWrapperCache >+{ ... >+protected: >+ nsCOMPtr<nsPIDOMWindow> mWindow; >+ nsCOMPtr<nsIIccInfo> mIccInfo; It is a bit annoying that we have mIccInfo >+class GsmIccInfo MOZ_FINAL : public IccInfo ... >+private: >+ nsCOMPtr<nsIGsmIccInfo> mGsmIccInfo; And then this. >+class CdmaIccInfo MOZ_FINAL : public IccInfo >+private: >+ nsCOMPtr<nsICdmaIccInfo> mCdmaIccInfo; >+}; And this, and the member variables in the inheriting classes point to the same object as in the inherited one. But don't have really better suggestion .
Attachment #8498600 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8498602 -
Flags: review?(echou) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8492059 -
Flags: review?(vicamo) → review?(btseng)
Comment 31•10 years ago
|
||
Comment on attachment 8492059 [details] [diff] [review] Part 4: MMS changes, v1 Review of attachment 8492059 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Thank you!
Attachment #8492059 -
Flags: review?(btseng) → review+
Updated•10 years ago
|
Attachment #8492052 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 32•10 years ago
|
||
Address nit in review comment #30 - s/nsAString &aMsisdn/nsAString& aMsisdn/
Attachment #8498600 -
Attachment is obsolete: true
Attachment #8500223 -
Flags: review+
Assignee | ||
Comment 33•10 years ago
|
||
Just correct reviewer information to r=bevistzeng in comment message.
Attachment #8492059 -
Attachment is obsolete: true
Attachment #8500227 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
Fix the build error in B2G emulator debug build (https://tbpl.mozilla.org/?tree=Try&rev=8aa5d422650f): - include nsIIccInfo.h instead of using forward declaration.
Attachment #8500223 -
Attachment is obsolete: true
Attachment #8500280 -
Flags: review+
Assignee | ||
Comment 35•10 years ago
|
||
Try result: https://tbpl.mozilla.org/?tree=Try&rev=d88e850fac54 https://tbpl.mozilla.org/?tree=Try&rev=6bf77949bc58
Assignee | ||
Comment 36•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/9df045c9adf8 https://hg.mozilla.org/integration/b2g-inbound/rev/e316c5c2060a https://hg.mozilla.org/integration/b2g-inbound/rev/0ba5f61a603b https://hg.mozilla.org/integration/b2g-inbound/rev/09ccd185571b
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9df045c9adf8 https://hg.mozilla.org/mozilla-central/rev/e316c5c2060a https://hg.mozilla.org/mozilla-central/rev/0ba5f61a603b https://hg.mozilla.org/mozilla-central/rev/09ccd185571b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Depends on: 1080059
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
•