B2G RIL: use ipdl as IPC in MozVoicemail

RESOLVED FIXED

Status

defect
--
minor
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog)

Details

(Whiteboard: [p=3])

Attachments

(10 attachments, 29 obsolete attachments)

11.09 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.20 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
14.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
30.04 KB, patch
edgar
: review+
Details | Diff | Splinter Review
7.31 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
23.28 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
11.88 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
9.39 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
13.64 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
21.82 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
No description provided.
Assignee

Updated

7 years ago
Depends on: 833278
Assignee

Updated

7 years ago
Depends on: 834160
Assignee: nobody → htsai
Assignee

Updated

6 years ago
Blocks: 873351
Assignee

Updated

6 years ago
Depends on: 906404
Assignee

Updated

6 years ago
No longer blocks: 873351
I've not been here for a while. Set assignee to default.
Assignee: htsai → nobody
I'll be happy to help here. Hsin-Yi, can I go for it? Thanks!
Flags: needinfo?(htsai)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #2)
> I'll be happy to help here. Hsin-Yi, can I go for it? Thanks!

Sure, go ahead! :D

I have a WIP for this and it works, but I don't get a chance to refine and finish it. I'll be happy to discuss with you if you'd like. :)
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)

> I have a WIP for this and it works, but I don't get a chance to refine and
> finish it. I'll be happy to discuss with you if you'd like. :)

Sure, feel free to attach it and will work on this bug from there.
Assignee: nobody → josea.olivera
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> 
> > I have a WIP for this and it works, but I don't get a chance to refine and
> > finish it. I'll be happy to discuss with you if you'd like. :)
> 
> Sure, feel free to attach it and will work on this bug from there.

Hi José,

This is the WIP branch I worked on several months ago... Hope it still helps :)
https://github.com/hsinyi/releases-mozilla-central/commits/bugzilla/voicemail_ipdl
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> 
> > I have a WIP for this and it works, but I don't get a chance to refine and
> > finish it. I'll be happy to discuss with you if you'd like. :)
> 
> Sure, feel free to attach it and will work on this bug from there.

Hi José,

This is the WIP branch I worked on several months ago... Hope it still helps :)
https://github.com/hsinyi/releases-mozilla-central/commits/bugzilla/voicemail_ipdl
Assignee

Updated

6 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #6)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> > 
> > > I have a WIP for this and it works, but I don't get a chance to refine and
> > > finish it. I'll be happy to discuss with you if you'd like. :)
> > 
> > Sure, feel free to attach it and will work on this bug from there.
> 
> Hi José,
> 
> This is the WIP branch I worked on several months ago... Hope it still helps
> :)
> https://github.com/hsinyi/releases-mozilla-central/commits/bugzilla/
> voicemail_ipdl

Hi José, 

Not sure if you've got time to work on this. If no and if you don't mind, I am glad to continue the previous work and make it completed. :)
Flags: needinfo?(josea.olivera)
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #7)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #6)
> > (In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> > > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #3)
> > > 
> > > > I have a WIP for this and it works, but I don't get a chance to refine and
> > > > finish it. I'll be happy to discuss with you if you'd like. :)
> > > 
> > > Sure, feel free to attach it and will work on this bug from there.
> > 
> > Hi José,
> > 
> > This is the WIP branch I worked on several months ago... Hope it still helps
> > :)
> > https://github.com/hsinyi/releases-mozilla-central/commits/bugzilla/
> > voicemail_ipdl
> 
> Hi José, 
> 
> Not sure if you've got time to work on this. If no and if you don't mind, I
> am glad to continue the previous work and make it completed. :)

Hey, sure. I won't be able to work on this soon so I'll leave the bug. Sorry for not having time to work on it. Glad you take it.
Assignee: josea.olivera → nobody
Flags: needinfo?(josea.olivera)
Put this bug into backlog.
blocking-b2g: --- → backlog

Comment 10

