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

RESOLVED FIXED

Status

Firefox OS
RIL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: kenzo Ohta, Assigned: kenzo Ohta)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Summary: Broadcast SMS on CDMA → Broadcast SMS on CDMA(can't get Service Category)
(Assignee)

Comment 2

5 years ago
Created attachment 807110 [details] [diff] [review]
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.
This is for the Mobile Message API team.
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
Vicamo is in charge of the cell broadcast. He can support you to work it out.
Assignee: nobody → mu-ota
Blocks: 726098
Attachment #807110 - Flags: feedback?(vyang)
Hi kenzo Ohta, please change feedback? to review? if you think your patch is actually ready to review. Thanks!
(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 :)
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
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.
Attachment #807110 - Flags: feedback?(vyang) → feedback-

Comment 8

5 years ago
Kenzo, it seems what you want have been implemented in bug 890819.
Flags: needinfo?(mu-ota)
(Assignee)

Comment 9

5 years ago
(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.
Flags: needinfo?(mu-ota)

Comment 10

5 years ago
Created attachment 814311 [details]
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

5 years ago
Created attachment 814312 [details]
BroadcastFlow2.JPG

Attachment for Comment 10.
(Assignee)

Updated

5 years ago
Flags: needinfo?(kchang)
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.
Flags: needinfo?(kchang)

Comment 13

5 years ago
Created attachment 819578 [details] [diff] [review]
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?
Attachment #819578 - Flags: review?(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
Attachment #819578 - Flags: review?(vyang) → review+

Comment 15

5 years ago
Created attachment 821543 [details] [diff] [review]
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.
Attachment #821543 - Flags: review?(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.`
Attachment #821543 - Flags: review?(vyang) → review+

Comment 17

5 years ago
Created attachment 822092 [details] [diff] [review]
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?
Attachment #822092 - Flags: review?(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
Attachment #822092 - Flags: review?(vyang)

Comment 20

5 years ago
Created attachment 823193 [details] [diff] [review]
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.
Attachment #819578 - Attachment is obsolete: true
Attachment #821543 - Attachment is obsolete: true
Attachment #822092 - Attachment is obsolete: true
Attachment #807110 - Attachment is obsolete: true
(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
https://hg.mozilla.org/mozilla-central/rev/09d749f0368b
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.