Closed Bug 997601 Opened 6 years ago Closed 5 years ago

[DSDS][Gaia] Gaia needs to save the caller id preference and restore it when reboot

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 wontfix, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S6 (25apr)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: arthurcc, Assigned: arthurcc)

References

Details

Attachments

(3 files)

Currently the caller id preference is maintained by gecko. After bug 997854 lands, we need to maintain the preference in gaia using mozSettings. The value must be restored when reboot.
Assignee: nobody → arthur.chen
Blocks: 997584
No longer blocks: 995458
No longer depends on: 997584
Hi Alive, Jose, to be consistent to other telephony related settings (voice privacy, roaming...), we need to maintain the caller id preferences in mozSettings. Please help review the patch, thanks!
Attachment #8408107 - Flags: review?(josea.olivera)
Attachment #8408107 - Flags: review?(alive)
Status: NEW → ASSIGNED
Target Milestone: --- → 1.4 S6 (25apr)
Attachment #8408107 - Flags: review?(alive) → review+
triage: bug 997854 did not land in 1.3T, don't believe this is needed for 1.3T, not blocking tarako release
blocking-b2g: 1.3T? → ---
Bug 997584 is the gecko part of this issue and is blocks bug 995458. We need this patch in v1.3T.
blocking-b2g: --- → 1.3T?
Flags: needinfo?(jcheng)
triage: 1.3T+ for blocking a 1.3T+
blocking-b2g: 1.3T? → 1.3T+
Flags: needinfo?(jcheng)
Whiteboard: [ETA: 4/25]
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Arthur, left a comment at [1]. Depending on what we decide about the comment at [1] this patch might take care of saving the permanent CLIR preference set via MMI codes as well.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=997584#c19
Attachment #8408107 - Flags: review?(josea.olivera)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> Comment on attachment 8408107 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/18408
> 
> Arthur, left a comment at [1]. Depending on what we decide about the comment
> at [1] this patch might take care of saving the permanent CLIR preference
> set via MMI codes as well.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=997584#c19

As Hsin-Yi commented at [2] Gaia should take care of it as well. Thanks guys!