5 years ago
Hey Wesley, is this bug intended for 2.1?
Flags: needinfo?(whuang)
(In reply to Anshul from comment #10)
> Hey Wesley, is this bug intended for 2.1?
No, this bug is not in the scope of 2.1.
Flags: needinfo?(whuang)
Assignee

Comment 12

5 years ago
The patch series is divided into five parts.

The first part, this part, is to rename VoicemailProvider to VoicemailService as other RIL components do.

The 2nd part is to create MozVoicemailStatus objects in mozVoicemail.

The 3rd part is to add IPDL protocol for Voicemail.

The 4th part is to add a VoicemailFactory class that helps instantiating the correct voicemail service according to the parent/child context at the time.

The last part, the 5th part, is to move Voicemail code out of RILContentHelper into a standalone VoicemailService.
Assignee: nobody → vyang
Attachment #8470601 - Flags: review?(htsai)
Assignee

Comment 13

5 years ago
VoicemailStatus will only be instantiated by Voicemail and its reference will be kept in a private array for further access.

The original JS implementation is removed, but the lines referencing it will not be removed until part 5.
Attachment #8470603 - Flags: review?(bugs)
Assignee

Comment 14

5 years ago
In this patch, Voicemail keeps all necessary info for its public interface. In its ctor, it calls nsIVoicemailService::Enumerate to retrieve info for all entities and register its owning listener object to nsIVoicemailService as usual.

The nsIVoicemailListener is now extended to include more notification callbacks in order to update local copies stored in every Voicemail instance.

Access functions originally in nsIVoicemailService is also replaced by an |enumerate()| call. It's supposed to be invoked once for each Voicemail instance's ctor.
Attachment #8470610 - Flags: review?(echen)
Attachment #8470610 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8470603 - Attachment description: part 2.a: implement VoicemailStatus in C++ → part 2.a/5: implement VoicemailStatus in C++
Assignee

Comment 15

5 years ago
Te eliminate the use of MOZ_B2G_RIL, both DOM and IPC parts of a certain sub-component are built by default. Only the back-end in part 5 will be guarded by MOZ_B2G_RIL and MOZ_WIDGET_GONK.
Attachment #8470611 - Flags: review?(khuey)
Attachment #8470611 - Flags: review?(bugs)
Assignee

Comment 16

5 years ago
This patch a new PContent managed PVoicemail protocol is added.

VoicemailChild inherits from PVoicemailChild and nsIVoicemailService (and nsIObserver to receive shutdown notification). No one should ever allocate a VoicemailChild instance manually except the VoicemailFactory in a latter patch.

A VoicemailChild is the voicemail service in the child process. Only when a client makes a request to access 1) default service id, 2) listener registration, 3) info enumeration will a VoicemailChild calls to PVoicemail::SendPVoicemailConstructor in VoicemailChild::EnsureSynced(). Once the IPDL channel is set, it will only be closed on child Gecko shutting down.

VoicemailChild keeps local copies of data of all voicemail entities. So the enumeration only occurs once and latter data sync-up depends on VoicemailChild::RecvNotify{DefaultServiceId,Info,Status}Changed signals.

VoicemailParent inherits from VoicemailParent and nsIVoicemailListener. In PVoicemail::RecvPVoicemailConstructor(), a VoicemailParent registers itself to a voicemail service, which, in Firefox OS, is the Gonk VoicemailService in part 5. VoicemailParent unregisters itself for the service in VoicemailParent::ActorDestroy().

VoicemailParent serves as a messenger for corresponding child process and does not keep any copy of voicemail infos.

The 'voicemail' prefix of some nsIVoicemailService attribute/methods has been removed because we'll no more inherit nsIVoicemailService in RILContentHelper after we have voicemail IPDL protocol.
Attachment #8470618 - Flags: review?(echen)
Attachment #8470618 - Flags: review?(bent.mozilla)
Assignee

Updated

5 years ago
Attachment #8470601 - Flags: review?(htsai) → review?(echen)
Assignee

Comment 17

5 years ago
In child process, we should always create VoicemailChild as nsIVoicemailService. In parent process, the Gonk implementation is available in the next part.
Attachment #8470619 - Flags: review?(khuey)
Assignee

Comment 18

5 years ago
Add nsIGonkVoicemailService.idl, gonk/VoicemailService.js, gonk/VoicemailService.manifest.

Voicemail frame messaging lines in RILContentHelper/RadioInterfaceLayer are either removed or accommodated to the new nsIGonkVoicemailService interface.
Attachment #8470621 - Flags: review?(khuey)
Attachment #8470621 - Flags: review?(echen)
Assignee

Comment 19

5 years ago
Built success on Firefox desktop on Mac, Marionette test passed on FxOS ICS ARM Emulator. Full try: https://tbpl.mozilla.org/?tree=Try&rev=9c9e4850291b

Comment 20

