[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

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.

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 on attachment 8492901 [details] 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 :)

Blocking as this looks like a fallout from 1001285

Comment on attachment 8492901 [details] 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.

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 !

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

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

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 !

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

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 !

(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.

(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 !

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 !

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

(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 on attachment 8492901 [details] patch on master Hi Alive, can you help me review this patch (with tests included), thanks :)

Comment on attachment 8494214 [details] 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 !

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

Comment on attachment 8494214 [details] 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

EJ, did this land on master yet?

(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.

(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.

Thanks for reminding, Fabrice.

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