833229 - B2G RIL: use ipdl as IPC in MozVoicemail

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I've not been here for a while. Set assignee to default.

Comment 2

[José Antonio Olivera Ortega [:jaoo]]

I'll be happy to help here. Hsin-Yi, can I go for it? Thanks!

Comment 3

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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. :)

Comment 4

[José Antonio Olivera Ortega [:jaoo]]

(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.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(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

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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

Comment 7

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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. :)

Comment 8

[José Antonio Olivera Ortega [:jaoo]]

(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.

Comment 9

[Ken Chang[:kenkai]]

Put this bug into backlog.

Comment 10

[Anshul]

Hey Wesley, is this bug intended for 2.1?

Comment 11

[Ken Chang[:kenkai]]

(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.

Comment 12

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

Attachment: part 1/5: rename VoicemailProvider to VoicemailService

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.

Comment 13

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

Attachment: part 2.a/5: implement VoicemailStatus in C++

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.

Comment 14

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

Attachment: part 2.b/5: owning MozVoicemailStatus at client side

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.

Comment 15

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

Attachment: part 3.a/5: build voicemail by default

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.

Comment 16

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

Attachment: part 3.b/5: add voicemail IPDL protocol

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.

Comment 17

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

Attachment: part 4/5: add VoicemailFactory for creating different voicemail services

In child process, we should always create VoicemailChild as nsIVoicemailService. In parent process, the Gonk implementation is available in the next part.

Comment 18

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

Attachment: part 5/5: Gonk VoicemailService

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.

Comment 19

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

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

[Anshul]

(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?

Comment 21

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

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.

Comment 22

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

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.

Comment 23

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

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.

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.

Comment 25

[Ken Chang[:kenkai]]

(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 26

[Edgar Chen [:edgar]]

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.

Comment 27

[Edgar Chen [:edgar]]

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?

Comment 28

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

(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.

Comment 29

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

Attachment: part 1/4: rename VoicemailProvider to VoicemailService : v2

Rebase after bug 1034312, bug 843452.

Comment 30

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

Attachment: part 2.a/4: wrap arguments list

A trivial patch to place only one argument per line.

Comment 31

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

Attachment: part 2.b/4: build voicemail by default : v2

Rebase after bug 843452, bug 900551.

Comment 33

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

Attachment: part 3.a/4: IPDL and internal XPIDL interface changes

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.

Comment 34

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

Attachment: part 3.b/4: add voicemail IPDL protocol : v2

Comment 35

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

Attachment: part 3.c/4: implement VoicemailStatus in C++ : v2

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.

Comment 36

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

Attachment: part 3.d/4: DOM changes

Comment 37

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

Attachment: part 3.e/4: Gonk VoicemailService

Comment 38

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

Attachment: part 3.f/4: add VoicemailFactory for creating different voicemail services : v2

Rebase after bug 843452.

Comment 39

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

Attachment: part 4/4: fix test cases

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

Comment 40

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

Attachment: part 2.a/4: wrap arguments list : v2

Update commit summary only.

Comment 41

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

Attachment: part 3.a/4: IPDL and internal XPIDL interface changes : v2

Per bug 1063304 comment 17, rename nsIVoicemailService::length to numItems as well.

Comment 42

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

Attachment: part 3.b/4: add voicemail IPDL protocol : v3

1) Rebase after bug 994190.

2) Address bug 1063304 comment 17.

May I have your review?

Comment 43

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

Attachment: part 3.c/4: implement VoicemailStatus in C++ : v3

Fix build failure in debug build.

Comment 44

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

Attachment: part 3.d/4: DOM changes : v2

1) don't include nsDOMClassInfo.h any more.
2) address bug 1063304 comment 17.

Comment 45

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

Attachment: part 3.e/4: Gonk VoicemailService : v2

Address bug 1063304 comment 17.

Comment 46

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

Attachment: part 3.f/4: voicemail service instantiation : v3

Rebase after bug 1064231.

Comment 47

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

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 48

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

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.

Comment 49

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

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+

Comment 50

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

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 51

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

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.

Comment 52

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

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.

Comment 53

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

Maybe nsIVoiceMail could have Internal in its name or something.

Comment 54

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

(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.

Comment 55

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

mozilla::dom::Voicemail is good name to me since it implements the webidl interface.
Better to change the internal interface names.

Comment 56

[Edgar Chen [:edgar]]

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.

Comment 57

[Edgar Chen [:edgar]]

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);

Comment 58

[Edgar Chen [:edgar]]

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 = "[phone]",
> +      body = "1 new voicemail";

nit: One declaration per line.

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

@@ +38,3 @@
>  
> +  let sender = "[phone]",
> +      body = aMessageCount + " voicemails";

Ditto.

Comment 59

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

Attachment: part 1/4: rename VoicemailProvider to VoicemailService : v2

Update commit summary only.

Comment 60

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

Attachment: part 2.a/4: wrap arguments list : v2

Update commit summary only.

Comment 61

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

Attachment: part 2.b/4: build voicemail by default : v2

Update commit summary only.

Comment 62

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

Attachment: part 3.a/4: IPDL and internal XPIDL interface changes : v2

Use 'nsIVoicemailProvider' for each entity and 'nsIVoicemailService' as the manager of them.

Comment 63

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

Attachment: part 3.b/4: add voicemail IPDL protocol : v4

1) Rebase after bug 994190 being backed out.

2) s/item/provider/ except for the name of the two access methods in nsIVoicemailService.

