Closed Bug 1000670 Opened 7 years ago Closed 7 years ago

[B2G] [RIL] expose "clirmodechange" event to gaia

Categories

(Firefox OS Graveyard :: RIL, defect)

x86_64
Linux
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: hsinyi, Assigned: hsinyi)

References

Details

(Whiteboard: [p=2])

Attachments

(15 files, 5 obsolete files)

1.80 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
6.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.96 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
3.44 KB, patch
airpingu
: review+
smaug
: review+
Details | Diff | Splinter Review
827 bytes, patch
echou
: review+
Details | Diff | Splinter Review
3.38 KB, patch
Details | Diff | Splinter Review
1.81 KB, patch
Details | Diff | Splinter Review
6.11 KB, patch
Details | Diff | Splinter Review
4.21 KB, patch
Details | Diff | Splinter Review
873 bytes, patch
Details | Diff | Splinter Review
3.52 KB, patch
Details | Diff | Splinter Review
1.89 KB, patch
Details | Diff | Splinter Review
6.51 KB, patch
Details | Diff | Splinter Review
4.05 KB, patch
Details | Diff | Splinter Review
924 bytes, patch
Details | Diff | Splinter Review
Bug 997601 and bug 997584 are going to let Gaia/Settings APP take care of store user preference for calling line id restriction.

In consideration of the case user changes the status via MMI code, gecko should notify gaia so that gaia knows that she needs to reset user preference accordingly.
nominate 1.3T since it blocks a 1.3T bug 997601.
blocking-b2g: --- → 1.3T?
I am going to add a new event in this bug and keep the gecko clear work, removing preference, in bug 997584. In this way, we are safe to land each piece to not break the whole feature.
Assignee: nobody → htsai
(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #1)
> nominate 1.3T since it blocks a 1.3T bug 997601.
It is.
blocking-b2g: 1.3T? → 1.3T+
Proposal:

partial interface nsIDOMMozMobileConnection {
  [implicit_jscontext] attribute jsval onclirmodechange;
};

interface CLIRModeChangeEvent : Event
{
  readonly unsigned long mode;
}

This event is dispatched when user sets clir successfully, either by Settings menu or by MMI code.

Look good to gaia, Arthur?
Flags: needinfo?(arthur.chen)
Looks good to me!
Flags: needinfo?(arthur.chen)
Whiteboard: [p=2]
Target Milestone: --- → 1.4 S6 (25apr)
Whiteboard: [p=2] → [p=2][ETA: 4/25]
Attached patch part 1 - webidl (obsolete) — Splinter Review
Add a new event onclirmodechange
nsIMobileConnectionProvider.notifyClirModeChanged()
Attached patch part 4 - ril (obsolete) — Splinter Review
Attached patch part 5 - BT (obsolete) — Splinter Review
Attached patch part 1 - webidl (2) (obsolete) — Splinter Review
Add a new event onclirmodechange 

Change since v1: Corrected moz.build
Attachment #8411688 - Attachment is obsolete: true
Change since v1: removed changes on moz.build in this part
Attached patch part 4 - ril (2)Splinter Review
Change since v1: corrected typo
Attachment #8411691 - Attachment is obsolete: true
Attachment #8411692 - Attachment is obsolete: true
Comment on attachment 8411711 [details] [diff] [review]
part 1 - webidl (2)

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

Hi Gene and Olli,

This patch is going to add a new event to notify gaia when CLIR mode changes. May I have your reviews? Thank you.
Attachment #8411711 - Flags: review?(gene.lian)
Attachment #8411711 - Flags: review?(bugs)
Summary: [B2G] [RIL] expose "clirstatuschange" event to gaia → [B2G] [RIL] expose "clirmodechange" event to gaia
Attachment #8411690 - Flags: review?(vyang)
Attachment #8411713 - Flags: review?(bugs)
Comment on attachment 8411714 [details] [diff] [review]
part 4 - ril (2)

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

Hi Vicamo,

I am going to add a new event in this bug and keep the gecko clear work, removing preference, in bug 997584. In this way, we are safe to land each piece to not break the whole feature. Please help review this patch, and I am going to revise bug 997584 based on this one.
Attachment #8411714 - Flags: review?(vyang)
Comment on attachment 8411711 [details] [diff] [review]
part 1 - webidl (2)

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

Oops... this is the wrong file.
Attachment #8411711 - Flags: review?(gene.lian)
Attachment #8411711 - Flags: review?(bugs)
Add a new event onclirmodechange 

Change since v2: fix missing ;
Attachment #8411711 - Attachment is obsolete: true
Attached patch part 5 - BT (2)Splinter Review
Change since v1: removed unrelated parts
Attachment #8411693 - Attachment is obsolete: true
Comment on attachment 8411722 [details] [diff] [review]
part 1 - webidl (3)

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

Hi Gene and Olli,

This patch is going to add a new event to notify gaia when CLIR mode changes. May I have your reviews? Thank you.
Attachment #8411722 - Flags: review?(gene.lian)
Attachment #8411722 - Flags: review?(bugs)
Comment on attachment 8411736 [details] [diff] [review]
part 5 - BT (2)

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

Hi Eric,

We add a new event listener, notifyClirModeChanged. Please help review the BT part, thank you!
Attachment #8411736 - Flags: review?(echou)
Comment on attachment 8411722 [details] [diff] [review]
part 1 - webidl (3)

We should get these interface somewhat spec'ed and remove moz-prefix.
But given that rest of this API is moz-prefixed, I think the event interface
can be too.

Should the comment be
'Indicates the mode of the calling line id restriction (CLIR).'
Attachment #8411722 - Flags: review?(bugs) → review+
Attachment #8411713 - Flags: review?(bugs) → review+
Comment on attachment 8411714 [details] [diff] [review]
part 4 - ril (2)

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

Thank you :)
Attachment #8411714 - Flags: review?(vyang) → review+
Attachment #8411690 - Flags: review?(vyang) → review+
Attachment #8411736 - Flags: review?(echou) → review+
Whiteboard: [p=2][ETA: 4/25] → [p=2][ETA: 4/29]
Thanks for the review, Olli.

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8411722 [details] [diff] [review]
> part 1 - webidl (3)
> 
> We should get these interface somewhat spec'ed and remove moz-prefix.
> But given that rest of this API is moz-prefixed, I think the event interface
> can be too.
> 

