Closed Bug 994461 Opened 6 years ago Closed 5 years ago

B2G RIL: Move MozIccInfo to Webidl

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.1 S6 (10oct)
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
Blocks: 981845
(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
Put this bug into backlog.
blocking-b2g: --- → backlog
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)
(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)
Whiteboard: [priority]
Blocks: 999307
Assignee: nobody → echen
Blocks: 864489
Use `hg mv` for file renaming.
Attachment #8492043 - Attachment is obsolete: true
I uploaded a wrong file, attach correct one. :(
Attachment #8492045 - Attachment is obsolete: true
Attached patch Part 3: BT changes, v1 (obsolete) — Splinter Review
Attached patch Part 4: MMS changes, v1 (obsolete) — Splinter Review
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 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)
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)
Address comment #13.
Attachment #8492051 - Attachment is obsolete: true
Attachment #8495870 - Flags: review?(vyang)
Attachment #8495870 - Flags: review?(bugs)
Attached patch Part 3: BT changes, v2 (obsolete) — Splinter Review
Rebase
Attachment #8492056 - Attachment is obsolete: true
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-
(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.
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?
(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?
createInstance?
(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 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+
(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.
- Rebase.
- Address comment #23

Hi Olli, may I have your review again? Thank you.
Attachment #8498600 - Flags: review?(bugs)
Attachment #8495870 - Attachment is obsolete: true
Rebase
Attachment #8495874 - Attachment is obsolete: true
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)
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)
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 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+
Attachment #8498602 - Flags: review?(echou) → review+
Attachment #8492059 - Flags: review?(vicamo) → review?(btseng)
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+
Attachment #8492052 - Flags: review?(kchen) → review+
Address nit in review comment #30
- s/nsAString &aMsisdn/nsAString& aMsisdn/
Attachment #8498600 - Attachment is obsolete: true
Attachment #8500223 - Flags: review+
Just correct reviewer information to r=bevistzeng in comment message.
Attachment #8492059 - Attachment is obsolete: true
Attachment #8500227 - Flags: review+
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+
Depends on: 1087839
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.