5 years ago
(In reply to Ken Chang[:ken] from comment #11)
> (In reply to Anshul from comment #10)
> > Hey Wesley, is this bug intended for 2.1?
> No, this bug is not in the scope of 2.1.
Ken, want to make sure this is bug still not intended for 2.1 as I see a lot of progress going on?
Flags: needinfo?(kchang)
Comment on attachment 8470603 [details] [diff] [review]
part 2.a/5: implement VoicemailStatus in C++

The .webidl for this interface changed a bit today, so a new patch would be good.
Attachment #8470603 - Flags: review?(bugs)
Comment on attachment 8470603 [details] [diff] [review]
part 2.a/5: implement VoicemailStatus in C++

Or you could actually create a patch on top of this too.
Attachment #8470603 - Flags: review+
Attachment #8470611 - Flags: review?(bugs) → review+
Comment on attachment 8470610 [details] [diff] [review]
part 2.b/5: owning MozVoicemailStatus at client side

> Voicemail::Voicemail(nsPIDOMWindow* aWindow,
>                      nsIVoicemailService* aService)
>   : DOMEventTargetHelper(aWindow)
>-  , mService(aService)
> {
>-  mListener = new Listener(this);
>-  DebugOnly<nsresult> rv = mService->RegisterVoicemailMsg(mListener);
>-  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>-                   "Failed registering voicemail messages with service");
>+  nsRefPtr<Listener> listener = new Listener(this);
>+
>+  nsresult rv = aService->Enumerate(listener);
I don't understand this call. What are we enumerating and why?
If we just want to get some data out of the service, we sure should call the method something else than
enumerate. But maybe I'm missing something here.
Attachment #8470610 - Flags: review?(bugs) → review-
Attachment #8470601 - Flags: review?(echen) → review+
Assignee

Updated

5 years ago
Whiteboard: [p=3]
(In reply to Anshul from comment #20)
> (In reply to Ken Chang[:ken] from comment #11)
> > (In reply to Anshul from comment #10)
> > > Hey Wesley, is this bug intended for 2.1?
> > No, this bug is not in the scope of 2.1.
> Ken, want to make sure this is bug still not intended for 2.1 as I see a lot
> of progress going on?

Hi Anshul,

I can answer this. This is not in the committed scope of 2.1. But as our offline mail thread (the meta bug 815526), this is encouraged to be viewed as high priority from DOM team.
Flags: needinfo?(kchang)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24)
> (In reply to Anshul from comment #20)
> > Ken, want to make sure this is bug still not intended for 2.1 as I see a lot
> > of progress going on?
> 
> Hi Anshul,
> 
> I can answer this. This is not in the committed scope of 2.1. But as our
> offline mail thread (the meta bug 815526), this is encouraged to be viewed
> as high priority from DOM team.

Hi Anshul, some important bugs need more time than one release to work on. When we aren't confident to finish this kind of bugs in one release, we don't commit any fix for these bugs. But we will keep working on this kind of bugs and try to fix it sooner.
Comment on attachment 8470618 [details] [diff] [review]
part 3.b/5: add voicemail IPDL protocol

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

::: dom/voicemail/ipc/VoicemailChild.cpp
@@ +111,5 @@
> +VoicemailChild::RecvNotifyDefaultServiceIdChanged(const uint32_t& aServiceId)
> +{
> +  MOZ_ASSERT(aServiceId < mEntries.Length());
> +
> +  mDefaultServiceId = aServiceId;

I guess you miss mListener[i]->NotifyDefaultServiceIdChanged() here.

@@ +125,5 @@
> +  MOZ_ASSERT(aServiceId < mEntries.Length());
> +
> +  EnumerateResult& entry = mEntries[aServiceId];
> +  entry.number() = aNumber;
> +  entry.displayName() = aDisplayName;

And mListener[i]->NotifyInfoChanged() here.
Attachment #8470618 - Flags: review?(echen)
Attachment #8470610 - Flags: review?(echen) → review+
Comment on attachment 8470621 [details] [diff] [review]
part 5/5: Gonk VoicemailService

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

::: dom/voicemail/gonk/VoicemailService.js
@@ +213,5 @@
> +      case NS_PREFBRANCH_PREFCHANGE_TOPIC_ID:
> +        if (aData === kPrefRilDebuggingEnabled) {
> +          this._updateDebugFlag();
> +        } else if (aData === kPrefDefaultServiceId) {
> +          this.defaultServiceId = this._getDefaultServiceId();

We should notifyDefaultServiceIdChanged if defaultServiceId is changed, no?
Attachment #8470621 - Flags: review?(echen)
Assignee

Updated

5 years ago
Attachment #8470611 - Flags: review?(khuey)
Assignee

Updated

5 years ago
Attachment #8470618 - Flags: review?(bent.mozilla)
Assignee

Updated

5 years ago
Attachment #8470619 - Flags: review?(khuey)
Assignee

Updated

5 years ago
Attachment #8470621 - Flags: review?(khuey)
Assignee

Comment 28

5 years ago
(In reply to Edgar Chen [:edgar][:echen] from comment #27)
> Comment on attachment 8470621 [details] [diff] [review]
> part 5/5: Gonk VoicemailService
> 
> We should notifyDefaultServiceIdChanged if defaultServiceId is changed, no?

Thank you for catching these.

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8470603 [details] [diff] [review]
> part 2.a/5: implement VoicemailStatus in C++
> 
> Or you could actually create a patch on top of this too.

The affected attributes happen to be nsStrings here. So I think there is probably nothing to do.

> Comment on attachment 8470610 [details] [diff] [review]
> part 2.b/5: owning MozVoicemailStatus at client side
> 
> I don't understand this call. What are we enumerating and why?
> If we just want to get some data out of the service, we sure should call the
> method something else than
> enumerate. But maybe I'm missing something here.

Voicemail has DSDS support. That means we have many { number, displayName, status } entries. The original idea is to retrieve all entries in constructor, but this may block app initialization path. I'm trying some more to delay such blocking calls to attribute getters.
Assignee

Updated

5 years ago
Depends on: 1063304
Assignee

Updated

5 years ago
Depends on: 1064231
Assignee

Comment 29

5 years ago
Rebase after bug 1034312, bug 843452.
Attachment #8470601 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8487199 - Flags: review+
Assignee

Comment 30

5 years ago
A trivial patch to place only one argument per line.
Attachment #8487201 - Flags: review?(echen)
Assignee

Updated

5 years ago
Attachment #8487201 - Attachment description: part 2/4: wrap arguments list → part 2.a/4: wrap arguments list
Assignee

Comment 31

5 years ago
Rebase after bug 843452, bug 900551.
Attachment #8470611 - Attachment is obsolete: true
Attachment #8487214 - Flags: review+
Assignee

Updated

5 years ago
Duplicate of this bug: 905228
Assignee

Comment 33

5 years ago
1) The obsoleted nsIVoicemailInfo interface was removed.

2) Let nsIVoicemailService owns several nsIVoicemail instances as MobileConnection (bug 1063304) and ICC (bug 864489) do.

3) The original VoicemailInfo & VoicemailStatus attributes are flattened and they're now attributes of nsIVoicemail.

4) For simplicity, IPDL protocol PVoicemail is still in charge of all traffic for all nsIVoicemail instances.
Assignee

Comment 34

5 years ago
Attachment #8470618 - Attachment is obsolete: true
Assignee

Comment 35

5 years ago
Let VoicemailStatus keep a reference to a nsIVoicemail instance so that we don't bother sync attributes between attributes of service managed nsIVoicemail instances and those belongs to VoicemailStatus.
Attachment #8470603 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8487228 - Attachment description: part 2.a/5: implement VoicemailStatus in C++ : v2 → part 3.c/4: implement VoicemailStatus in C++ : v2
Assignee

Comment 36

5 years ago
Posted patch part 3.d/4: DOM changes (obsolete) — Splinter Review
Attachment #8470610 - Attachment is obsolete: true
Assignee

Comment 37

5 years ago
Attachment #8470621 - Attachment is obsolete: true
Assignee

Comment 39

5 years ago
Posted patch part 4/4: fix test cases (obsolete) — Splinter Review
1) rename pdu_builder.js to head.js to share voicemail PDU builder,

2) rewrite all test cases with Promise and reuse newly added head.js
Attachment #8487201 - Flags: review?(echen) → review+
Assignee

Comment 40

5 years ago
Update commit summary only.
Attachment #8487201 - Attachment is obsolete: true
Attachment #8489918 - Flags: review+
Assignee

Comment 41

5 years ago
Per bug 1063304 comment 17, rename nsIVoicemailService::length to numItems as well.
Attachment #8487224 - Attachment is obsolete: true
Attachment #8489920 - Flags: review?(echen)
Assignee

Comment 42

5 years ago
1) Rebase after bug 994190.

2) Address bug 1063304 comment 17.

