1068978 - [CBS] Migrate all ril.cellbroadcast.disabled setting to multi client format

Status: RESOLVED FIXED

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

Description

[Michael Schwartz [:m4]]

[Blocking Requested - why for this release]:

https://bugzilla.mozilla.org/show_bug.cgi?id=1001285 swapped the ril.cellbroadcast.disabled setting to be an array [1].  This breaks the usage of that setting [2].

[1] http://mxr.mozilla.org/gaia/source/build/config/common-settings.json#180
[2] http://mxr.mozilla.org/gaia/source/apps/system/js/cell_broadcast_system.js#29

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi Vicamo,

is there anyway that Gaia can understand where does the CellBroadCast message coming from ? Because in order to support DSDS devices, users can decide which card to receive CBS message. 

Any idea would be appreciated. Thanks.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Hi,

Navigator API entry:
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozCellBroadcast.webidl
Event type:
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MozCellBroadcastEvent.webidl
And message type: (going to be moved to WebIDL in bug 864484)
http://mxr.mozilla.org/mozilla-central/source/dom/cellbroadcast/interfaces/nsIDOMMozCellBroadcastMessage.idl
It has an 'serviceId' attribute so that one may check if this message is expected.

Comment 3

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8492901 [details] [review]
patch on master

Hi Michael, 

because I can't send CBS message by myself, do you recommend any way to do so or can you help me test this patch ?

Any idea would be appreciated ! Thanks a lot :)

Comment 5

[bhavana bajaj [:bajaj]]

Blocking as this looks like a fallout from 1001285

Comment 6

[Michael Schwartz [:m4]]

Comment on attachment 8492901 [details] [review]
patch on master

The UI should not test the setting value but should instead trust the lower layers to only send messages that have been enabled.

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hi @Vicamo,

from Gaia, if we changed the value of `ril.cellbroadcast.disabled`, Gecko would just directly discard any incoming messages if the pref is disabled, right ?

@Michael,

If I understand you correctly, your mainly concern would be https://github.com/EragonJ/gaia/blob/bug-1068978/apps/system/js/cell_broadcast_system.js#L73-L75 , right ? If Gecko does discard any incoming messages with pref disabled, I this we can directly take off these lines.

And by the way, does this patch work there ? 

Thanks all !

Comment 8

[Michael Schwartz [:m4]]

Hi,

This change doesn't apply cleanly on Gaia v2.1 so I can't verify it.  Since it's blocking on that branch anyway, would you please prepare a corresponding patch?

Yes, you should remove all of the settings code in that file as is duplicates the logic provided for you in the lower layers and will therefore reduce the maintenance burden of your UI code.

Thx,

M

Comment 9

[Michael Schwartz [:m4]]

Note that 2.1 suffers from another issue but I can verify despite that in a pinch https://bugzilla.mozilla.org/show_bug.cgi?id=1068860

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on v2.1

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Michael,

I added another separate patch for v2.1, and you can now verify them on master & v2.1

Hope this helps and wait for your good news !

Thanks !

Comment 12

[Michael Schwartz [:m4]]

Hi,

By checking the setting first, you actually break cell broadcast functionality as there are some ranges which cannot be disabled ie Presidential Alert see 3GPP 23.041 section 9.4.1.2.2 Message Identifier.  I'll test it once the patch is cleaned up.

Thanks,

M

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Michael,

after asking HsinYi about your comment, it seems you are talking about another bug 1008501 that no one has a final conclusion and there are still rooms to discuss.

For this bug, all we did is to make sure we can enable/disable CBS in DSDS devices. So, checking from code, there is no any functionalities got changed in this patch. What Gaia did is to provide another interface to make sure users can enable/disable CBS under different simcards.

So, for functionality part which you mentioned on comment 12, please go discuss on bug 1008501 instead of this one. (And also, this is not what we want to fix in this patch.)

If anything looks fine, I'll go ask for reviewing this patch.

Thanks !

Comment 14

