994461 - B2G RIL: Move MozIccInfo to Webidl

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Edgar Chen [:edgar]]

Move MozIccInfo/MozGsmIccInfo/MozCdmaIccInfo to Webidl

Comment 1

[Hsin-Yi Tsai [:hsinyi]]

(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 2

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Hsin-Yi Tsai [:hsinyi]]

(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.

Comment 5

[Bobby Holley (:bholley)]

See also bug 906398 comment 5.

Comment 6

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v2

Use `hg mv` for file renaming.

Comment 8

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v2

I uploaded a wrong file, attach correct one. :(

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 2: GPS changes, v1

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 3: BT changes, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 4: MMS changes, v1

Comment 12

[Edgar Chen [:edgar]]

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.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

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)
  }

Comment 14

[Edgar Chen [:edgar]]

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.

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v3

Address comment #13.

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 3: BT changes, v2

Rebase

Comment 17

[Olli Pettay [:smaug]]

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)

Comment 18

[Edgar Chen [:edgar]]

(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

[Olli Pettay [:smaug]]

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?

Comment 20

[Edgar Chen [:edgar]]

(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

[Olli Pettay [:smaug]]

createInstance?

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(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

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 24

[Olli Pettay [:smaug]]

(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.

Comment 25

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v4, r=vicamo

- Rebase.
- Address comment #23

Hi Olli, may I have your review again? Thank you.

Comment 26

[Edgar Chen [:edgar]]

Attachment: Part 3: BT changes, v3

Rebase

Comment 27

[Edgar Chen [:edgar]]

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.

Comment 28

[Edgar Chen [:edgar]]

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.

Comment 29

[Edgar Chen [:edgar]]

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.

Comment 30

[Olli Pettay [:smaug]]

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 .

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

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!

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v5, r=vicamo,smaug

Address nit in review comment #30
- s/nsAString &aMsisdn/nsAString& aMsisdn/

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 4: MMS changes, v1, r=bevistzeng

Just correct reviewer information to r=bevistzeng in comment message.

Comment 34

[Edgar Chen [:edgar]]

Attachment: Part 1: Move IccInfo to WebIDL, v6, r=vicamo,smaug

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.

Comment 35

[Edgar Chen [:edgar]]

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=d88e850fac54
https://tbpl.mozilla.org/?tree=Try&rev=6bf77949bc58

Comment 36

[Edgar Chen [:edgar]]

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

[Ryan VanderMeulen [:RyanVM]]

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