May I have your review?
Attachment #8487225 - Attachment is obsolete: true
Attachment #8489925 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8489920 - Flags: review?(bugs)
Assignee

Comment 43

5 years ago
Fix build failure in debug build.
Attachment #8487228 - Attachment is obsolete: true
Attachment #8489927 - Flags: review?(bugs)
Assignee

Comment 44

5 years ago
Posted patch part 3.d/4: DOM changes : v2 (obsolete) — Splinter Review
1) don't include nsDOMClassInfo.h any more.
2) address bug 1063304 comment 17.
Attachment #8487232 - Attachment is obsolete: true
Attachment #8489930 - Flags: review?(bugs)
Assignee

Comment 45

5 years ago
Address bug 1063304 comment 17.
Attachment #8487234 - Attachment is obsolete: true
Attachment #8489982 - Flags: review?(echen)
Assignee

Comment 46

5 years ago
Rebase after bug 1064231.
Attachment #8487236 - Attachment is obsolete: true
Attachment #8489985 - Flags: review?(bugs)
Assignee

Updated

5 years ago
Attachment #8487238 - Flags: review?(echen)
Assignee

Updated

5 years ago
Blocks: 873351
Comment on attachment 8489920 [details] [diff] [review]
part 3.a/4: IPDL and internal XPIDL interface changes : v2

