B2G RIL: move MozVoicemailStatus to WebIDL

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: vicamo, Assigned: rvid)

Tracking

unspecified
mozilla33
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 905228
(Reporter)

Updated

5 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Blocks: 981845
Put this bug into backlog.
blocking-b2g: --- → backlog

Comment 2

5 years ago
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!
Assignee: nobody → roshanvid
Flags: needinfo?(vyang)
(Reporter)

Comment 3

5 years ago
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.
Flags: needinfo?(vyang)

Comment 4

5 years ago
Thanks, Vicamo!
(Assignee)

Comment 5

5 years ago
Created attachment 8449115 [details] [diff] [review]
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!
Attachment #8449115 - Flags: review?(vyang)
(Reporter)

Comment 6

5 years ago
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.
Attachment #8449115 - Flags: review?(vyang)
Attachment #8449115 - Flags: review?(bzbarsky)
Attachment #8449115 - Flags: review+
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.
Attachment #8449115 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 8456485 [details] [diff] [review]
mozvoicemailstatus2.patch

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

Thanks!
Attachment #8449115 - Attachment is obsolete: true
Attachment #8456485 - Flags: review?(vyang)

Comment 9

5 years ago
(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?
(Reporter)

Updated

5 years ago
Blocks: 959978
(Reporter)

Comment 10

5 years ago
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.
Attachment #8456485 - Flags: review?(vyang) → review+
(Assignee)

Comment 11

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

Comment 12

5 years ago
Created attachment 8457836 [details] [diff] [review]
mozvoicemailstatus3.patch
Attachment #8456485 - Attachment is obsolete: true
Attachment #8457836 - Flags: review?(bugs)
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 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
Attachment #8457836 - Flags: review?(bugs) → review+

Comment 15

5 years ago
(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.
(Assignee)

Comment 16

5 years ago
Created attachment 8458272 [details] [diff] [review]
mozvoicemailstatus_final.patch

Rebased and fixed small issues
Attachment #8457836 - Attachment is obsolete: true
Attachment #8458272 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e8ad29f1166a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S6 (18july)
Component: RIL → DOM: Device Interfaces
Product: Firefox OS → Core
Target Milestone: 2.0 S6 (18july) → mozilla33

Updated

4 years ago
No longer blocks: 1050665
Depends on: 1050665
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
You need to log in before you can comment on or make changes to this bug.