[2] https://bugzilla.mozilla.org/show_bug.cgi?id=997584#c20
Hi Arthur, 
Looks gecko needs to offer one more event to notify Gaia CLIR status changes. Otherwise, how would Settings know if CLIR is changed via MMI? My understanding correct?
Indeed, we need event similar to 'cfstatechange'. I did not think of this case before...
(In reply to Arthur Chen [:arthurcc] from comment #8)
> Indeed, we need event similar to 'cfstatechange'. I did not think of this
> case before...

Then, I will file another bug for this new event that blocks this gaia bug. Sounds good?
Depends on: 1000670
Whiteboard: [ETA: 4/25] → [ETA: 4/30][p=2]
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Jose, I've already modified the patch on the basis of the clir change event. I've tested on with tarako and it worked well. Will test on inari tomorrow. Could you help check if the patch makes sense to you? Thanks.
Attachment #8408107 - Flags: feedback?(josea.olivera)
Attachment #8414282 - Flags: feedback?(josea.olivera)
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

It works well on inari.
Attachment #8408107 - Flags: feedback?(josea.olivera) → review?(josea.olivera)
Attachment #8414282 - Flags: feedback?(josea.olivera) → review?(josea.olivera)
Why is "status-b2g-v1.3:affected" when this is only needed for 1.3T?
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Arthur, left a comment on the PR. Please, take a look. Thanks.
Attachment #8408107 - Flags: review?(josea.olivera)
Attachment #8414282 - Flags: review?(josea.olivera)
Arthur, please see my comment #13.
Flags: needinfo?(arthur.chen)
I simply marked all branches having this issue. We still do the uplifting based on the blocking flags and approvals.
Flags: needinfo?(arthur.chen)
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Jose, I've addressed your comments. Would you mind take a look at it again? Thanks.
Attachment #8408107 - Flags: review?(josea.olivera)
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Talked to Arthur on IRC. We agreed the solution for this bug. Arthur will update the patch. Thanks Arthur.
Attachment #8408107 - Flags: review?(josea.olivera)
Hi Arthur,
Looks we are getting close to complete this bug?
Just a reminder that it blocks Bug 997584, which is supposedly to fix this week.
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

I've updated the patch on the basis of our discussion. As for call.js, it is more than the one line change you suggested. 

The reason is that we add a handler on clirmodechange in the system app, and it triggers the query to the current clir mode. If we force query the clir mode from the carrier after the call to 'setCallingLineIdRestriction' no matter it fails or not, it may cause the query in the system fails.

Please let me know if that make sense to you. I'll update the unit tests later. Thanks!
Attachment #8408107 - Flags: review?(josea.olivera)
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Looking good! Please, update the test and request review at me again please. Thanks!
Attachment #8408107 - Flags: review?(josea.olivera) → feedback+
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Patch updated based on the comment. Could you check it again? I'll request a review for the patch against the v1.3t branch later. Thanks.
Attachment #8408107 - Flags: review?(josea.olivera)
Comment on attachment 8414282 [details]
PR for v1.3T

v1.3 PR updated.
Attachment #8414282 - Flags: review?(josea.olivera)
Whiteboard: [ETA: 4/30][p=2] → [ETA: 5/9][p=2]
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Very good job. Tested on both peak and fugu device. Everything seems to work fine and It seems there is no risk on taking the patch. Thanks Arthur.
Attachment #8408107 - Flags: review?(josea.olivera) → review+
Thanks, Jose!

master: 7427342276d63c44790b91c9767fe352c2d7a011
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8414282 [details]
PR for v1.3T

LGTM as well. Sadly I cannot test this patch as I don't have a Tarako device. Arthur did and everything seems to work fine as well. Thanks Arthur.
Attachment #8414282 - Flags: review?(josea.olivera) → review+
v1.3T: 554fd996205fef74d88e988f3596c58dcd05df28
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8408107 - Flags: approval-gaia-v1.4?(praghunath)
Comment on attachment 8414282 [details]
PR for v1.3T

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:
Attachment #8414282 - Flags: approval-gaia-v1.4?(praghunath)
Whiteboard: [ETA: 5/9][p=2] → [ETA: 5/9][p=2][approval-v1.4]
Whiteboard: [ETA: 5/9][p=2][approval-v1.4] → [ETA: 5/9][p=2][1.4-approval-needed]
Please fill in the response from Comment 29
Flags: needinfo?(itsay)
Hi Preeti,

Here is the update per your request.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): DSDS feature
[User impact] if declined: Partner blocker for Dolphin PTR test. Also happen in V1.4
[Testing completed]: verified in bug 995458
[Risk to taking this patch] (and alternatives if risky): Should be ok since QA already the test cases on this 
[String changes made]:No change
Flags: needinfo?(itsay)
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Taking into 1.4
Attachment #8408107 - Flags: approval-gaia-v1.4?(praghunath) → approval-gaia-v1.4+
Preeti, if you guys are taking the Gaia patch for 1.4 then I think you should also consider uplifting corresponding gecko bug.
Hi Arthur,

I we take this to v1.4, we also need to take patch in bug 1000670, correct?
Flags: needinfo?(arthur.chen)
Yes, we also need to uplift bug 100670.
Flags: needinfo?(arthur.chen)
Chris,

