906404 - B2G RIL: move MozVoicemailStatus to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 2

[(no longer active)]

Please see <http://mxr.mozilla.org/mozilla-central/search?string=MozVoicemailStatus> for the usages of this interface.

Vicamo, can you please tell Roshan how he can test his patch?  This is his first WebIDL conversion patch and I will help him on it, but I'm not quite sure what level of automated test if any we have on this stuff.  Thanks!

Comment 3

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

Hi Ehsan & Roshan,

Welcome to the RIL world!

We have only test cases for B2G emulator for sure, so the very first thing you'll need is to prepare a working environment for B2G emulator.  ARM/x86, ICS/JB/KK all work for voicemail test cases, but ARM-ICS will be an ideal choice for you now.  See https://developer.mozilla.org/en-US/Firefox_OS/Firefox_OS_build_prerequisites for further steps.

With an emulator, you can simply run Voicemail Marionette test cases by executing following command under B2G top source directory:

  ./mach marionette-webapi `pwd`/gecko/dom/voicemail/test/marionette/manifest.ini

You should have everything passed.

Comment 4

[(no longer active)]

Thanks, Vicamo!

Comment 5

[Roshan Vidyashankar [:rvid]]

Attachment: mozvoicemailstatus.patch

The 
+#include nsMimeTypeArray.h
is to fix an issue caused by unified builds.

I had some issues with marionette-webapi tests initially but its passing the tests right now. This seems to be a known issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1025275

Thanks!

Comment 6

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

Comment on attachment 8449115 [details] [diff] [review]
mozvoicemailstatus.patch

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

Please also re-generate this patch in hg format or at least include patch summary, user info.

::: dom/voicemail/Voicemail.cpp
@@ +13,5 @@
>  #include "mozilla/Services.h"
>  #include "nsDOMClassInfo.h"
>  #include "nsServiceManagerUtils.h"
>  #include "GeneratedEvents.h"
> +#include "mozilla/dom/MozVoicemailStatusBinding.h"

This patch has to be rebased because we have no longer XPIDL based voice events. Please also move this line after |#include "mozilla/dom/MozVoicemailEvent.h"| to keep sorted inclusion order.

@@ +114,5 @@
>    if (NS_FAILED(rv)) {
>      aRv.Throw(rv);
>      return nullptr;
>    }
> +  

nit: trailing white spaces.

::: dom/webidl/MozVoicemailStatus.webidl
@@ +6,2 @@
>  
> +// nsIDOMMozVoicemailStatus

nit: please remove this line. Since we don't have nsIDOMMozVoicemailStatus anymore, I think this may cause some confusion.

@@ +10,1 @@
>  

super-nit: and this empty line.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8449115 [details] [diff] [review]
mozvoicemailstatus.patch

>+Voicemail::NotifyStatusChanged(nsISupports* aStatus)
>+  init.mStatus = static_cast<MozVoicemailStatus*> (aStatus);

Why is this cast safe?  What makes sure that the caller passes in an actual MozVoicemailStatus object?  I see nothing that would ensure that.

The rest looks fine, but this part needs addressing.

Comment 8

[Roshan Vidyashankar [:rvid]]

Attachment: mozvoicemailstatus2.patch

Passing all tests on Try. The patch also needs a review from a DOM peer.

Thanks!

Comment 9

[(no longer active)]