Yup, this is the reason I have it moz-prefixed.

> Should the comment be
> 'Indicates the mode of the calling line id restriction (CLIR).'

Will correct it in the next version.
Attachment #8411722 - Flags: review?(gene.lian) → review+
Hsin-Yi, please rebase and land on 1.3t. thanks!
Flags: needinfo?(htsai)
(In reply to Fabrice Desré [:fabrice] from comment #27)
> Hsin-Yi, please rebase and land on 1.3t. thanks!

I am working on that. 1.3t code base differs quite a lot. To be safe, I will attach 1.3t patches after try on v1.3t is good.
Attached patch v1.3T - part 1Splinter Review
Flags: needinfo?(htsai)
Hsin-Yi, will this change be uplifted to 1.4 as well?
Flags: needinfo?(htsai)
(In reply to Anshul from comment #36)
> Hsin-Yi, will this change be uplifted to 1.4 as well?

Hi Anshul,

This bug was originally coming from bug 995458 and bug 997584, which is a shortcoming in mozRIL, and not sure if QCRIL has the same issue. If you need this fix, please set the 1.4? flag and let PM triage. Thanks!
Flags: needinfo?(htsai)
Whiteboard: [p=2][ETA: 4/29] → [p=2][ETA: 4/29][1.4-approval-needed]
ni? owner to prepare v1.4 patch. I will ask for v1.4 uplift approval once the v1.4 patch is available.
Flags: needinfo?(htsai)
Attached patch v1.4 - part 1Splinter Review
Flags: needinfo?(htsai)
Ivan, I've provided patches for 1.4 branch, v1.4 - part 1 ~ v1.4 - part 5. Thanks!
Flags: needinfo?(itsay)
Comment on attachment 8439108 [details] [diff] [review]
v1.4 - part 1

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439108 - Flags: approval-mozilla-b2g30?
Flags: needinfo?(itsay)
Comment on attachment 8439109 [details] [diff] [review]
v1.4 - part 2

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439109 - Flags: approval-mozilla-b2g30?
Comment on attachment 8439110 [details] [diff] [review]
v1.4 - part 3

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439110 - Flags: approval-mozilla-b2g30?
Comment on attachment 8439111 [details] [diff] [review]
v1.4 - part 4

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439111 - Flags: approval-mozilla-b2g30?
Comment on attachment 8439112 [details] [diff] [review]
v1.4 - part 5

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439112 - Flags: approval-mozilla-b2g30?
(In reply to Ivan Tsay (:ITsay) from comment #45)
> Comment on attachment 8439108 [details] [diff] [review]
> v1.4 - part 1
> 
> NOTE: This flag is now for security issues only. 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 in v1.4
> User impact if declined: please refer to
> https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
> Testing completed: yes in bug 995458
> Risk to taking this patch (and alternatives if risky): Low. 
> String or UUID changes made by this patch: no change

I'd like to provide additional remark --
Actually |String or UUID changes made by this patch| should be *yes*.

We add a new interface in this bug. However, the code path is quite isolated as it's a new event, so the risk is still low IMO.
Comment on attachment 8439108 [details] [diff] [review]
v1.4 - part 1

needs to land on Dolphin branch
Attachment #8439108 - Flags: approval-mozilla-b2g30?
Whiteboard: [p=2][ETA: 4/29][1.4-approval-needed] → [p=2][ETA: 4/29][1.4-approval-needed][dolphin land]
Comment on attachment 8439108 [details] [diff] [review]
v1.4 - part 1

NOTE: This flag is now for security issues only. 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 in v1.4
User impact if declined: please refer to https://bugzilla.mozilla.org/show_bug.cgi?id=997601#c39
Testing completed: yes in bug 995458
Risk to taking this patch (and alternatives if risky): Low. 
String or UUID changes made by this patch: no change
Attachment #8439108 - Flags: approval-mozilla-b2g30?
Comment on attachment 8439108 [details] [diff] [review]
v1.4 - part 1

Approved for 1.4.
Attachment #8439108 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8439109 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8439110 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8439111 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
Attachment #8439112 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g30+
You need to log in before you can comment on or make changes to this bug.