814634 - WebVoicemail API: support multiple sim cards

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

We should figure out how app communicates with multiple sim cards via WebVoicemail DOM API.

Comment 1

[Dietrich Ayala (:dietrich)]

Blocking-, no longer targeting Leo release. Product will retarget for future release.

Comment 2

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: Working branch

github working branch

Comment 3

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: Working branch

updated

Comment 4

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: WPI - part1 - webAPI

Comment 5

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: WIP - part2 - internal API

Comment 6

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: WIP - part3 - RIL impl

Comment 8

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p1.patch - WebAPI

Remove numeric constants per Marcos' comment on dev-webapi.

Comment 9

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p2.patch - internal API

Comment 10

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p3.patch - DOM impl

Comment 11

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p4.patch - RIL & test

Comment 12

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p4v2.patch - RIL & Test

Comment 13

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Comment on attachment 823767 [details] [diff] [review]
p1.patch - WebAPI

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

Hi Gene and Kyle,

In addition to have multisim support, the patch also removes numeric constants per Marcos' comment on dev-webapi. Could you please help review it? Thank you.

Comment 14

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

Comment on attachment 823768 [details] [diff] [review]
p2.patch - internal API

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

::: dom/voicemail/nsIVoicemailProvider.idl
@@ +36,5 @@
>    void unregisterVoicemailMsg(in nsIVoicemailListener listener);
>  
> +  nsIDOMMozVoicemailStatus getVoicemailStatus(in unsigned long clientId);
> +  DOMString getVoicemailNumber(in unsigned long clientId);
> +  DOMString getVoicemailDisplayName(in unsigned long clientId);

nit: ok, that's kind of strange now.  Just have 'serviceId' for them, shall we?  'clientId' was first used in rild/rilproxy thing.

Comment 15

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> nit: ok, that's kind of strange now.  Just have 'serviceId' for them, shall
> we?  'clientId' was first used in rild/rilproxy thing.

Well, just see a lot of 'clientId' in RILContentHelper, but other DOM interfaces and nsIRilNetworkInterface use 'serviceId'.

Comment 16

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

Comment on attachment 823909 [details] [diff] [review]
p4v2.patch - RIL & Test

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

::: dom/system/gonk/RILContentHelper.js
@@ +290,2 @@
>    hasMessages: false,
> +  messageCount: -1,

Why?  Isn't a numeric constant here a good idea?

@@ +1459,1 @@
>      this.registerListener("_voicemailListeners", 0, listener);

It follows only voicemail events from first radio interface are propagated to DOM.  Sounds not a good idea.

Comment 17

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Thanks for the comment, Vicamo.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> Comment on attachment 823909 [details] [diff] [review]
> p4v2.patch - RIL & Test
> 
> Review of attachment 823909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RILContentHelper.js
> @@ +290,2 @@
> >    hasMessages: false,
> > +  messageCount: -1,
> 
> Why?  Isn't a numeric constant here a good idea?

It's the feedback from dev-webapi.

> 
> @@ +1459,1 @@
> >      this.registerListener("_voicemailListeners", 0, listener);
> 
> It follows only voicemail events from first radio interface are propagated
> to DOM.  Sounds not a good idea.

This line is for adding a listener into the listener array in RILContentHelper. The reason I use '0' here is that |nsIVoicemailProvider.registerVoicemailMsg| doesn't take any 'clientId' input parameter while I'd like to reuse the existing data structure |this[listenerType][clientId]|. RadioInterfaceLayer still send messages to RILContentHelper no matter by which RadioInterface the notification is sent out. Should be no problem with your concern.

I forgot to mention this depends on bug 818353.

Comment 18

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #14)
> > nit: ok, that's kind of strange now.  Just have 'serviceId' for them, shall
> > we?  'clientId' was first used in rild/rilproxy thing.
> 
> Well, just see a lot of 'clientId' in RILContentHelper, but other DOM
> interfaces and nsIRilNetworkInterface use 'serviceId'.

In our previous meeting, we decided to use 'clientId' in RIL internal API and implementation as it's kinda weird to call ril_worker/RadioInterface 'service.' In addition, we've been always using 'clientId' in RIL code.

Comment 19

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #17)
> Thanks for the comment, Vicamo.
> 
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> > Comment on attachment 823909 [details] [diff] [review]
> > p4v2.patch - RIL & Test
> > 
> > Review of attachment 823909 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/system/gonk/RILContentHelper.js
> > @@ +290,2 @@
> > >    hasMessages: false,
> > > +  messageCount: -1,
> > 
> > Why?  Isn't a numeric constant here a good idea?
> 

It's the feedback from dev-webapi. And according to [1], it makes sense to remove the numeric constant from nsIDOMMozVoicemailStatus.idl.

[1] http://www.w3.org/TR/api-design/#numerical-constants

Comment 20

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

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #19)
> It's the feedback from dev-webapi. And according to [1], it makes sense to
> remove the numeric constant from nsIDOMMozVoicemailStatus.idl.
> 
> [1] http://www.w3.org/TR/api-design/#numerical-constants

Yes, but please have a comment after that magic number '-1'.

Comment 21

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #20)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #19)
> > It's the feedback from dev-webapi. And according to [1], it makes sense to
> > remove the numeric constant from nsIDOMMozVoicemailStatus.idl.
> > 
> > [1] http://www.w3.org/TR/api-design/#numerical-constants
> 
> Yes, but please have a comment after that magic number '-1'.

Sure, will do!

Comment 22

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

Comment on attachment 823909 [details] [diff] [review]
p4v2.patch - RIL & Test

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

::: dom/system/gonk/RILContentHelper.js
@@ +1459,1 @@
>      this.registerListener("_voicemailListeners", 0, listener);