Does GetAttributes need to be sync?
Comment on attachment 8489920 [details] [diff] [review]
part 3.a/4: IPDL and internal XPIDL interface changes : v2

>+  /**
>+   * Whether or not there are messages waiting in the voicemail box. When
>+   * changed, |notifyStatusChanged| of registered nsIVoicemailListener instances
>+   * are called.
>+   *
>+   * Default: false
>+   */
>+  readonly attribute boolean hasMessages;
>+
>+  /**
>+   * When #hasMessages is true, #messageCount should be a positive number for
>+   * the messages waiting, or -1 if the exact number is not available. When
>+   * changed, |notifyStatusChanged| of registered nsIVoicemailListener instances
>+   * are called.
>+   *
>+   * Default: 0
>+   */
>+  readonly attribute long messageCount;
>+
>+  /**
>+   * When #hasMessages is true may contain a non-null string as the phone
>+   * number for the return call. When changed, |notifyStatusChanged| of
>+   * registered nsIVoicemailListener instances are called.
>+   *
>+   * Default: null
>+   */
>+  readonly attribute DOMString returnNumber;
>+
>+  /**
>+   * When #hasMessages is true may contain a non-null string as the notification
>+   * message. When changed, |notifyStatusChanged| of registered
>+   * nsIVoicemailListener instances are called.
>+   *
>+   * Default: null
>+   */
>+  readonly attribute DOMString returnMessage;
So if I just read the interface it is not clear to me what returnNumber and returnMessage are for.
They have something to do with hasMessages, but are they related to the first or last message or neither or what.

So, could you clarify the comments a bit, and perhaps, if needed, rename returnNumber and returnMessage.
Attachment #8489920 - Flags: review?(bugs) → review-
Attachment #8489985 - Flags: review?(bugs) → review+
Comment on attachment 8489927 [details] [diff] [review]
part 3.c/4: implement VoicemailStatus in C++ : v3

>+[Pref="dom.voicemail.enabled"]
> interface MozVoicemailStatus 

So this interface has the same issue as nsIVoicemail.
Needs clarifications.
But assuming those are applied here, r+
Attachment #8489927 - Flags: review?(bugs) → review+
Comment on attachment 8489920 [details] [diff] [review]
part 3.a/4: IPDL and internal XPIDL interface changes : v2

>+interface nsIVoicemail : nsISupports

This is also confusing name given that we have MozVoicemail which does something very different.
Comment on attachment 8489930 [details] [diff] [review]
part 3.d/4: DOM changes : v2

This looks pretty much ok, but I want to look at this after the
clarifications to nsIVoicemail.

