On v2.1, Bug 1055994 fires a system message when a CB message is received in addition to the existing call of gMessageManager.sendCellBroadcastMessage(). Because of this, I now see two pop ups for one cell broadcast message (see video attached). Also, Bug 1051795 will only take care of blocking the display of CB messages for the system message. So even with 'Emergency Alert' turned off, we'll still see CB messages. My questions are: 1) Why do both messages need to be fired? What's the meaning of firing each? 2) Is the current behavior the desired behavior?

NI Bevis, to help answer Carol's questions here.

Or Steve.

(In reply to Carol Yang [:cyang] from comment #0) > Created attachment 8514418 [details] > IMG_3471.mov > > On v2.1, Bug 1055994 fires a system message when a CB message is received in > addition to the existing call of gMessageManager.sendCellBroadcastMessage(). > Because of this, I now see two pop ups for one cell broadcast message (see > video attached). > > Also, Bug 1051795 will only take care of blocking the display of CB messages > for the system message. So even with 'Emergency Alert' turned off, we'll > still see CB messages. > > My questions are: > 1) Why do both messages need to be fired? What's the meaning of firing each? The purpose is to launch the app that is interested in this event. > 2) Is the current behavior the desired behavior? Apps have to filter out what channel is desired to be displayed. For Question 2), we need Steve's help to comment in advance.

(In reply to Carol Yang [:cyang] from comment #0) > Created attachment 8514418 [details] > IMG_3471.mov > > > My questions are: > 1) Why do both messages need to be fired? What's the meaning of firing each? Since the CMAS message is controlled in app now, we will need system message to launch the app first, otherwise we could not handle message with app closed. > 2) Is the current behavior the desired behavior? No, if the Emergency Alert turned off, we should not see this alert dialog. My concern is we also have cell-broadcast management in system before CMAS app landed, so it's possible to get Emergency Alert in both side your testing mobile operator is in Brazil. Please note Emergency Alert is a subset of cell-broadcast, so even you turn off Emergency Alert, you could still get cell-broadcast callback in system. Hi Carol, could you please help verify that testing with non-Emergency-alert cell-broadcast message, and see if you could see the popup dialog without Emergency alert title?

But are we

But are we correctly filtering out CMAS alerts in the System app? Bug 1051795 should have done it but I don't see this in the patch. I think we should always filtering out CMAS alerts in the System app, with CMAS enabled or disabled.

Blocking requested: broken feature.

(In reply to Steve Chung [:steveck] from comment #4) > (In reply to Carol Yang [:cyang] from comment #0) > > Created attachment 8514418 [details] > > IMG_3471.mov > > > > > > My questions are: > > 1) Why do both messages need to be fired? What's the meaning of firing each? > Since the CMAS message is controlled in app now, we will need system message > to launch the app first, otherwise we could not handle message with app > closed. > > 2) Is the current behavior the desired behavior? > No, if the Emergency Alert turned off, we should not see this alert dialog. > My concern is we also have cell-broadcast management in system before CMAS > app landed, so it's possible to get Emergency Alert in both side your > testing mobile operator is in Brazil. Please note Emergency Alert is a > subset of cell-broadcast, so even you turn off Emergency Alert, you could > still get cell-broadcast callback in system. > > Hi Carol, could you please help verify that testing with non-Emergency-alert > cell-broadcast message, and see if you could see the popup dialog without > Emergency alert title? Yes, if I sent a non-Emergency-alert CB message, I only see one popup (the one without the 'Emergency Alert' title). So it seems like the 'cell-broadcast management in system' that you mentioned above is only meant for non-Emergency alerts? If yes, was just curious what the reasoning was to have two different kinds of handling, i.e. why have a specific 'Emergency Alert' popup, why not just use the existing popup for both? Also, as I mentioned in bug 1091960, I am wondering if it would make less sense for this filtering to happen at Gaia level. Rather, the filtering should be done down at RIL?

Steven, it seems that you will take this bug, right?

(In reply to Carol Yang [:cyang] from comment #8) > > Yes, if I sent a non-Emergency-alert CB message, I only see one popup (the > one without the 'Emergency Alert' title). > > So it seems like the 'cell-broadcast management in system' that you > mentioned above is only meant for non-Emergency alerts? If yes, was just > curious what the reasoning was to have two different kinds of handling, i.e. > why have a specific 'Emergency Alert' popup, why not just use the existing > popup for both? > > Also, as I mentioned in bug 1091960, I am wondering if it would make less > sense for this filtering to happen at Gaia level. Rather, the filtering > should be done down at RIL? Thanks for the clarification, the original popup dialog in system is just a normal dialog for showing CB message only. But in Emergency Alert use case, we will need to cache the message in notification tray instead of dismiss right away. So we decide to create a new app to control Emergency Alert for more flexibility and less limitation than dialog. We might control both message in Network Alerts app in the future, in order to solve ambiguous responsibility like this case. I don't think it's necessary to filtering CMAS or CB in RIL level, since CMAS is still a branch of CB, and it's easy to classify the whether the message is CMAS or not.

Hi Carol, here is a WIP which simply ignore the CMAS in system CB message. Could you please help verify if the issue is addressed after patch applied? thanks.

Comment on attachment 8516608 [details] [diff] [review] Bug-1091751.patch Hi Steve, With your patch, I'm only seeing the Emergency Alert popup for CMAS messages. And likewise, only see the system CB message popup for a normal CB message. Thanks for your explanation! That was helpful :) I was curious how you felt about having the new Network Alerts app just control both types of messages (CMAS and non-CMAS) now? Seems like handling all messages from one app would be the cleanest way to handle this.

I think it's a proper long term goal but it was too much work given the time frame.

(In reply to Julien Wajsberg [:julienw] from comment #13) > I think it's a proper long term goal but it was too much work given the time > frame. If that's the case, this patch alone would break our existing functionality and we would rather not have this go into v2.1. Is there already a bug that would track this long term goal to handle both messages in Network Alerts app?

> If that's the case, this patch alone would break our existing functionality and we would rather not have this go into v2.1. I don't understand this sentence, Carol? > With your patch, I'm only seeing the Emergency Alert popup for CMAS messages. And > likewise, only see the system CB message popup for a normal CB message. Isn't it the expected behavior? No bug that I know, makes sense to file one.

Julien, this patch breaks CDMA CMAS functionality as Gaia is being too restrictive with the allowed range. We will file a new bug outline the limitations of the current design and ways to address them. Carol could you please file a new bug?

Filed bug 1093922 to track moving CB message handling to Network Alerts app.

Per discussion with Julien, we both agreed that we should handle both within Network Alerts (In reply to Anshul from comment #16) > Julien, this patch breaks CDMA CMAS functionality as Gaia is being too > restrictive with the allowed range. We will file a new bug outline the > limitations of the current design and ways to address them. > > Carol could you please file a new bug? Anshul, since we didn't claim we support CMAS on CDMA, if you mean we need to handle CDMA CMAS properly, we will need to create another bug for this.

Anshul, Carol, I still think we should uplift this to v2.1 because this is broken functionality in my opinion. We see 2 alerts for CMAS messages which is wrong. Is there any way we can make this patch works for you? Like not filtering the CDMA CMAS messages from the system app for now?

Hi Kevin, this patch forces cell broadcast system ignore CMAS message. I think you're the appropriate candidate for reviewing this changes since you created this subsystem.

Comment on attachment 8517365 [details] Link to github I'm not too familiar with how this all works, but the code seems fine to me. I think a better strategy might be to have a single place where we handle these alerts, and dispatch it - either in the platform or system app. I think this is asking for trouble down the road. R+ for the patch though, seems simple enough.

(In reply to Julien Wajsberg [:julienw] from comment #19) > Anshul, Carol, I still think we should uplift this to v2.1 because this is > broken functionality in my opinion. We see 2 alerts for CMAS messages which > is wrong. > > Is there any way we can make this patch works for you? Like not filtering > the CDMA CMAS messages from the system app for now? Julien, adding filtering for CDMA CMAS is not trivial as the format of CDMA CMAS is different than GSM CMAS in some respects. We currently have found a tiny workaround in our code to avoid this issue and I feel that it is best to not include this patch and the follow up patch for CDMA CMAS you were suggesting to avoid possible fallouts from those patches. Once we have our own branch, hopefully soon, please feel free to uplift this patch to non QC specific 2.1 branch.

Thanks for the review, I added more comments about the changes. In master : db9c35438be5bc1ebe79ec3ef103f7e17c339e71 BTW, we created Bug 1094589 for addressing the CMAS on CDMA, so we could discuss any possible solution for CDMA in this bug.

Comment on attachment 8517365 [details] Link to github [Approval Request Comment] [Bug caused by] (feature/regressing bug #): CMAS feature bug 1026685 [User impact] if declined: Duplicated popup dialog for cell broadcast message [Testing completed]: Yes [Risk to taking this patch] (and alternatives if risky): Low [String changes made]: N/A

According to comment 22, adding NO_UPLIFT until we have a dedicated branch for CA. Anshul, do you know when this happens?

(In reply to Julien Wajsberg [:julienw] from comment #25) > According to comment 22, adding NO_UPLIFT until we have a dedicated branch > for CA. > > Anshul, do you know when this happens? That's not a CAF decision, NI KKuo who was planning on this branch. Julien, the dates/process of creation are not confirmed yet and hoping Keven will send out more details on that.

Agree with Julien, adding NO_UPLIFT until we have a dedicated branch for CA. Branching is under process still. Will make an announcement once the branch is created. -- Keven

Comment on attachment 8517365 [details] Link to github Approving the change but please do not land until proper branching is done by :kkuo's team. Kevin, how are you planning to track these issues, there are a couple of other bugs in the same state and not sure what works best for you folks.

Device team is waiting for CAF's decision, the revision number that CAF wants us to branch. -- Keven

Any updates on this?

We don't want this patch if bug 1093922 isn't going to be fixed in 2.2 as it breaks our existing CMAS functionality.

Anshul, can you care to explain why this breaks your existing CMAS functionality? Or is it CMAS on CDMA ?

Julien, yes it is CMAS on CDMA that will be broken due to this change.

So it means that we need to fix bug 1094589 or bug 1093922 in v2.2, otherwise we'll have the issue in that version.

You are right Julien.

Wilfred, please look at the previous comments, I think we need to prioritize bug 1094589 (that could be fixed by bug 1093922) in v2.2.

(In reply to Julien Wajsberg [:julienw] from comment #36) > Wilfred, please look at the previous comments, I think we need to prioritize > bug 1094589 (that could be fixed by bug 1093922) in v2.2. wilfred, can you help with this one ? we either need to bug 1093922 in 2.2 or may be backout the current patch and maintain the status kuo ? Steve, can you comment on how feasible risk wise it is to back this out if we have to and would you think that's the best path forward?

Hi bhavana, the result of backing out this patch would cause the GSM CMAS notification popup twice with different types. It brings bad use experience but it won't break the GSM CMAS functionality. Instead of implementing full support of CDMA CMAS or bug 1093922 to control all the CB in network alert, we can make a quick fix to make CDMA CB message available again. The reason why this patch block the CDMA CB is because the CMAS checking in CDMA and GSM is different, and we can not apply the GSM checking logic to CDMA. If we classify the type of CB message first and ignore the CDMA case, CDMA CB should be displyed correctly.

Hi Anshul, could you please verify if CDMA case is fixed after applying this patch? Thanks.

Comment on attachment 8538957 [details] [diff] [review] Ignore-CDMA-checking.patch Carol, could you please test the patch provided by Steve and provide your feedback.

Comment on attachment 8538957 [details] [diff] [review] Ignore-CDMA-checking.patch Review of attachment 8538957 [details] [diff] [review]: ----------------------------------------------------------------- Hi Steve, sorry for the delayed response. I had been on vacation and just got all caught up. ::: apps/system/js/cell_broadcast_system.js @@ +78,5 @@ > // Message id from range 4370 to 4399(1112 hex to 112f hex) should be CMAS > // message and network alert will display detail information. > + // This CMAS checking is only available for GSM, if the CB message is > + // CDMA, we will address this in bug 1113466. > + if (!cdmaCategory && id >= 4370 && id < 4400) { I'm a little curious why not let Network Alert app handle all CMAS including CDMA. Basically return early here if cdmaCategory and then in Network Alerts app, do this if !cdmaCategory check. At least this way, there will be a separation of cell_broadcast_system.js handling all the non-CMAS messages and Network Alert app handling both GSM and CDMA CMAS messages. What do you think here?

(In reply to Carol Yang [:cyang] from comment #41) > Comment on attachment 8538957 [details] [diff] [review] > Ignore-CDMA-checking.patch > > Review of attachment 8538957 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hi Steve, sorry for the delayed response. I had been on vacation and just > got all caught up. > > ::: apps/system/js/cell_broadcast_system.js > @@ +78,5 @@ > > // Message id from range 4370 to 4399(1112 hex to 112f hex) should be CMAS > > // message and network alert will display detail information. > > + // This CMAS checking is only available for GSM, if the CB message is > > + // CDMA, we will address this in bug 1113466. > > + if (!cdmaCategory && id >= 4370 && id < 4400) { > > I'm a little curious why not let Network Alert app handle all CMAS including > CDMA. Basically return early here if cdmaCategory and then in Network Alerts > app, do this if !cdmaCategory check. At least this way, there will be a > separation of cell_broadcast_system.js handling all the non-CMAS messages > and Network Alert app handling both GSM and CDMA CMAS messages. What do you > think here? We can definitely check CDMA CMAS in Network Alert app, but I think it would be clear to track it in bug 1113466. The purpose of this patch is only to fix the regression from landed patch attachment 8517365 [details] that causes the CDMA cell broadcast not working any more. BTW please use needinfo flag for the reply, otherwise it's easy to lose track :)

(In reply to Steve Chung [:steveck] from comment #42) > We can definitely check CDMA CMAS in Network Alert app, but I think it would > be clear to track it in bug 1113466. The purpose of this patch is only to > fix the regression from landed patch attachment 8517365 [details] that > causes the CDMA cell broadcast not working any more. BTW please use needinfo > flag for the reply, otherwise it's easy to lose track :) If that's the case, then this patch does what it needs to do (fix regression for CDMA CB) and works fine for me.

Is this wontfix for v2.1?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #44) > Is this wontfix for v2.1? yes.