888593 - Move MozVoicemail to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[José Antonio Olivera Ortega [:jaoo]]

I'll go for it.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

Attachment: WIP v1

Comment 3

[José Antonio Olivera Ortega [:jaoo]]

Comment on attachment 788508 [details] [diff] [review]
WIP v1

:Ms2ger, this patch tries to move MozVoicemail to webidl. Note it doesn't move MozVoicemailStatus since the nsIVoicemailProvider interface has an attribute of that type. I guess you knew that by the moment you filed this bug. I request your feedback here before continuing. Could you take a look please? Thanks!

Comment 4

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

Comment on attachment 788508 [details] [diff] [review]
WIP v1

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

Haven't looked too carefully, but this all looks good. Thanks for doing this!

::: dom/base/Navigator.h
@@ +317,5 @@
>    nsRefPtr<power::PowerManager> mPowerManager;
>    nsRefPtr<MobileMessageManager> mMobileMessageManager;
>  #ifdef MOZ_B2G_RIL
>    nsRefPtr<telephony::Telephony> mTelephony;
> +  nsCOMPtr<Voicemail> mVoicemail;

nsRefPtr

::: dom/bindings/Bindings.conf
@@ +743,5 @@
>      'nativeType': 'mozilla::dom::icc::StkCommandEvent',
>      'headerFile': 'StkCommandEvent.h',
>  },
>  
> +'MozVoicemail' : {

No space before the colon. (I see that's wrong elsewhere too; don't worry about those.)

::: dom/voicemail/Voicemail.cpp
@@ +58,5 @@
>    DebugOnly<nsresult> rv = mProvider->RegisterVoicemailMsg(mListener);
>    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>                     "Failed registering voicemail messages with provider");
> +
> +  SetIsDOMBinding();

You can use the nsDOMEventTargetHelper constructor that takes a window; that'll do this for you.

@@ +83,4 @@
>  {
> +  nsCOMPtr<nsIDOMMozVoicemailStatus> status;
> +
> +  nsresult rv = mProvider->GetVoicemailStatus(getter_AddRefs(status));

You lost a null-check for mProvider. Also elsewhere.

::: dom/voicemail/Voicemail.h
@@ +14,3 @@
>  #include "nsIVoicemailProvider.h"
>  
> +struct JSObject;

class

::: dom/webidl/WebIDL.mk
@@ +440,5 @@
>    CallsList.webidl \
>    MozStkCommandEvent.webidl \
>    Telephony.webidl \
>    TelephonyCall.webidl \
> +  MozVoicemail.webidl \

This list is sorted.

Comment 5

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ms2ger from comment #4)
> Comment on attachment 788508 [details] [diff] [review]
> WIP v1
> 
> Review of attachment 788508 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Haven't looked too carefully, but this all looks good. Thanks for doing this!

Thanks for reviewing it! Just a few things below.

> ::: dom/voicemail/Voicemail.cpp
> @@ +58,5 @@
> >    DebugOnly<nsresult> rv = mProvider->RegisterVoicemailMsg(mListener);
> >    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
> >                     "Failed registering voicemail messages with provider");
> > +
> > +  SetIsDOMBinding();
> 
> You can use the nsDOMEventTargetHelper constructor that takes a window;
> that'll do this for you.

Does that mean I could just remove it?