Can we please have the patch for bug 100670?
Flags: needinfo?(waterson)
(In reply to Preeti Raghunath(:Preeti) from comment #36)
> Chris,
> 
> Can we please have the patch for bug 1000670?
Attachment #8414282 - Flags: approval-gaia-v1.4?(praghunath) → approval-gaia-v1.4+
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

The risk analysis vs. value added here isn't really well presented here. I don't think a partner blocker is a compelling argument to take a patch into 1.4, since that risks impacting other partners doing device certification for 1.4 if we take this into the reference branch. If we don't take this but the partner wants this, then they should take this into their own custom 1.4 Gaia branch.

Clearing the approval flag - I don't think the argument presented here is compelling to take this patch.
Attachment #8408107 - Flags: approval-gaia-v1.4+
Attachment #8414282 - Flags: approval-gaia-v1.4+
(In reply to Jason Smith [:jsmith] from comment #38)
> Comment on attachment 8408107 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/18408
> 
> The risk analysis vs. value added here isn't really well presented here. I
> don't think a partner blocker is a compelling argument to take a patch into
> 1.4, since that risks impacting other partners doing device certification
> for 1.4 if we take this into the reference branch. If we don't take this but
> the partner wants this, then they should take this into their own custom 1.4
> Gaia branch.
> 
> Clearing the approval flag - I don't think the argument presented here is
> compelling to take this patch.

I'd like to give more update on the user impact if we don't take this one to v1.4. Here is the use case.
 
1. User set the preference to not show the caller ID (e.g. phone number) on receiver’s phone when calling.
2. User’s phone is rebooted
3. User makes a call but the caller ID will show up in receiver’s phone without being aware of it because the preference cannot be kept in settings.

I think this bug is critical because some users do attempt to hide the caller ID for some privacy purpose. However, this bug cause the calle ID to be turned on again after the phone reboot.

For this bug to be fixed completed, we also need bug 1000670, bug 997584, and bug 995458. 

I totally agree that we need to always be serious about the risk management to maintain the stability of our build. Thus, I suggest we can have QA to locally verify the patch in v1.4 before we uplift the patch. What do you think?
Flags: needinfo?(praghunath)
Flags: needinfo?(jsmith)
If this is really a critical use case for Dolphin, then the partner should take the patch on their own 1.4 branch, not the general 1.4 release. I understand the criticality of the use case, but I also think it's too late to take potential risky patches into the release at this time.
Flags: needinfo?(jsmith)
(In reply to Jason Smith [:jsmith] from comment #40)
> If this is really a critical use case for Dolphin, then the partner should
> take the patch on their own 1.4 branch, not the general 1.4 release. I
> understand the criticality of the use case, but I also think it's too late
> to take potential risky patches into the release at this time.

After talking with Clint - QA is in agreement that this is a bad bug for DSDS on 1.4, so we are okay with taking this patch.
Comment on attachment 8408107 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/18408

Per discussion with release management and QA, we will uplift this bug to v1.4. Resend the approval request.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): DSDS feature
[User impact] if declined: See user impact on comment 39
[Testing completed]: verified in bug 995458
[Risk to taking this patch] (and alternatives if risky): Low, since QA already the test cases on this 
[String changes made]:No change
Attachment #8408107 - Flags: approval-gaia-v1.4?(praghunath)
Attachment #8408107 - Flags: approval-gaia-v1.4?(praghunath) → approval-gaia-v1.4+
Flags: needinfo?(praghunath)
Whiteboard: [ETA: 5/9][p=2][1.4-approval-needed]
Hi Arthur,

Per discussion with release management, we want to back out this patch from v1.4 for now. Since Ryan is on PTO, I wonder if you can help to back out the patch. Thanks.
Flags: needinfo?(arthur.chen)
backed out from v1.4: 379ac0e17cd5915e48a0c3e4af3426f56a43c15a
Flags: needinfo?(arthur.chen)
Comment on attachment 8440539 [details]
PR for v1.4

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): DSDS feature
[User impact] if declined: See user impact on comment 39
[Testing completed]: verified in bug 995458
[Risk to taking this patch] (and alternatives if risky): Low, since QA already the test cases on this 
[String changes made]:No change
Attachment #8440539 - Flags: approval-gaia-v1.4?(praghunath)
(In reply to Ivan Tsay (:ITsay) from comment #47)
> Comment on attachment 8440539 [details]
> PR for v1.4
> 
> NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to
> better understand the B2G approval process and landings.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): DSDS feature
> [User impact] if declined: See user impact on comment 39
> [Testing completed]: verified in bug 995458
> [Risk to taking this patch] (and alternatives if risky): Low, since QA
> already the test cases on this 
> [String changes made]:No change
Please make sure this only goes in 1.4 dolphin branch.
Flags: needinfo?(praghunath)
Anshul,

Is there a reason you prefer this code not be uplifted into 1.4 branch? Why should this only go to the Dolphin branch?
Flags: needinfo?(praghunath) → needinfo?(anshulj)
Flags: needinfo?(anshulj)
Attachment #8440539 - Flags: approval-gaia-v1.4?(praghunath) → approval-gaia-v1.4+
1.4: a054aa221df20ca550e6c67f3fbb20d0ad086598
Flags: needinfo?(waterson)
You need to log in before you can comment on or make changes to this bug.