Code like
 Voicemail::GetNumber(const Optional<uint32_t>& aServiceId,
                      nsString& aNumber,
                      ErrorResult& aRv) const
 {
   aNumber.SetIsVoid(true);
 
-  if (!mService) {
+  uint32_t unused = 0;
+  nsCOMPtr<nsIVoicemail> item = GetItemByServiceId(aServiceId, unused);
is just a bit too hard to interpret.
What is the relationship between nsIVoicemail and Voicemail objects.
Attachment #8489930 - Flags: review?(bugs)
Comment on attachment 8489925 [details] [diff] [review]
part 3.b/4: add voicemail IPDL protocol : v3

>+class Item MOZ_FINAL : public nsIVoicemail
Ok, I want to review this after the clarifications.
What is nsIVoicemail, and what is VoiceMail in webidl.
Attachment #8489925 - Flags: review?(bugs)
Maybe nsIVoiceMail could have Internal in its name or something.
Assignee

Comment 54

5 years ago
(In reply to Olli Pettay [:smaug] from comment #53)
> Maybe nsIVoiceMail could have Internal in its name or something.

Or how about we rename mozillla::dom::Voicemail to VoicemailManager? VoicemailManager implements WebIDL MozVoicemail, nsIVoicemailService is the internal interface for retrieving API entries in multi-sim world. nsIVoicemailService owns serveral instances of nsIVoicemail, each of which is the internal interface for voicemail function of a specific modem.
mozilla::dom::Voicemail is good name to me since it implements the webidl interface.
Better to change the internal interface names.
Comment on attachment 8489920 [details] [diff] [review]
part 3.a/4: IPDL and internal XPIDL interface changes : v2

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

Looks good, but I would like to review this again after comment #55 addressed.
Thank you.
Attachment #8489920 - Flags: review?(echen)
Comment on attachment 8489982 [details] [diff] [review]
part 3.e/4: Gonk VoicemailService : v2

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

::: dom/voicemail/gonk/VoicemailService.js
@@ +244,5 @@
> +        }
> +        break;
> +
> +      case NS_XPCOM_SHUTDOWN_OBSERVER_ID:
> +        Services.obs.removeObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID);

Please also remove observer for prefs here.

Services.prefs.removeObserver(kPrefRilDebuggingEnabled, this);
Services.prefs.removeObserver(kPrefDefaultServiceId, this);
Attachment #8489982 - Flags: review?(echen)
Comment on attachment 8487238 [details] [diff] [review]
part 4/4: fix test cases

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

Looks nice, thank you. :)

::: dom/voicemail/test/marionette/test_voicemail_statuschanged.js
@@ +25,3 @@
>  
> +  let sender = "+15125551235",
> +      body = "1 new voicemail";

nit: One declaration per line.

let sender = "+...";
let body = "1 new ..";

@@ +38,3 @@
>  
> +  let sender = "+15125551236",
> +      body = aMessageCount + " voicemails";

Ditto.
Attachment #8487238 - Flags: review?(echen) → review+
Assignee

Comment 59

5 years ago
Update commit summary only.
Attachment #8487199 - Attachment is obsolete: true
Attachment #8492704 - Flags: review+
Assignee

Comment 60

5 years ago
Update commit summary only.
Attachment #8489918 - Attachment is obsolete: true
Attachment #8492706 - Flags: review+
Assignee

Comment 61

5 years ago
Update commit summary only.
Attachment #8487214 - Attachment is obsolete: true
Attachment #8492707 - Flags: review+
Assignee

Comment 62

5 years ago
Use 'nsIVoicemailProvider' for each entity and 'nsIVoicemailService' as the manager of them.
Attachment #8489920 - Attachment is obsolete: true
Attachment #8492708 - Flags: review?(echen)
Attachment #8492708 - Flags: review?(bugs)
Assignee

Comment 63

5 years ago
1) Rebase after bug 994190 being backed out.

2) s/item/provider/ except for the name of the two access methods in nsIVoicemailService.
Attachment #8489925 - Attachment is obsolete: true
Attachment #8492709 - Flags: review?(echen)
Attachment #8492709 - Flags: review?(bugs)
Assignee

Comment 64

5 years ago
Accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).
Attachment #8489927 - Attachment is obsolete: true
Attachment #8492710 - Flags: review+
Assignee

Comment 65

5 years ago
Posted patch part 3.d/4: DOM changes : v3 (obsolete) — Splinter Review
Accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).
Attachment #8489930 - Attachment is obsolete: true
Attachment #8492711 - Flags: review?(bugs)
Assignee

Comment 66

5 years ago
Address comment 57.
Attachment #8489982 - Attachment is obsolete: true
Attachment #8492712 - Flags: review?(echen)
Assignee

Comment 67

5 years ago
1) update commit summary,

2) accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).
Attachment #8489985 - Attachment is obsolete: true
Attachment #8492713 - Flags: review+
Assignee

Comment 68

5 years ago
Address comment 58.
Attachment #8487238 - Attachment is obsolete: true
Attachment #8492716 - Flags: review+
Comment on attachment 8492711 [details] [diff] [review]
part 3.d/4: DOM changes : v3

