Closed Bug 888593 Opened 7 years ago Closed 6 years ago

Move MozVoicemail to WebIDL

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Ms2ger, Assigned: jaoo)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
I'll go for it.
Assignee: nobody → josea.olivera
Attached patch WIP v1 (obsolete) — Splinter Review
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!
Attachment #788508 - Flags: feedback?(Ms2ger)
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.
Attachment #788508 - Flags: feedback?(Ms2ger) → feedback+
(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;
+  }
:Ms2ger, could you take a look at my comments above (comment #5) please? Thanks!
Flags: needinfo?(Ms2ger)
(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.
Flags: needinfo?(Ms2ger)
Attached patch v1 (obsolete) — Splinter Review
After some tests everything seems ok so I request review here. Guys, could you take a look please? Thanks.
Attachment #788508 - Attachment is obsolete: true
Attachment #790714 - Flags: superreview?(jonas)
Attachment #790714 - Flags: review?(Ms2ger)
No longer blocks: 818352, 905228
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.
Attachment #790714 - Flags: review?(Ms2ger) → review+
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.
Attachment #790714 - Flags: superreview?(jonas)
Attached patch v2. r=Ms2gerSplinter Review
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
Attachment #790714 - Attachment is obsolete: true
(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.
Flags: needinfo?(Ms2ger)
(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>.
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/ed40bb16c4f0
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.