865403 - B2G RIL: move MozCellBroadcast to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

Depends on bug 864484 because it will introduce more XPIDL based DOM types.

Comment 2

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

Move only MozCellBroadcast only.

Comment 3

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

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=2175fd410724

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 791784 [details] [diff] [review]
patch

Review of attachment 791784 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 865403 - Move CellBroadcast to WebIDL. Move MozCellBroadcast.

This commit message seems to be a little repetitive.

I don't have time for a careful review until next weekend, but it looks fine on a first look.

::: dom/bindings/Bindings.conf
@@ +752,5 @@
>  }],
>  
> +'MozCellBroadcast' : {
> +    'nativeType': 'mozilla::dom::CellBroadcast',
> +    'headerFile': 'mozilla/dom/CellBroadcast.h',

You can drop the headerFile; it's implied by the nativeType.

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +57,5 @@
> +CellBroadcast::Create(nsPIDOMWindow* aWindow, ErrorResult& aRv)
> +{
> +  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
> +    aWindow :
> +    aWindow->GetCurrentInnerWindow();

I realize you only moved this code, but please assert that it's an inner window.

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +12,3 @@
>  #include "nsICellBroadcastProvider.h"
> +
> +struct JSObject;

class

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
@@ -79,5 @@
>  is(BODY_UCS2_IND.length,  CB_MAX_CONTENT_UCS2 - 1, "BODY_UCS2_IND.length")
>  
>  let cbs = window.navigator.mozCellBroadcast;
> -ok(cbs instanceof MozCellBroadcast,
> -   "mozCellBroadcast is instanceof " + cbs.constructor);

Why remove this test?

::: dom/webidl/Navigator.webidl
@@ +267,3 @@
>  partial interface Navigator {
>    [Throws, Func="Navigator::HasCellBroadcastSupport"]
> +  readonly attribute MozCellBroadcast? mozCellBroadcast;

You're only ever going to return null if you also throw, so this doesn't need the '?'.

Comment 5

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

(In reply to :Ms2ger from comment #4)
> Review of attachment 791784 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
> @@ -79,5 @@
> >  is(BODY_UCS2_IND.length,  CB_MAX_CONTENT_UCS2 - 1, "BODY_UCS2_IND.length")
> >  
> >  let cbs = window.navigator.mozCellBroadcast;
> > -ok(cbs instanceof MozCellBroadcast,
> > -   "mozCellBroadcast is instanceof " + cbs.constructor);
> 
> Why remove this test?

The test fails.  I find no similar test in Telephony's, so I thought that's a side effect of moving to WebIDL.

> ::: dom/webidl/Navigator.webidl
> @@ +267,3 @@
> >  partial interface Navigator {
> >    [Throws, Func="Navigator::HasCellBroadcastSupport"]
> > +  readonly attribute MozCellBroadcast? mozCellBroadcast;
> 
> You're only ever going to return null if you also throw, so this doesn't
> need the '?'.

But Telephony has.  File a bug to remove '?' for telephony?

Comment 6

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

(In reply to :Ms2ger from comment #4)
> Review of attachment 791784 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/cellbroadcast/src/CellBroadcast.cpp
> @@ +57,5 @@
> > +CellBroadcast::Create(nsPIDOMWindow* aWindow, ErrorResult& aRv)
> > +{
> > +  nsPIDOMWindow* innerWindow = aWindow->IsInnerWindow() ?
> > +    aWindow :
> > +    aWindow->GetCurrentInnerWindow();
> 
> I realize you only moved this code, but please assert that it's an inner
> window.

I know little about that inner/outer window assertions.  I found Navigator::HasTelephonySupport calls Telephony::CheckPermission[1] while others call Navigator::CheckPermission instead[2].  Detailed differences are:

                   | Own CheckPermission() | Assert IsInnerWindow() |
  -----------------+-----------------------+------------------------+
  Battery          | yes[3]                | no, check perf "dom.battery.enabled" only
  Power            | yes[4]                | no
  PhoneNumber      | no                    | no
  Idle             | no                    | no
  MobileMessage    | no                    | no
  Camera           | yes[5]                | no
  Telephony        | yes[6]                | yes
  MobileConnection | no                    | no
  CellBroadcast    | no                    | no
  Voicemail        | no                    | no
  Icc              | no                    | no
  Bluetooth        | yes[7]                | no
  Time             | no                    | no
  UserMedia        | no                    | no, check perf "media.navigator.enabled"
                   |                       | or "media.peerconnection.enabled"
  PushNotification | no                    | no, check perf "services.push.enabled" only

As you can see, Telephony is the only one that asserts on IsInnerWindow().  Besides, Power/Camera/Bluetooth define their own CheckPermission() but they're actually exactly the same with Navigator::CheckPermission().

So, are you suggesting that we should 1) assert IsInnerWindow() on all components, 2) have a shared implementation for Telephony and CellBroadcast only, 3) otherwise? 

[1]: http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1666
[2]: http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1684
[3]: http://mxr.mozilla.org/mozilla-central/source/dom/battery/BatteryManager.cpp#139
[4]: http://mxr.mozilla.org/mozilla-central/source/dom/power/PowerManager.cpp#184
[5]: http://mxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraManager.cpp#73
[6]: http://mxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#596
[7]: http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothManager.cpp#178

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't follow; the only caller of CellBroadcast::Create is Navigator, and that always has an inner window.

Comment 8

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

(In reply to :Ms2ger from comment #7)
> I don't follow; the only caller of CellBroadcast::Create is Navigator, and
> that always has an inner window.

The the same will be applied to at least icc/mobileconnection/voicemail, right?  I'll have another update for the patches.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #8)
> (In reply to :Ms2ger from comment #7)
> > I don't follow; the only caller of CellBroadcast::Create is Navigator, and
> > that always has an inner window.
> 
> The the same will be applied to at least icc/mobileconnection/voicemail,
> right?  I'll have another update for the patches.

Quite possible; I don't know that code at all.

Comment 10

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

Attachment: patch : v2

Address comment 4.

Comment 11

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

Attachment: patch : v3

Don't touch Telephony::CheckPermission or Navigator::CheckPermission, just add a MOZ_ASSERT.

Comment 12

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

Telephony follow-up is in bug 908681.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 794696 [details] [diff] [review]
patch : v3

Review of attachment 794696 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 865403 - Move CellBroadcast to WebIDL. Move MozCellBroadcast.

Still need to fix the commit message.

r=me with these changes

::: dom/bindings/Bindings.conf
@@ +751,5 @@
>  {
>      'workers': True,
>  }],
>  
> +'MozCellBroadcast' : {

Please drop the space before the colon

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +45,5 @@
>  /**
>   * CellBroadcast Implementation.
>   */
>  
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(CellBroadcast)

I don't think you're adding anything to the CC, so please remove this, the AddRef/Release implementations and the NS_DECL_ISUPPORTS_INHERITED macro in the header.

@@ +55,5 @@
> +// static
> +already_AddRefed<CellBroadcast>
> +CellBroadcast::Create(nsPIDOMWindow* aWindow, ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(aWindow && aWindow->IsInnerWindow());

Please use two separate assertions, to make it obvious what went wrong in case this fails.

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +42,4 @@
>    CellBroadcast() MOZ_DELETE;
>    CellBroadcast(nsPIDOMWindow *aWindow,
>                  nsICellBroadcastProvider* aProvider);
> +  virtual ~CellBroadcast();

I don't think you need this change

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_etws.js
@@ -23,5 @@
>  SpecialPowers.addPermission("cellbroadcast", true, document);
>  SpecialPowers.addPermission("mobileconnection", true, document);
>  
>  let cbs = window.navigator.mozCellBroadcast;
> -ok(cbs instanceof MozCellBroadcast,

If this doesn't work anymore, please try "window.MozCellBroadcast". If *that* doesn't work, please do file a followup to figure out why.

Comment 14

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

Attachment: patch : v4

Hi Ms2ger,

I've addressed all the previous review comments in this patch except one -- the virtual destructor.  We do have a virtual function:

  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope);

declared in class CellBroadcast.  Without a virtual destructor, compiler may complain and give an error if compiled with flag -Werror,-Wdelete-non-virtual-dtor just like bug 864485 comment 12.  It's true we have been no virtual destructor for class CellBroadcast/Telephony for a long time, and I have idea why that becomes a problem in tbpl.  But having virtual destructor for a class with virtual functions sound absolutely correct for me.  Maybe you'll reconsider that.

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> Created attachment 795333 [details] [diff] [review]
> patch : v4
> 
> Hi Ms2ger,
> 
> I've addressed all the previous review comments in this patch except one --
> the virtual destructor.  We do have a virtual function:
> 
>   virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope);
> 
> declared in class CellBroadcast.  Without a virtual destructor, compiler may
> complain and give an error if compiled with flag
> -Werror,-Wdelete-non-virtual-dtor just like bug 864485 comment 12.  It's
> true we have been no virtual destructor for class CellBroadcast/Telephony
> for a long time, and I have idea why that becomes a problem in tbpl.  But
> having virtual destructor for a class with virtual functions sound
> absolutely correct for me.  Maybe you'll reconsider that.

You're very much right in general, but CellBroadcast is marked as MOZ_FINAL; that means there's no actual danger, and that does suppress the warning as well.

Comment 16

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

(In reply to :Ms2ger from comment #15)
> You're very much right in general, but CellBroadcast is marked as MOZ_FINAL;
> that means there's no actual danger, and that does suppress the warning as
> well.

Thank you.  Will land with "virtual" removed.

Comment 17

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

Attachment: patch : v5

update landing patch. r=Ms2ger

Comment 18

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

https://hg.mozilla.org/integration/b2g-inbound/rev/77bd0d897a2a

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/77bd0d897a2a