> Voicemail::Voicemail(nsPIDOMWindow* aWindow,
>                      nsIVoicemailService* aService)
>   : DOMEventTargetHelper(aWindow)
>   , mService(aService)
> {
>+  MOZ_ASSERT(mService);
>+
>   mListener = new Listener(this);
>-  DebugOnly<nsresult> rv = mService->RegisterVoicemailMsg(mListener);
>+  DebugOnly<nsresult> rv = mService->RegisterListener(mListener);
>   NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
>                    "Failed registering voicemail messages with service");
>+
>+  uint32_t length = 0;
>+  if (NS_SUCCEEDED(mService->GetNumItems(&length)) && length != 0) {
>+    mStatuses.SetLength(length);
>+  }
I don't understand this. Why do we want to set the length of the array?
Wouldn't it make more sense to set the size when you actually set some data to the array.

>+Voicemail::GetOrCreateStatus(uint32_t aServiceId,
>+                             nsIVoicemailProvider* aProvider)
> {
>-  if (aServiceId.WasPassed()) {
>-    if (!IsValidServiceId(aServiceId.Value())) {
>-      return false;
>-    }
>-    aResult = aServiceId.Value();
>-  } else {
>-    mService->GetVoicemailDefaultServiceId(&aResult);
>+  MOZ_ASSERT(aServiceId <= mStatuses.Length());
>+  MOZ_ASSERT(aProvider);
>+
>+  nsRefPtr<VoicemailStatus> res = mStatuses[aServiceId];
>+  if (!res) {
>+    mStatuses[aServiceId] = res = new VoicemailStatus(GetOwner(), aProvider);
>   }
... so I would ensure the right length here.
Attachment #8492711 - Flags: review?(bugs) → review+
Comment on attachment 8492708 [details] [diff] [review]
part 3.a/4: IPDL and internal XPIDL interface changes : v2

>+
>+  /**
>+   * Voicemail center number. When changed, |notifyInfoChanged| of registered
>+   * nsIVoicemailListener instances are called.
>+   *
>+   * Default: null
>+   */
>+  readonly attribute DOMString number;
Ok, so this is voicemail center number

>+  /**
>+   * When #hasMessages is true may contain a non-null string as the phone
... is true this may contain ...

>+   * number for the return call. When changed, |notifyStatusChanged| of
>+   * registered nsIVoicemailListener instances are called.
>+   *
>+   * Default: null
>+   */
>+  readonly attribute DOMString returnNumber;
But what number is this?


>+  /**
>+   * When #hasMessages is true may contain a non-null string as the notification
... is true this may contain ...


I asked some more clarifications to returnNumber and returnMessage in comment 48.
Could you clarify the comments a bit.
Attachment #8492708 - Flags: review?(bugs) → review+
Comment on attachment 8492709 [details] [diff] [review]
part 3.b/4: add voicemail IPDL protocol : v4

>+VoicemailIPCService::Shutdown()
>+{
>+  if (!mActorDestroyed) {
>+    Send__delete__(this);
>+  }
>+
>+  mListeners.Clear();
>+  mProviders.Clear();
>+}
I don't see any code calling Shutdown()
Please explain what calls it.
Attachment #8492709 - Flags: review?(bugs) → review+
Assignee

Comment 72

5 years ago
(In reply to Olli Pettay [:smaug] from comment #69)
> Comment on attachment 8492711 [details] [diff] [review]
> part 3.d/4: DOM changes : v3
> 
> > Voicemail::Voicemail(nsPIDOMWindow* aWindow,
> >                      nsIVoicemailService* aService)
> >   : DOMEventTargetHelper(aWindow)
> >   , mService(aService)
> > {
> >+  MOZ_ASSERT(mService);
> >+
> >   mListener = new Listener(this);
> >-  DebugOnly<nsresult> rv = mService->RegisterVoicemailMsg(mListener);
> >+  DebugOnly<nsresult> rv = mService->RegisterListener(mListener);
> >   NS_WARN_IF_FALSE(NS_SUCCEEDED(rv),
> >                    "Failed registering voicemail messages with service");
> >+
> >+  uint32_t length = 0;
> >+  if (NS_SUCCEEDED(mService->GetNumItems(&length)) && length != 0) {
> >+    mStatuses.SetLength(length);
> >+  }
> I don't understand this. Why do we want to set the length of the array?
> Wouldn't it make more sense to set the size when you actually set some data
> to the array.

1) that means |mStatuses.Length()| is not the actual total number of entities available,
2) and it follows we need an extra member representing the actual number,
3) user may request VoicemailStatus out of order, that is, mStatuses[3] => mStatues[1] => mStatuses[5] ...
3.1) that follows we have to call |mStatuses.SetLength()| multiple times,
3.2) and don't forget not to set length to a smaller value than it currently is,
3.3) and don't forget to compare to mActualLength instead of |mStatuses.Length()|,

Judging from above complexity, I'd rather simplify the design.

> >+Voicemail::GetOrCreateStatus(uint32_t aServiceId,
> >+                             nsIVoicemailProvider* aProvider)
> > {
> >-  if (aServiceId.WasPassed()) {
> >-    if (!IsValidServiceId(aServiceId.Value())) {
> >-      return false;
> >-    }
> >-    aResult = aServiceId.Value();
> >-  } else {
> >-    mService->GetVoicemailDefaultServiceId(&aResult);
> >+  MOZ_ASSERT(aServiceId <= mStatuses.Length());
> >+  MOZ_ASSERT(aProvider);
> >+
> >+  nsRefPtr<VoicemailStatus> res = mStatuses[aServiceId];
> >+  if (!res) {
> >+    mStatuses[aServiceId] = res = new VoicemailStatus(GetOwner(), aProvider);
> >   }
> ... so I would ensure the right length here.

|mStatuses.Length()| always has the right value. It never changes after being set in Voicemail's constructor.
Ok, fine, set the length in ctor, but please add a comment there, and
change MOZ_ASSERT(aServiceId <= mStatuses.Length()); to
MOZ_ASSERT(aServiceId < mStatuses.Length()); everywhere in the patch.
and make sure the assertion actually holds. (I think it does)
Attachment #8492708 - Flags: review?(echen) → review+
Attachment #8492709 - Flags: review?(echen) → review+
Comment on attachment 8492712 [details] [diff] [review]
part 3.e/4: Gonk VoicemailService : v3

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

Thank you. :)
Attachment #8492712 - Flags: review?(echen) → review+
Assignee

Comment 75

5 years ago
Rebase only.
Attachment #8492707 - Attachment is obsolete: true
Attachment #8495279 - Flags: review+
Assignee

Comment 77

5 years ago
Address comment 69, comment 73. There is only two places that needs

  MOZ_ASSERT(aServiceId < mStatuses.Length());

because all other methods either call |GetItemByServiceId| when requesting for a nsIVoicemailProvider instance or call |GetOrCreateStatus| for a VoicemailStatus.
Attachment #8492711 - Attachment is obsolete: true
Attachment #8495287 - Flags: review+
Assignee

Comment 78

5 years ago
The missed patch for comment 71. :P
Assignee

Updated

5 years ago
Attachment #8495291 - Flags: review?(bugs)
Comment on attachment 8495291 [details] [diff] [review]
part 3.g/4: listen to xpcom shutdown

Why do we need this at all?
xpcom-shutdown happens very late during shutdown, don't we usually kill
child processes before that?

Could you explain, and ask review again if needed.
Attachment #8495291 - Flags: review?(bugs)
Assignee

Comment 81

5 years ago
(In reply to Olli Pettay [:smaug] from comment #80)
> Comment on attachment 8495291 [details] [diff] [review]
> part 3.g/4: listen to xpcom shutdown
> 
> Why do we need this at all?
> xpcom-shutdown happens very late during shutdown, don't we usually kill
> child processes before that?
> 
> Could you explain, and ask review again if needed.

I don't know XPCOM shutdown very late even after child process being killed. Will move cleanup steps into dtor (part 3.b/4) instead.
Assignee

Comment 82

5 years ago
Remove VoicemailIPCService::Shutdown().
Attachment #8492709 - Attachment is obsolete: true
Attachment #8495689 - Flags: review+
Assignee

Updated

5 years ago
Attachment #8495291 - Attachment is obsolete: true

Comment 85

5 years ago
Hey Vicamo, wondering why is NotifyInfoChanged in dom/voicemail/Voicemail.cpp not currently implemented?
Flags: needinfo?(vicamo)
Assignee

Comment 86

5 years ago
(In reply to Anshul from comment #85)
> Hey Vicamo, wondering why is NotifyInfoChanged in
> dom/voicemail/Voicemail.cpp not currently implemented?

Because we're not caching the two properties, number and displayName, in Voicemail object. All access operations to them are directed to underlying service object.
Flags: needinfo?(vicamo)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.