[Michael Schwartz [:m4]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #13)
> it seems you are talking about
> another bug 1008501 that no one has a final conclusion and there are still
> rooms to discuss.

That's a different issue.  As stated above the settings check here is a duplicating the functionality of that provided by the lower layers.  Does SMS check to ensure Messaging is enabled before receiving messages?  Does telephony check that APM is not enabled before receiving a call?  Remove the check and we're good.  I don't understand why you're pushing back here.  The additional code is a maintenance burden that is not required and caused this issue to begin with.

Comment 15

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Michael Schwartz [:m4] from comment #12)
> Hi,
> 
> By checking the setting first, you actually break cell broadcast
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~    
> functionality as there are some ranges which cannot be disabled ie
  ~~~~~~~~~~~~~

So on this comment, you focused on the functionality instead of mozSettings problems.

(In reply to Michael Schwartz [:m4] from comment #14)
> (In reply to EJ Chen [:eragonj][:小龍哥] from comment #13)
> > it seems you are talking about
> > another bug 1008501 that no one has a final conclusion and there are still
> > rooms to discuss.
> 
> That's a different issue.  As stated above the settings check here is a
                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> duplicating the functionality of that provided by the lower layers.  Does
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

On this comment, you were talking about the mozSettings problem instead of functionality you mentioned on comment 12.

--
Based on these two comments, I don't think you are talking about the same thing ...
--

To clarify things easily, can you directly point out which part of code that you mainly concern about ? If I didn't get you wrong, I thought you were talking about this code : https://github.com/EragonJ/gaia/commit/ca7e549fcb265b4d8194323c0ab5297b639245c3#diff-c5ad41a1641005ca1986003bbac34be7L77 and I am sure they got removed from the patch already.

If not, which part of codes are problematic ? 

Any ideas / suggestions would be appreciated. Thanks !

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

And for https://github.com/EragonJ/gaia/commit/ca7e549fcb265b4d8194323c0ab5297b639245c3#diff-c5ad41a1641005ca1986003bbac34be7R58 , we have to understand the change of settings to inform LockScreen to show related information on it. 

Hope this makes things clearer !

Comment 17

[Michael Schwartz [:m4]]

Hi,

Yes it does make things clearer - thank you.  Do we have a requirement to clear the lockscreen message when the settings are disabled?  Seems like letting it go on its own would be sufficient.

Anyway, I have verified your patch on 2.1. Thx,

Michael

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to Michael Schwartz [:m4] from comment #17)
> Hi,
> 
> Yes it does make things clearer - thank you.  Do we have a requirement to
> clear the lockscreen message when the settings are disabled?  Seems like
> letting it go on its own would be sufficient.

This is made by design and I would ask Alive's review to let him decide this part. 

> 
> Anyway, I have verified your patch on 2.1. Thx,
> 
> Michael

Great, thanks a lot !

Comment 19

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8492901 [details] [review]
patch on master

Hi Alive, can you help me review this patch (with tests included), thanks :)

Comment 20

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8494214 [details] [review]
patch on v2.1

Because there are some conflicts between master & 2.1, I made another patch for it and basically the codes are the same with only some tweaks.

Alive, please help me check this patch too, thanks a lot !

Comment 21

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Based on comment 17, we are good to review codes now, thanks Alive !

Comment 22

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8494214 [details] [review]
patch on v2.1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1001285
[User impact] if declined: Users would not be able to receive CBS messages.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: no

Comment 23

[bhavana bajaj [:bajaj]]

EJ, did this land on master yet?

Comment 24

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

(In reply to bhavana bajaj [:bajaj] from comment #23)
> EJ, did this land on master yet?

I just merged to master at https://github.com/mozilla-b2g/gaia/commit/1a1ab35aed2f1f540111abfeed1ccdc8603a8e08 just now because Tree was closed and all PR got closed at the same time, so I had to wait for CI again last night.

Comment 25

[[:fabrice] Fabrice Desré]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #24)
> (In reply to bhavana bajaj [:bajaj] from comment #23)
> > EJ, did this land on master yet?
> 
> I just merged to master at
> https://github.com/mozilla-b2g/gaia/commit/
> 1a1ab35aed2f1f540111abfeed1ccdc8603a8e08 just now because Tree was closed
> and all PR got closed at the same time, so I had to wait for CI again last
> night.

Please mark bugs as closed when landing on master.

Comment 26

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks for reminding, Fabrice.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/8a97e5167151268db0db0316575fdda0b05684ec