> @@ +83,4 @@
> >  {
> > +  nsCOMPtr<nsIDOMMozVoicemailStatus> status;
> > +
> > +  nsresult rv = mProvider->GetVoicemailStatus(getter_AddRefs(status));
> 
> You lost a null-check for mProvider. Also elsewhere.

Ooops, could the following piece of code be a valid way to replace all NS_ENSURE_STATE deleted macros?

+  if (!mProvider) {
+    aRv.Throw(NS_ERROR_UNEXPECTED);
+    return nullptr;
+  }

Comment 6

[José Antonio Olivera Ortega [:jaoo]]

:Ms2ger, could you take a look at my comments above (comment #5) please? Thanks!

Comment 7

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

(In reply to José Antonio Olivera Ortega [:jaoo] (PTO August 19th till Sept 2nd) from comment #5)
> (In reply to :Ms2ger from comment #4)
> > Comment on attachment 788508 [details] [diff] [review]
> > WIP v1
> > 
> > Review of attachment 788508 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Haven't looked too carefully, but this all looks good. Thanks for doing this!
> 
> Thanks for reviewing it! Just a few things below.
> 
> > ::: dom/voicemail/Voicemail.cpp
> > @@ +58,5 @@
> > >    DebugOnly<nsresult> rv = mProvider->RegisterVoicemailMsg(mListener);
> > >    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
> > >                     "Failed registering voicemail messages with provider");
> > > +
> > > +  SetIsDOMBinding();
> > 
> > You can use the nsDOMEventTargetHelper constructor that takes a window;
> > that'll do this for you.
> 
> Does that mean I could just remove it?

Remove the SetIsDOMBinding and BindToOwner calls, and use...

  Voicemail::Voicemail(nsPIDOMWindow* aWindow,
                       nsIVoicemailProvider* aProvider)
    : nsDOMEventTargetHelper(aWindow)
    , mProvider(aProvider)

> > @@ +83,4 @@
> > >  {
> > > +  nsCOMPtr<nsIDOMMozVoicemailStatus> status;
> > > +
> > > +  nsresult rv = mProvider->GetVoicemailStatus(getter_AddRefs(status));
> > 
> > You lost a null-check for mProvider. Also elsewhere.
> 
> Ooops, could the following piece of code be a valid way to replace all
> NS_ENSURE_STATE deleted macros?
> 
> +  if (!mProvider) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }

Yep.

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

https://tbpl.mozilla.org/?tree=Try&rev=2bf0d4524a3f

Comment 9

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v1

After some tests everything seems ok so I request review here. Guys, could you take a look please? Thanks.

Comment 10

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

Comment on attachment 790714 [details] [diff] [review]
v1

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

lgtm. Please don't forget to address the comments below.

::: dom/base/nsDOMClassInfo.cpp
@@ -1500,5 @@
>    DOM_CLASSINFO_MAP_END
>  
>  #ifdef MOZ_B2G_RIL
>  
> -  DOM_CLASSINFO_MAP_BEGIN(MozVoicemail, nsIDOMMozVoicemail)

Nit: remove the empty newline above this too

::: dom/bindings/Bindings.conf
@@ +1783,5 @@
>                   notflattened=True)
>  addExternalIface('MozTreeColumn', nativeType='nsITreeColumn',
>                   headerFile='nsITreeColumns.h')
> +addExternalIface('MozVoicemailStatus', nativeType='nsIDOMMozVoicemailStatus',
> +                 headerFile='nsIDOMMozVoicemailStatus.h')

I think you can do without passing the nativeType and headerFile arguments. Please drop them if that works.

::: dom/voicemail/Voicemail.cpp
@@ +4,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "Voicemail.h"
> +#include "mozilla/dom/MozVoicemailBinding.h"

Nit: add an empty line after Voicemail.h

@@ +41,5 @@
>  };
>  
>  NS_IMPL_ISUPPORTS1(Voicemail::Listener, nsIVoicemailListener)
>  
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Voicemail)

I don't think this is required; you're not adding anything to the CC. If that's the case, please remove the QI/AddRef/Release implementations and the NS_DECL_ISUPPORTS_INHERITED macro.