Comment 64

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

Attachment: part 3.c/4: implement VoicemailStatus in C++ : v4

Accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).

Comment 65

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

Attachment: part 3.d/4: DOM changes : v3

Accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).

Comment 66

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

Attachment: part 3.e/4: Gonk VoicemailService : v3

Address comment 57.

Comment 67

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

Attachment: part 3.f/4: voicemail service instantiation : v4

1) update commit summary,

2) accommodate to internal interface name change in part 3.a (attachment 8492708 [details] [diff] [review]).

Comment 68

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

Attachment: part 4/4: fix test cases : v2

Address comment 58.

Comment 69

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

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.

Comment 70

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

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.

Comment 71

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

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.

Comment 72

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

(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.

Comment 73

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

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)

Comment 74

[Edgar Chen [:edgar]]

Comment on attachment 8492712 [details] [diff] [review]
part 3.e/4: Gonk VoicemailService : v3

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

Thank you. :)

Comment 75

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

Attachment: part 2.b/4: build voicemail by default : v3

Rebase only.

Comment 76

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

Attachment: part 3.a/4: IPDL and internal XPIDL interface changes : v3

Address comment 48, comment 70.

Comment 77

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

Attachment: part 3.d/4: DOM changes : v4

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.

Comment 78

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

Attachment: part 3.g/4: listen to xpcom shutdown

The missed patch for comment 71. :P

Comment 79

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

https://tbpl.mozilla.org/?tree=Try&rev=115b6a9bd62a

Comment 80

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

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.

Comment 81

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

(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.

Comment 82

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

Attachment: part 3.b/4: add voicemail IPDL protocol : v5

Remove VoicemailIPCService::Shutdown().

Comment 83

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

https://hg.mozilla.org/integration/b2g-inbound/rev/de92c4532ac4
https://hg.mozilla.org/integration/b2g-inbound/rev/866f6bfa05c2
https://hg.mozilla.org/integration/b2g-inbound/rev/1b4442700ce4
https://hg.mozilla.org/integration/b2g-inbound/rev/7987b5cd8a18
https://hg.mozilla.org/integration/b2g-inbound/rev/022a7506f13e
https://hg.mozilla.org/integration/b2g-inbound/rev/6f906bc6cc5f
https://hg.mozilla.org/integration/b2g-inbound/rev/48caa28fa726
https://hg.mozilla.org/integration/b2g-inbound/rev/b3382d21812f
https://hg.mozilla.org/integration/b2g-inbound/rev/20f42bc73cbe
https://hg.mozilla.org/integration/b2g-inbound/rev/21f9a42963dd

Comment 84

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/de92c4532ac4
https://hg.mozilla.org/mozilla-central/rev/866f6bfa05c2
https://hg.mozilla.org/mozilla-central/rev/1b4442700ce4
https://hg.mozilla.org/mozilla-central/rev/7987b5cd8a18
https://hg.mozilla.org/mozilla-central/rev/022a7506f13e
https://hg.mozilla.org/mozilla-central/rev/6f906bc6cc5f
https://hg.mozilla.org/mozilla-central/rev/48caa28fa726
https://hg.mozilla.org/mozilla-central/rev/b3382d21812f
https://hg.mozilla.org/mozilla-central/rev/20f42bc73cbe
https://hg.mozilla.org/mozilla-central/rev/21f9a42963dd

Comment 85

[Anshul]

Hey Vicamo, wondering why is NotifyInfoChanged in dom/voicemail/Voicemail.cpp not currently implemented?

Comment 86

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

(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.