Per offline discussion, please have comment saying "all voicemail events are routed to listener for client id 0.  See |handleVoicemailNotification|."  Thank you.

@@ +1882,5 @@
>      }
>  
>      if (changed) {
> +      // To follow the event delivering scheme, we add a dummy clientId 0.
> +      this._deliverEvent(0,

And here.  "Routes all voicemail events to handlers registered for client id 0".

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 823770 [details] [diff] [review]
p3.patch - DOM impl

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

super-nit: Turn Optional<uint32_t > into Optional<uint32_t> (no space) throughout this patch.

I would like to see this again.

::: dom/voicemail/Voicemail.cpp
@@ +71,5 @@
>  
> +bool
> +Voicemail::IsValidServiceId(uint32_t aServiceId) const
> +{
> +  int32_t numClients = mozilla::Preferences::GetInt(kPrefRilNumRadioInterfaces, 1);

GetUint?

@@ +96,5 @@
> +    }
> +    id = aServiceId.Value();
> +  } else {
> +    mProvider->GetVoicemailDefaultServiceId(&id);
> +  }

Please put this code into a separate function and call it, rather than duplicating it three times.

Maybe something like bool PassedOrDefaultServiceId(const Optional<uint32_t>&, uint32_t&). Slightly tricky because there are two "result" values (whether the function succeeded or not and the service ID).

Comment 24

[Gene Lian (I already quit Mozilla)]

Comment on attachment 823767 [details] [diff] [review]
p1.patch - WebAPI

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

sr=gene. Thank you!

::: dom/webidl/MozVoicemail.webidl
@@ +11,5 @@
>  interface MozVoicemail : EventTarget
>  {
>    /**
> +   * The current voicemail status of a specified service, or null when the
> +   * status is unknown

Add a period.

@@ +19,4 @@
>  
>    /**
> +   * The voicemail box dialing number of a specified service, or null if one
> +   * wasn't found

Add a period.

@@ +26,3 @@
>  
>    /**
>     * The display name of the voicemail box dialing number, or null if one

This line is redundant.

@@ +26,5 @@
>  
>    /**
>     * The display name of the voicemail box dialing number, or null if one
> +   * The display name of the voicemail box dialing number of a specified service,
> +   * or null if one wasn't found

Add a period.

Comment 25

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p1v2.patch - WebAPI r=khuey, sr=gene

comment 24 addressed. Thank you for the review.

Comment 26

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p4v3.patch - RIL and test. r=vicamo

comment 20 and comment 22 addressed.

Comment 27

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Attachment: p3v2.patch - DOM

comment 23 addressed.

Comment 28

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Comment on attachment 825802 [details] [diff] [review]
p3v2.patch - DOM

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

Hi Kyle,

I've addressed your comment 23. Would you please take a look again? Thank.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 825802 [details] [diff] [review]
p3v2.patch - DOM

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

r=me

Comment 30

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

try looks good: https://tbpl.mozilla.org/?tree=Try&rev=997d29cb08f8
Bug 927724 is fixed. Patches are safe to land.

Comment 31

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

My bad... r= info is missing in the bug comment. 
Backout from b2g-inbound...

遠端:   https://hg.mozilla.org/integration/b2g-inbound/rev/7fc68005bf0f
遠端:   https://hg.mozilla.org/integration/b2g-inbound/rev/3a64feaff928
遠端:   https://hg.mozilla.org/integration/b2g-inbound/rev/4d432f97d4b5
遠端:   https://hg.mozilla.org/integration/b2g-inbound/rev/984cc39d660c

Comment 32

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

Sorry for the noise. Pushing to b2g-inboud again.

   https://hg.mozilla.org/integration/b2g-inbound/rev/32206ae7721b
   https://hg.mozilla.org/integration/b2g-inbound/rev/f1f17c5ae015
   https://hg.mozilla.org/integration/b2g-inbound/rev/3f4814326443
   https://hg.mozilla.org/integration/b2g-inbound/rev/95d8eeb0f741

Comment 33

[Ed Morley [:emorley]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #31)
> My bad... r= info is missing in the bug comment. 
> Backout from b2g-inbound...

Hi! :-)

The r= is nice to have but not essential. Unfortunately the backout and the relanding were done as separate pushes, both of which without the string "DONTBUILD" in the commit message - so this caused another approx 500 hours of compute time (2 extra pushes x ~250 hours) to be spent on what was a no-op in terms of code change.

If correcting the commit message (eg incorrect bug number, which is more critical than the r=), please use DONTBUILD to save resources. Thank you :-)

Comment 34

[Hsin-Yi Tsai (away for a while) [:hsinyi]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #33)
> (In reply to Hsin-Yi Tsai  [:hsinyi] from comment #31)
> > My bad... r= info is missing in the bug comment. 
> > Backout from b2g-inbound...
> 
> Hi! :-)
> 
> The r= is nice to have but not essential. Unfortunately the backout and the
> relanding were done as separate pushes, both of which without the string
> "DONTBUILD" in the commit message - so this caused another approx 500 hours
> of compute time (2 extra pushes x ~250 hours) to be spent on what was a
> no-op in terms of code change.
> 
> If correcting the commit message (eg incorrect bug number, which is more
> critical than the r=), please use DONTBUILD to save resources. Thank you :-)
Hi Ed,
Sorry for the disaster :( and thank you for the kind information. I've learnt the right way!

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32206ae7721b
https://hg.mozilla.org/mozilla-central/rev/f1f17c5ae015
https://hg.mozilla.org/mozilla-central/rev/3f4814326443
https://hg.mozilla.org/mozilla-central/rev/95d8eeb0f741