910091 - Broadcast SMS on CDMA(can't get Service Category)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[kenzo Ohta]

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.57 Safari/537.36

Comment 1

[kenzo Ohta]

As a user I want to be notified about incoming CDMA Broadcast messages.
Now Cell Broadcast on GSM is implementing.
I think Broadcast SMS on CDMA should be implemented so.

Comment 2

[kenzo Ohta]

Attachment: BroadcastSMS

I have challenged to writte a patch.
<<BroadcastSMS>>
I want to check.
Could someone follow me.

----------
My implementation is broadcastSMS(CDMA's cellbroadcast).
BroadcastSMS(CDMA's cellbroadcast) is separate from GSM cellbroadcast.
These are similer feature but different feature, 
because these are different communication standards, methods, and data.
So my implementaion is separate from cellbroadcast and copy design from cellbroadcast.

Comment 3

[Julien Wajsberg [:julienw]]

This is for the Mobile Message API team.

Comment 4

[Gene Lian (I already quit Mozilla)]

Vicamo is in charge of the cell broadcast. He can support you to work it out.

Comment 5

[Gene Lian (I already quit Mozilla)]

Hi kenzo Ohta, please change feedback? to review? if you think your patch is actually ready to review. Thanks!

Comment 6

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

(In reply to Julien Wajsberg [:julienw] from comment #3)
> This is for the Mobile Message API team.

We have finally our own component in B2G::RIL.  Thank you :)

Comment 7

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

Comment on attachment 807110 [details] [diff] [review]
BroadcastSMS

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

Kenzo, thank you for the outstanding work for bringing CDMA broadcast SMS into Firefox OS, but please just reuse current CellBroadcast component, add CDMA specific attributes to MozCellBroadcastMessage, and have most of the implementation in ril_worker only.

Comment 8

[Ken Chang[:kenkai]]

Kenzo, it seems what you want have been implemented in bug 890819.

Comment 9

[kenzo Ohta]

(In reply to Ken Chang from comment #8)
> Kenzo, it seems what you want have been implemented in bug 890819.

Thank you for your comment.

Now I'm checking the bug 869776 about relation of our implementation.

Comment 10

[na-Kobayashi]

Attachment: BroadcastFlow1.JPG

Dear Ken Chang

Kenzo is a colleague of mine from the office.

I checked the bug 869776. 
bug 869776 and bug 910091 are different.
Bug 910091 point is 'Application can't receive "service category"'.
Service Category parameter is important for Broadcast SMS of CDMA.

I have a question. 
Are "Cell Broadcast for GSM" and "Broadcast SMS for CDMA" same sequence of broadcast?
For details, please refer to the attached.
I assumed Flow 1 for Broadcast SMS of CDMA.
------
 Flow 1 : 
  "SMS for CDMA" and "Broadcast SMS for CDMA" are same sequence.

 Flow 2 : 
  "Cell Broadcast for GSM" and "Broadcast SMS for CDMA" are same sequence.
------
Which is correct?

If "Flow 2" is correct, How does application receive "service category"?

Comment 11

[na-Kobayashi]

Attachment: BroadcastFlow2.JPG

Attachment for Comment 10.

Comment 12

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

The first (amazing) chart is correct.  See https://android.googlesource.com/platform/frameworks/opt/telephony/+/jb-mr2-dev/src/java/com/android/internal/telephony/cdma/CdmaSMSDispatcher.java , line 151.  All CDMA SMS messages are dispatched to |CdmaSMSDispatcher.dispatchMessage|, which is called in EVENT_NEW_SMS handler in SMSDispatcher, and that's emitted in RIL_UNSOL_RESPONSE_CDMA_NEW_SMS handler in RIL.java.

Comment 13

[na-Kobayashi]

Attachment: Bug 910091.patch

Hi Vicamo

Thank you for your reply about my #commet 10.
I have created patch and uploaded about your #comment 7.
 - Add "serviceCategory" attributes to MozCellBroadcastMessage.
please confirm it.

and I think, 
name for "mosCellBroadcast" couldn't understanding in CDMA area.
if "Cell Broadcast for GSM" and "Broadcast SMS for CDMA" are same implementation,
We should rename "mozCellBroadcast" to abstract name for Broadcast. e.g. "mozBroadcastMessage".

What do you think about it?

Comment 14

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

Comment on attachment 819578 [details] [diff] [review]
Bug 910091.patch

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

About the rename thing, I think unless we'll make this into W3C but with a different name, I'd rather keep the way it is so that we don't introduce extra work to Gaia and anyone else.  Thank you :)

::: dom/cellbroadcast/interfaces/nsIDOMMozCellBroadcastMessage.idl
@@ +62,5 @@
> +
> +  /**
> +   * Service Category.
> +   */
> +  readonly attribute long serviceCategory;

Could you rename this attribute to 'cdmaServiceCategory' and rename 'geographicalScope' to 'gsmGeographicalScope'?  This means you'll have to update a few GSM test cases as well.  Beside, place this right below 'geographicalScope' might be a good idea.

::: dom/system/gonk/RILContentHelper.js
@@ +336,5 @@
>    if (pdu.etws != null) {
>      this.etws = new CellBroadcastEtwsInfo(pdu.etws);
>    }
> +
> +  this.serviceCategory = pdu.serviceCategory;

ditto

@@ +359,5 @@
>    messageClass: null,
>    timestamp: null,
>  
> +  etws: null,
> +  serviceCategory: null

ditto

Comment 15

[na-Kobayashi]

Attachment: Bug 910091_2.patch

Hi Vicamo,

Thank you for review.

I have created patch and uploaded.
 - rename attribute name.
 - modified test code.

Please check it.

Comment 16

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

Comment on attachment 821543 [details] [diff] [review]
Bug 910091_2.patch

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

Thank you.  r=me with uuid updated. :)

::: dom/cellbroadcast/interfaces/nsIDOMMozCellBroadcastMessage.idl
@@ +62,5 @@
> +
> +  /**
> +   * Service Category.
> +   */
> +  readonly attribute long cdmaServiceCategory;

I should have pointed out in previous patch.  You have to update uuid of this interface as well.  You might execute `uuidgen -r` to get one :)

::: dom/cellbroadcast/tests/marionette/test_cellbroadcast_gsm.js
@@ +453,5 @@
> +    let message = event.message;
> +
> +    // Bug 910091
> +    // in GSM, "service category" is "undefined" in gecko layer.
> +    // application receive always "Service Category" is '0'.

`"Service Category" is not defined in GSM.  We should always get '0' here.`

Comment 17

[na-Kobayashi]

Attachment: Bug 910091_3.patch

Hi Vicamo,

Thank you for review.
I have created patch and uploaded.

This is the first time that commit it.
Please tell me what should I do next step?

Comment 18

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

(In reply to na-Kobayashi from comment #17)
> Bug 910091_3.patch
> 
> Hi Vicamo,
> 
> Thank you for review.
> I have created patch and uploaded.
> 
> This is the first time that commit it.
> Please tell me what should I do next step?

Kobayashi, several bits below in regard to your patches here, you may find some more documents in MDN. [1]

1. Obsolete previous revision when you upload a newer patch that is supposed to replace it.  This helps committer, probably your reviewer, to identify which patch to review, to land.  Also helps others grep a full picture of what's going on here and what's next.

2. Usually when a reviewer gives a "r+" on the patch, that means "it looks nice, I'd like to skip further reviews if it doesn't change too much in latter revisions."  And usually this comes with "r=me blah blah blah", which is a friendly remind if you forget to put the reviewer's name in your patch commit summary.

3. Follow "How can I generate a patch" [2] document to create patches to land.  Some reviewers may even ask you to prepare correct patch at very beginning.  A patch of incorrect format shouldn't be landed ever.  Even it somehow does, it will be backed out soon or later.

4. To checkin code, place a "checkin-needed" in the Keywords field on top of this page as a sign to ask sheriffs/reviewers to commit all patches attached to this bug, or, set "checkin-needed" for each reviewed patch that is ready to land.

Er, I'm a little bit too talky here.  In summary, please 1) generate a final patch as [2] suggests, 2) upload it here and obsolete all previous revisions, 3)  place a "checkin-needed" string kn Keywords, or need info me and I will commit that patch for you.

Thank you :)

[1]: https://developer.mozilla.org/en-US/docs/Introduction
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
https://wiki.mozilla.org/Release_Management/B2G_Landing
[2]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 19

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

Have a try first :) https://tbpl.mozilla.org/?tree=Try&rev=bc28ea7b79a3

Comment 20

[na-Kobayashi]

Attachment: Bug 910091_final.patch

Hi Vicamo,

Thank you for your advice. Your advice is really useful for me.
I have created patch as Mercurial and uploaded.
Please commit it.

> Have a try first :) https://tbpl.mozilla.org/?tree=Try&rev=bc28ea7b79a3

I don't have access permission to try server.

Comment 21

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

https://hg.mozilla.org/integration/b2g-inbound/rev/09d749f0368b

Comment 22

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

(In reply to na-Kobayashi from comment #20)
> I don't have access permission to try server.

You might consider applying for it (Level 1 try server access).  See:

https://wiki.mozilla.org/ReleaseEngineering/TryServer
http://www.mozilla.org/hacking/committer/
https://bugzilla.mozilla.org/show_bug.cgi?id=726493

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09d749f0368b