@@ +80,2 @@
>  {
> +  nsCOMPtr<nsIDOMMozVoicemailStatus> status;

Nit: declare right before the call where you need it

@@ +84,5 @@
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  nsresult rv = mProvider->GetVoicemailStatus(getter_AddRefs(status));

(Would be nice if nsIVoicemailProvider had a nicer interface. Or is that implemented in JS?)

::: dom/voicemail/Voicemail.h
@@ +14,3 @@
>  #include "nsIVoicemailProvider.h"
>  
> +struct JSObject;

class. (I said this before.)

::: dom/voicemail/nsIDOMMozVoicemail.idl
@@ -1,1 @@
> -/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */

Make this a hg mv, if that ends up with a reasonable diff.

::: dom/webidl/MozVoicemailEvent.webidl
@@ -2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
> -interface MozVoicemailStatus;

Drop this change.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 790714 [details] [diff] [review]
v1

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

If all you're doing is migrating the existing API to WebIDL then you don't need sr on this.

Comment 12

[José Antonio Olivera Ortega [:jaoo]]

Attachment: v2. r=Ms2ger

Review comments (the ones from comment #10) addressed.

Everything seems to work right and test are passing. See https://tbpl.mozilla.org/?tree=Try&rev=c362c11b47ca

Comment 13

[José Antonio Olivera Ortega [:jaoo]]

(In reply to :Ms2ger from comment #10)
> Review of attachment 790714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm. Please don't forget to address the comments below.

Thanks for the review, just a couple of comment bellow. I'll land this asap.

> ::: dom/bindings/Bindings.conf
> @@ +1783,5 @@
> >                   notflattened=True)
> >  addExternalIface('MozTreeColumn', nativeType='nsITreeColumn',
> >                   headerFile='nsITreeColumns.h')
> > +addExternalIface('MozVoicemailStatus', nativeType='nsIDOMMozVoicemailStatus',
> > +                 headerFile='nsIDOMMozVoicemailStatus.h')
> 
> I think you can do without passing the nativeType and headerFile arguments.
> Please drop them if that works.

That worked, so I dropped them.

> ::: dom/voicemail/Voicemail.cpp
> @@ +41,5 @@
> >  };
> >  
> >  NS_IMPL_ISUPPORTS1(Voicemail::Listener, nsIVoicemailListener)
> >  
> > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Voicemail)
> 
> I don't think this is required; you're not adding anything to the CC. If
> that's the case, please remove the QI/AddRef/Release implementations and the
> NS_DECL_ISUPPORTS_INHERITED macro.

Well I still have some doubts to fully understand CC stuff. I've read/learnt that, in practice, most DOM objects need to be cycle collected. Here we are deleting all the macros that implement QueryInterface, AddRef, and Release and not adding anything to the CC. How is that possible? I mean, navigator.voicemail is a DOM object and it's supposed to be cycle-collected, right? I have also read that determining when a class needs to get part in cycle collection involves some engineering judgement, so could you please shed some light here? Thanks.

> @@ +84,5 @@
> > +    aRv.Throw(NS_ERROR_UNEXPECTED);
> > +    return nullptr;
> > +  }
> > +
> > +  nsresult rv = mProvider->GetVoicemailStatus(getter_AddRefs(status));
> 
> (Would be nice if nsIVoicemailProvider had a nicer interface. Or is that
> implemented in JS?)

It is implemented in JS.

Comment 14

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

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #13)
> > ::: dom/voicemail/Voicemail.cpp
> > @@ +41,5 @@
> > >  };
> > >  
> > >  NS_IMPL_ISUPPORTS1(Voicemail::Listener, nsIVoicemailListener)
> > >  
> > > +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(Voicemail)
> > 
> > I don't think this is required; you're not adding anything to the CC. If
> > that's the case, please remove the QI/AddRef/Release implementations and the
> > NS_DECL_ISUPPORTS_INHERITED macro.
> 
> Well I still have some doubts to fully understand CC stuff. I've read/learnt
> that, in practice, most DOM objects need to be cycle collected. Here we are
> deleting all the macros that implement QueryInterface, AddRef, and Release
> and not adding anything to the CC. How is that possible? I mean,
> navigator.voicemail is a DOM object and it's supposed to be cycle-collected,
> right? I have also read that determining when a class needs to get part in
> cycle collection involves some engineering judgement, so could you please
> shed some light here? Thanks.

We cleared this up on IRC <http://krijnhoetmer.nl/irc-logs/developers/20130903#l-1060>.

Comment 15

[José Antonio Olivera Ortega [:jaoo]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed40bb16c4f0