Closed
Bug 814634
Opened 10 years ago
Closed 9 years ago
WebVoicemail API: support multiple sim cards
Categories
(Firefox OS Graveyard :: RIL, defect)
Tracking
(blocking-b2g:1.3+, firefox28 fixed)
Tracking | Status | |
---|---|---|
firefox28 | --- | fixed |
People
(Reporter: hsinyi, Assigned: hsinyi)
References
Details
(Keywords: dev-doc-needed, Whiteboard: [FT:RIL])
Attachments
(4 files, 9 obsolete files)
1.47 KB,
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
3.18 KB,
patch
|
Details | Diff | Splinter Review | |
12.78 KB,
patch
|
Details | Diff | Splinter Review | |
4.91 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
We should figure out how app communicates with multiple sim cards via WebVoicemail DOM API.
Updated•10 years ago
|
blocking-b2g: --- → leo?
Comment 1•10 years ago
|
||
Blocking-, no longer targeting Leo release. Product will retarget for future release.
blocking-b2g: leo? → -
Assignee | ||
Comment 2•10 years ago
|
||
github working branch
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → htsai
Updated•9 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #715393 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Updated•9 years ago
|
blocking-b2g: - → 1.3+
Assignee | ||
Updated•9 years ago
|
Target Milestone: --- → 1.3 Sprint 3 - 10/25
Updated•9 years ago
|
Whiteboard: [FT:RIL]
Assignee | ||
Updated•9 years ago
|
Target Milestone: 1.3 Sprint 3 - 10/25 → 1.3 Sprint 4 - 11/8
Assignee | ||
Comment 8•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #823771 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #823768 -
Flags: review?(vyang)
Assignee | ||
Updated•9 years ago
|
Attachment #823770 -
Flags: review?(khuey)
Assignee | ||
Updated•9 years ago
|
Attachment #823909 -
Flags: review?(vyang)
Updated•9 years ago
|
Attachment #823768 -
Flags: review?(vyang) → review+
Comment 14•9 years ago
|
||
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•9 years ago
|
||
(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•9 years ago
|
||
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)
Assignee | ||
Comment 17•9 years ago
|
||
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.
Assignee | ||
Comment 18•9 years ago
|
||
(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.
Assignee | ||
Comment 19•9 years ago
|
||
(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•9 years ago
|
||
(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'.
Assignee | ||
Comment 21•9 years ago
|
||
(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•9 years ago
|
||
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+
Attachment #823767 -
Flags: review?(khuey) → 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 24•9 years ago
|
||
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+
Assignee | ||
Comment 25•9 years ago
|
||
comment 24 addressed. Thank you for the review.
Attachment #823767 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
comment 20 and comment 22 addressed.
Attachment #823909 -
Attachment is obsolete: true
Assignee | ||
Comment 27•9 years ago
|
||
comment 23 addressed.
Attachment #823770 -
Attachment is obsolete: true
Assignee | ||
Comment 28•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 30•9 years ago
|
||
try looks good: https://tbpl.mozilla.org/?tree=Try&rev=997d29cb08f8 Bug 927724 is fixed. Patches are safe to land.
Assignee | ||
Comment 31•9 years ago
|
||
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
Assignee | ||
Comment 32•9 years ago
|
||
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•9 years ago
|
||
(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 :-)
Assignee | ||
Comment 34•9 years ago
|
||
(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•9 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
status-firefox28:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•