(In reply to comment #8)
> Created attachment 8456485 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=8456485&action=edit
> mozvoicemailstatus2.patch
> 
> Passing all tests on Try. The patch also needs a review from a DOM peer.

Try smaug perhaps?

Comment 10

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

Comment on attachment 8456485 [details] [diff] [review]
mozvoicemailstatus2.patch

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

::: dom/voicemail/Voicemail.cpp
@@ +12,2 @@
>  
> +#include"nsContentUtils.h"

nit: please keep included header files sorted by its path.

@@ +175,5 @@
>  {
>    MozVoicemailEventInit init;
>    init.mBubbles = false;
>    init.mCancelable = false;
> +  if(arg.isObject()){

nit: if (...) {

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

Please use `hg mv <src> <dst>` instead.

::: dom/voicemail/nsIVoicemailProvider.idl
@@ +33,4 @@
>    void registerVoicemailMsg(in nsIVoicemailListener listener);
>    void unregisterVoicemailMsg(in nsIVoicemailListener listener);
>  
> +  jsval getVoicemailStatus(in unsigned long clientId);

Were it possible to return an interface instead of a jsval? Or, we'll have to change it yet again in the future.

::: dom/webidl/MozVoicemailStatus.webidl
@@ +3,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/. */
> +
> +// nsIDOMMozVoicemailStatus

nit: please remove this line. 'nsIDOMMozVoicemailStatus' shouldn't appear anywhere from now on.

Comment 11

[Roshan Vidyashankar [:rvid]]

Vicamo: I don't think I can use a webidl interface in xpidl(nsIVoicemailProvider)?

Comment 12

[Roshan Vidyashankar [:rvid]]

Attachment: mozvoicemailstatus3.patch

Comment 13

[Boris Zbarsky [:bzbarsky]]

The code in VoiceMail::GetStatus seems to assume that GetVoicemailStatus will definitely return an object if it doesn't throw.  It may be better to actually check that it did (and throw if it did not).

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8457836 [details] [diff] [review]
mozvoicemailstatus3.patch

>+++ b/dom/tests/mochitest/general/test_interfaces.html
>@@ -711,16 +711,18 @@ var interfaceNamesInGlobalScope =
>     {name: "MozStkCommandEvent", b2g: true, pref: "dom.icc.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozTimeManager", b2g: true},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozVoicemail", b2g: true, pref: "dom.voicemail.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>     {name: "MozVoicemailEvent", b2g: true, pref: "dom.voicemail.enabled"},
> // IMPORTANT: Do not change this list without review from a DOM peer!
>+    {name: "MozVoicemailStatus", b2g: true, pref: "dom.voicemail.enabled"},
>+// IMPORTANT: Do not change this list without review from a DOM peer!
Hmm, should we use permission, not pref for this interface. Though same question to
MozVoicemail and MozVoicemailEvent
ehsan might know


>-  nsCOMPtr<nsIDOMMozVoicemailStatus> status;
>-  nsresult rv = mProvider->GetVoicemailStatus(id, getter_AddRefs(status));
>+  JSContext *cx = nsContentUtils::GetCurrentJSContext();
>+  JS::Rooted<JS::Value> status(cx);
>+  nsresult rv = mProvider->GetVoicemailStatus(id, &status);
>   if (NS_FAILED(rv)) {
>     aRv.Throw(rv);
>     return nullptr;
>   }
> 
>-  return status.forget();
>+  JS::Rooted<JSObject*> statusObj(cx, &status.toObject());
Nothing guarantees status is object here.

> NS_IMETHODIMP
>-Voicemail::NotifyStatusChanged(nsIDOMMozVoicemailStatus* aStatus)
>+Voicemail::NotifyStatusChanged(JS::HandleValue arg)
Nit, arguments should be in form aName, so aArg or perhaps aStatus

Comment 15

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #14)
> >+++ b/dom/tests/mochitest/general/test_interfaces.html
> >@@ -711,16 +711,18 @@ var interfaceNamesInGlobalScope =
> >     {name: "MozStkCommandEvent", b2g: true, pref: "dom.icc.enabled"},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     {name: "MozTimeManager", b2g: true},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     {name: "MozVoicemail", b2g: true, pref: "dom.voicemail.enabled"},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >     {name: "MozVoicemailEvent", b2g: true, pref: "dom.voicemail.enabled"},
> > // IMPORTANT: Do not change this list without review from a DOM peer!
> >+    {name: "MozVoicemailStatus", b2g: true, pref: "dom.voicemail.enabled"},
> >+// IMPORTANT: Do not change this list without review from a DOM peer!
> Hmm, should we use permission, not pref for this interface. Though same
> question to
> MozVoicemail and MozVoicemailEvent
> ehsan might know

Yeah, I think we should use [CheckPermission] similar to the rest of the RIL WebIDL interfaces.

Comment 16

[Roshan Vidyashankar [:rvid]]

Attachment: mozvoicemailstatus_final.patch

Rebased and fixed small issues

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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