Closed Bug 814634 Opened 8 years ago Closed 7 years ago

WebVoicemail API: support multiple sim cards

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
1.3 Sprint 4 - 11/8
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [FT:RIL])

Attachments

(4 files, 9 obsolete files)

We should figure out how app communicates with multiple sim cards via WebVoicemail DOM API.
blocking-b2g: --- → leo?
Blocking-, no longer targeting Leo release. Product will retarget for future release.
blocking-b2g: leo? → -
Attached file Working branch (obsolete) —
github working branch
Attached file Working branch (obsolete) —
updated
Attachment #715081 - Attachment is obsolete: true
Assignee: nobody → htsai
Depends on: 818352
Blocks: 918553
Attached patch WPI - part1 - webAPI (obsolete) — Splinter Review
Attachment #715393 - Attachment is obsolete: true
Attached patch WIP - part2 - internal API (obsolete) — Splinter Review
Depends on: 842239
Attached patch WIP - part3 - RIL impl (obsolete) — Splinter Review
Depends on: 926302
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Duplicate of this bug: 818352
Blocks: 927724
blocking-b2g: - → 1.3+
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Whiteboard: [FT:RIL]
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Attached patch p1.patch - WebAPI (obsolete) — Splinter Review
Remove numeric constants per Marcos' comment on dev-webapi.
Attachment #811844 - Attachment is obsolete: true
Attachment #811846 - Attachment is obsolete: true
Attachment #812956 - Attachment is obsolete: true
Attached patch p3.patch - DOM impl (obsolete) — Splinter Review
Attached patch p4.patch - RIL & test (obsolete) — Splinter Review
Attached patch p4v2.patch - RIL & Test (obsolete) — Splinter Review
Attachment #823771 - Attachment is obsolete: true
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.
Attachment #823767 - Flags: superreview?(gene.lian)
Attachment #823767 - Flags: review?(khuey)
Attachment #823768 - Flags: review?(vyang)
Attachment #823770 - Flags: review?(khuey)
Attachment #823909 - Flags: review?(vyang)
Attachment #823768 - Flags: review?(vyang) → review+
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.
(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 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.
Attachment #823909 - Flags: review?(vyang)
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.
(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.
(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
(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'.
(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 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".
Attachment #823909 - Flags: review+
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).
Attachment #823770 - Flags: review?(khuey) → review-
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.
Attachment #823767 - Flags: superreview?(gene.lian) → superreview+
comment 24 addressed. Thank you for the review.
Attachment #823767 - Attachment is obsolete: true
comment 20 and comment 22 addressed.
Attachment #823909 - Attachment is obsolete: true
Attached patch p3v2.patch - DOMSplinter Review
comment 23 addressed.
Attachment #823770 - Attachment is obsolete: true
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.
Attachment #825802 - Flags: review?(khuey)
Comment on attachment 825802 [details] [diff] [review]
p3v2.patch - DOM

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

r=me
Attachment #825802 - Flags: review?(khuey) → review+
No longer blocks: 927724
Depends on: 927724
try looks good: https://tbpl.mozilla.org/?tree=Try&rev=997d29cb08f8
Bug 927724 is fixed. Patches are safe to land.
(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 :-)
(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!
Blocks: 938440
You need to log in before you can comment on or make changes to this bug.