Closed
Bug 793192
Opened 12 years ago
Closed 12 years ago
MMI Codes: support call forwarding
Categories
(Core :: DOM: Device Interfaces, defect, P1)
Core
DOM: Device Interfaces
Tracking
()
People
(Reporter: ferjm, Assigned: ferjm)
References
Details
(Keywords: feature, Whiteboard: [LOE:S])
Attachments
(4 files, 5 obsolete files)
4.54 KB,
patch
|
marshall
:
review+
|
Details | Diff | Splinter Review |
5.28 KB,
patch
|
marshall
:
review+
|
Details | Diff | Splinter Review |
355 bytes,
text/html
|
arcturus
:
review+
|
Details |
5.66 KB,
patch
|
Details | Diff | Splinter Review |
Call forwarding is a device certification requirement. Even if there are not specific test cases of call forwarding through MMI codes for the device certification that I know about, this codes allows the device to handle the call forwarding functionality, so we should probably support this too.
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 1•12 years ago
|
||
Is this a P1 blockers or is it a nice-to-have?
Whiteboard: [blocked-on-input philikon]
Comment 2•12 years ago
|
||
(In reply to Andrew Overholt [:overholt] from comment #1)
> Is this a P1 blockers or is it a nice-to-have?
Blocker because device certification requirement.
Whiteboard: [blocked-on-input philikon]
Updated•12 years ago
|
blocking-basecamp: ? → +
Comment 3•12 years ago
|
||
Fernando, please re-assign if you're not the correct owner here.
Assignee: nobody → ferjmoreno
Updated•12 years ago
|
Assignee: ferjmoreno → nobody
Component: DOM: Apps → DOM: Device Interfaces
Updated•12 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Updated•12 years ago
|
Whiteboard: [LOE:S]
Assignee | ||
Comment 4•12 years ago
|
||
I consider this task as LOE:S once bug 793186 and bug 793208 lands
Updated•12 years ago
|
Priority: -- → P1
Assignee | ||
Comment 5•12 years ago
|
||
I am still waiting for Bug 793208 to land, but in the mean time I can upload my WIP patches.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #675617 -
Flags: feedback?(marshall)
Assignee | ||
Updated•12 years ago
|
Attachment #675616 -
Flags: feedback?(marshall)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #676225 -
Flags: review?(marshall)
Assignee | ||
Updated•12 years ago
|
Attachment #675616 -
Attachment is obsolete: true
Attachment #675616 -
Flags: feedback?(marshall)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #675617 -
Attachment is obsolete: true
Attachment #675617 -
Flags: feedback?(marshall)
Attachment #676226 -
Flags: review?(marshall)
Assignee | ||
Comment 9•12 years ago
|
||
Attachment #676227 -
Flags: review?(marshall)
Comment 10•12 years ago
|
||
Comment on attachment 676225 [details] [diff] [review]
Part 1: RIL
Review of attachment 676225 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/ril_worker.js
@@ +2384,5 @@
> + options.rilMessageType = "sendMMI";
> + options.reason = MMI_SC_TO_CF_REASON[sc];
> + options.number = mmi.sia;
> + options.serviceClass = mmi.sib;
> + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) {
nit: would it be better to do this check before we set all the fields on |options|?
Attachment #676225 -
Flags: review?(marshall) → review+
Comment 11•12 years ago
|
||
Comment on attachment 676226 [details] [diff] [review]
Part 2: RIL. Query CF status
Review of attachment 676226 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/RILContentHelper.js
@@ +998,5 @@
> + this._cfRulesToMobileCfInfo(message.rules);
> + message.result = message.rules;
> + }
> +
> + Services.DOMRequest.fireSuccess(request, message.result);
Is it necessary to use |message.result| instead of |message.rules| here?
Attachment #676226 -
Flags: review?(marshall) → review+
Comment 12•12 years ago
|
||
Comment on attachment 676227 [details] [diff] [review]
Part 3: Tests
Review of attachment 676227 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/system/gonk/tests/test_ril_worker_mmi.js
@@ +327,5 @@
> +
> + worker.RIL.sendMMI({mmi: mmi});
> +
> + do_check_eq(postedMessage.errorMsg, GECKO_ERROR_SUCCESS);
> + do_check_true(postedMessage.success);
nit: trailing space
@@ +337,5 @@
> + run_next_test();
> +});
> +
> +add_test(function test_sendMMI_call_forwarding_deactivation() {
> + setCallForwardSuccess("#21*12345*99*10#");
nit: 1 space indentation should be 2 :)
@@ +350,5 @@
> + },
> + postMessage: function fakePostMessage(message) {
> + postedMessage = message;
> + }
> + });
Same as my comment in the tests for Bug 793208: The general newWorker / postedMessage pattern seems to be reused a lot here.. maybe we could separate this out into a helper function?
@@ +425,5 @@
> +});
> +
> +add_test(function test_sendMMI_call_forwarding_erasure() {
> + setCallForwardSuccess("##21*12345*99#");
> +
nit: trailing spaces
Attachment #676227 -
Flags: review?(marshall) → review+
Assignee | ||
Comment 13•12 years ago
|
||
(In reply to Marshall Culpepper [:marshall_law] from comment #10)
> Comment on attachment 676225 [details] [diff] [review]
> Part 1: RIL
>
> Review of attachment 676225 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/system/gonk/ril_worker.js
> @@ +2384,5 @@
> > + options.rilMessageType = "sendMMI";
> > + options.reason = MMI_SC_TO_CF_REASON[sc];
> > + options.number = mmi.sia;
> > + options.serviceClass = mmi.sib;
> > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) {
>
> nit: would it be better to do this check before we set all the fields on
> |options|?
Thanks for the quick revision Marshall!
I am afraid that the call to |_sendMMIError("CF_QUERY_STATUS_NOT_SUPPORTED");| might have lead you to a confusion here. That call is changed for a |this.queryCallForwardStatus(options)| on the second patch that makes use of the |options| object. Sorry about that.
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Marshall Culpepper [:marshall_law] from comment #11)
> Comment on attachment 676226 [details] [diff] [review]
> Part 2: RIL. Query CF status
>
> Review of attachment 676226 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: dom/system/gonk/RILContentHelper.js
> @@ +998,5 @@
> > + this._cfRulesToMobileCfInfo(message.rules);
> > + message.result = message.rules;
> > + }
> > +
> > + Services.DOMRequest.fireSuccess(request, message.result);
>
> Is it necessary to use |message.result| instead of |message.rules| here?
The RadioInterfaceLayer sends |message.result| instead of |message.rules| for some MMI functionality. An alternative to that code would be:
if (message.success && message.rules) {
this._cfRulesToMobileCfInfo(message.rules);
Services.DOMRequest.fireSuccess(request, message.rules);
}
Services.DOMRequest.fireSuccess(request, message.result);
Is that what you meant?
Assignee | ||
Comment 15•12 years ago
|
||
I'd like to also add the Gaia part before landing this patches.
Comment 16•12 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] (PTO until 5th Nov) from comment #15)
> I'd like to also add the Gaia part before landing this patches.
Is there a bug # for that?
Assignee | ||
Comment 17•12 years ago
|
||
No, I was planning to also attach the Gaia patch in this bug. Please, let me know if you want me to land the platform part and open a follow up bug for the Gaia patch. I am uploading a WIP patch that I wrote on the plane and I couldn't test it yet that should look pretty much like the final patch.
Assignee | ||
Comment 18•12 years ago
|
||
I had to expose the CALL_FORWARD_* from nsIDOMMozMobileConnection instead from nsIDOMMozMobileCFInfo since this last interface is not exposed to content.
Attachment #676225 -
Attachment is obsolete: true
Attachment #678470 -
Flags: review?(marshall)
Assignee | ||
Comment 19•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•12 years ago
|
Attachment #678482 -
Flags: review?(francisco.jordano)
Assignee | ||
Updated•12 years ago
|
Attachment #678193 -
Attachment is obsolete: true
Comment 20•12 years ago
|
||
Comment on attachment 678470 [details] [diff] [review]
Part 1: RIL
Review of attachment 678470 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good :)
::: dom/system/gonk/ril_worker.js
@@ +2384,5 @@
> + options.rilMessageType = "sendMMI";
> + options.reason = MMI_SC_TO_CF_REASON[sc];
> + options.number = mmi.sia;
> + options.serviceClass = mmi.sib;
> + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) {
nit: can we do this validation first before we set values on |options|?
Attachment #678470 -
Flags: review?(marshall) → review+
Comment 21•12 years ago
|
||
Ack, just realized you will need to change UUIDs for these interfaces as well.. please make sure you do that. We will probably need to get an exception for this change on aurora, as well..
Assignee | ||
Comment 22•12 years ago
|
||
r=marshall_law, sr=sicking
I've talked with Jonas about the required changes in the idl and he agreed to give his sr+
Attachment #678470 -
Attachment is obsolete: true
Assignee | ||
Comment 23•12 years ago
|
||
(In reply to Marshall Culpepper [:marshall_law] from comment #20)
> Comment on attachment 678470 [details] [diff] [review]
> Part 1: RIL
>
> Review of attachment 678470 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> Looks good :)
>
> ::: dom/system/gonk/ril_worker.js
> @@ +2384,5 @@
> > + options.rilMessageType = "sendMMI";
> > + options.reason = MMI_SC_TO_CF_REASON[sc];
> > + options.number = mmi.sia;
> > + options.serviceClass = mmi.sib;
> > + if (options.action == CALL_FORWARD_ACTION_QUERY_STATUS) {
>
> nit: can we do this validation first before we set values on |options|?
Comment 13 :)
Updated•12 years ago
|
Attachment #678482 -
Flags: review?(francisco.jordano) → review+
Comment 24•12 years ago
|
||
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).
If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Assignee | ||
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c1feb6d4a8b5
https://hg.mozilla.org/mozilla-central/rev/8ce4b53fb015
https://hg.mozilla.org/mozilla-central/rev/85867b1963cf
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
Updated•12 years ago
|
Blocks: b2g-v1-certification
You need to log in
before you can comment on or make changes to this bug.
Description
•