Closed Bug 778093 Opened 7 years ago Closed 7 years ago

B2G RIL: support Cell Broadcast

Categories

(Core :: DOM: Device Interfaces, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Keywords: dev-doc-needed, feature, Whiteboard: [LOE:M] [WebAPI:P0] [vivo-lab])

Attachments

(11 files, 67 obsolete files)

158.42 KB, text/plain
Details
179.07 KB, text/plain
Details
14.43 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
6.62 KB, patch
Details | Diff | Splinter Review
16.58 KB, patch
Details | Diff | Splinter Review
5.75 KB, patch
mounir
: superreview+
Details | Diff | Splinter Review
19.40 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
17.21 KB, patch
Details | Diff | Splinter Review
24.28 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
5.63 KB, patch
Details | Diff | Splinter Review
8.28 KB, patch
Details | Diff | Splinter Review
Vicamo, can you please outline what this is needed for and whether it blocks any feature in v1?
(In reply to Philipp von Weitershausen [:philikon] from comment #1)
> Vicamo, can you please outline what this is needed for and whether it blocks
> any feature in v1?

Jose told me cell broadcasting itself is a requested feature that has to be done in Brazil. I'm not aware of this before last Friday. Maybe Mvines, Dietrich, or Kevin can give us more detailed project feature items info.
I've started another round of SMSCB document study yesterday. A CB client application has to ask RIL to listen necessary channels(RIL_REQUEST_GSM_SET_BROADCAST_CONFIG) and activates CB reception(RIL_REQUEST_GSM_BROADCAST_ACTIVATION). The received message will be available in RIL_UNSOL_RESPONSE_NEW_BROADCAST_SMS. However, we can automatically activate/deactivate cb reception upon the number of listened channels, then we'll only need a new `(un)registerBroadcastChannel(int[])` API and a new `onbroadcast` event.

Another technical problem would be SMSCB has different binary formats on different radio tech and each of them may have different parameter set. They will probably don't fit into current SmsMessage API. I'll try to list all these parameters and see what we can do in a few days.
Thanks, Vicamo. I talked to Andreas, apparently this feature is necessary for device certification. Nom'ing for basecamp.
blocking-basecamp: --- → ?
Attached patch Part 1: IDL changes : WIP (obsolete) — Splinter Review
for public interfaces, add nsIDOMMozSmsCbMessage/nsIDOMMozSmsCbEvent and new APIs to SmsManager
Attached patch Part 2: DOM API : WIP (obsolete) — Splinter Review
Attached patch Part 3: RIL implementation : WIP (obsolete) — Splinter Review
Per comment #4.
blocking-basecamp: ? → +
Assignee: nobody → vyang
Status: NEW → ASSIGNED
Attached patch Part 1: IDL changes : WIP 2 (obsolete) — Splinter Review
Attachment #647985 - Attachment is obsolete: true
Attached patch Part 2: DOM API : WIP 2 (obsolete) — Splinter Review
Attachment #647986 - Attachment is obsolete: true
Mostly done, but need test cases to ensure basic functions work as expected.
Attachment #647987 - Attachment is obsolete: true
Whiteboard: [LOE:M]
Attached patch Part 1: IDL changes : WIP 3 (obsolete) — Splinter Review
1. Place cell broadcast DOM in navigator.mozCbs.
2. Remove enable/disable range API for now for they'll have the same OOP problem as bug 775997. Get cell broadcast search lists from (U)SIM or settings API instead.
Attachment #650285 - Attachment is obsolete: true
Attachment #657216 - Attachment description: Part 1: IDL changes : WIP 2 → Part 1: IDL changes : WIP 3
Attached patch Part 2: DOM API : WIP 3 (obsolete) — Splinter Review
Attachment #650286 - Attachment is obsolete: true
Attachment #650287 - Attachment is obsolete: true
Attached patch [DEBUG] (obsolete) — Splinter Review
Hi Michael, I managed to set cell broadcast settings & activate it, but there is no messages come up from modem. Could you help review the implementation here?

Steps to reproduce:
1. apply the four patches in order.
2. turn on debug messages in dom/system/gonk/ril_consts.js
3. rebuild gecko & install to device
4. send a sms message out from Otoro to trigger config/activation process.

Expects: UNSOLICITED_RESPONSE_NEW_BROADCAST_SMS events in ril_worker
Attachment #657219 - Flags: feedback?(mvines)
Depends on: 787420
Whiteboard: [LOE:M] → [LOE:M] [WebAPI:P0]
Keywords: feature
Whiteboard: [LOE:M] [WebAPI:P0] → [LOE:M] [WebAPI:P0] [vivo-lab]
Can we get an update on progress here?  Thanks.
Hi Andrew, I'm working on following three sub-tasks and will update all the patches tomorrow hopefully:

1. Rebase to accommodate to event object management change.
2. Cleanup IDL, add valid values, comments, etc. Rename to mozCellBroadcast.
3. Hook up with Settings API.
4. CMAS
5. Test cases if emulator allows.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> 4. CMAS

Documents for CMAS over CDMA is available in:
http://ftp.3gpp2.org/TSGC/working/2009/2009-12-Maui/TR45.5-2009-12-Maui/WG2/520-09120806A__Editor_TIA-1149-0-1_CMASoverCDMA_Baseline.doc

And CMAS over GSM/UMTS:
http://webstore.ansi.org/RecordDetail.aspx?sku=ATIS-0700006#.UGKrlBL5E3M

I will remove CMAS definitions/interfaces.
Attached patch Part 1: IDL changes (obsolete) — Splinter Review
For now, we supports GSM & ETWS Cell Broadcast messages only.
Attachment #657216 - Attachment is obsolete: true
Attachment #665171 - Flags: superreview?(mounir)
Attachment #657217 - Attachment is obsolete: true
Attachment #665173 - Flags: review?(mounir)
Implementation of CellBroadcastMessage is in later patch written in javascript.
Attachment #665178 - Flags: review?(mounir)
Attachment #657218 - Attachment is obsolete: true
Attachment #665180 - Attachment description: Part 3: RIL implementation : WIP 4 → Part N-1: RIL implementation : WIP 4
Attached patch Part N: test cases : WIP (obsolete) — Splinter Review
Attached patch [DEBUG] (obsolete) — Splinter Review
Attachment #657219 - Attachment is obsolete: true
Attachment #657219 - Flags: feedback?(mvines)
Comment on attachment 665171 [details] [diff] [review]
Part 1: IDL changes

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

::: dom/cellbroadcast/interfaces/nsIDOMCellBroadcastMessage.idl
@@ +14,5 @@
> +   */
> +  const unsigned short GEOGRAPHICAL_SCOPE_CELL_WIDE_IMMEDIATE = 0x00;
> +  const unsigned short GEOGRAPHICAL_SCOPE_PLMN_WIDE           = 0x01;
> +  const unsigned short GEOGRAPHICAL_SCOPE_LOCATION_AREA_WIDE  = 0x02;
> +  const unsigned short GEOGRAPHICAL_SCOPE_CELL_WIDE           = 0x03;

Use strings.

@@ +50,5 @@
> +  readonly attribute nsIDOMMozCellBroadcastEtwsInfo etws;
> +};
> +
> +[scriptable, uuid(af009d9a-f5e8-4573-a6ee-a85118465bed)]
> +interface nsIDOMMozCellBroadcastEtwsInfo : nsISupports

What ETWS means? Can you put some comments explaining that.

@@ +59,5 @@
> +  const unsigned short WARNING_TYPE_EARTHQUAKE             = 0x0;
> +  const unsigned short WARNING_TYPE_TSUNAMI                = 0x1;
> +  const unsigned short WARNING_TYPE_EARTHQUAKE_AND_TSUNAMI = 0x2;
> +  const unsigned short WARNING_TYPE_TEST                   = 0x3;
> +  const unsigned short WARNING_TYPE_OTHER                  = 0x4;

Use strings.

@@ +64,5 @@
> +
> +  readonly attribute unsigned short warningType;
> +
> +  /**
> +   * Activate emergency user alert.

"Activate" seems to be the wrong word here. Given that the attribute is RO, it's more about reading the state than interacting with it.

@@ +69,5 @@
> +   */
> +  readonly attribute boolean emergencyUserAlert;
> +
> +  /**
> +   * Activate popup on the display.

ditto
Attachment #665171 - Flags: superreview?(mounir) → superreview-
Comment on attachment 665176 [details] [diff] [review]
Part 3: add switch perference dom.cellbroadcast.enabled

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

I do not think you need a preference for that. There is already a compilation flag (RIL) and a permission.
The preference could be useful to have users disabling the feature but it seems like it's not something we want them to be able to do on a phone, right?
Attachment #665176 - Flags: review?(mounir) → review-
Comment on attachment 665179 [details] [diff] [review]
Part 5: add to global javascript namespace test list

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

::: dom/tests/mochitest/general/test_interfaces.html
@@ +535,5 @@
>      "MozTimeManager",
>      "MozNavigatorTime",
> +    "PermissionSettings",
> +    "MozCellBroadcast",
> +    "MozCellBroadcastEvent"

Those two interfaces should be visible only if we are running a B2G build with RIL enabled.

If we have no way to check that for the moment, you could just not have them here. We don't run tests on B2G devices on m-c anyway. But if we have no way to check this, you should open a follow-up.
Attachment #665179 - Flags: review?(mounir) → review-
Comment on attachment 665178 [details] [diff] [review]
Part 4: add MozCellBroadcastEvent and complete onreceived event handling

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

This patch looks generally good but there are a lot of small things I would like to see fixed and there are patches in the patch queue that will have to change and will make that patch changes. So, I can't r+ because I want to have another look after all those updates.

::: dom/base/nsDOMClassInfo.cpp
@@ +1503,5 @@
>    NS_DEFINE_CLASSINFO_DATA(MozCellBroadcast, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)
>  
> +  NS_DEFINE_CLASSINFO_DATA(MozCellBroadcastEvent, nsDOMGenericSH,
> +                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

This should be in #ifdef B2G_RIL.

@@ +4154,5 @@
>  
> +  DOM_CLASSINFO_MAP_BEGIN(MozCellBroadcastEvent, nsIDOMMozCellBroadcastEvent)
> +     DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozCellBroadcastEvent)
> +     DOM_CLASSINFO_EVENT_MAP_ENTRIES
> +  DOM_CLASSINFO_MAP_END

Same for that block.

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +46,5 @@
> +  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +  // GetObserverService() can return null is some situations like shutdown.
> +  if (!obs) {
> +    return;
> +  }

So, in an Init() method, doing that would be incorrect because you would be able to return a nsresult. However, in a ctor, you should do that. And here, you should definitely use a ctor.

@@ +65,5 @@
> +}
> +
> +nsresult
> +CellBroadcast::DispatchTrustedEventToSelf(const nsAString& aEventName,
> +                                          nsIDOMMozCellBroadcastMessage* aMessage)

Please, add a nsContentUtils method doing that. It's the nth class with a DispatchTrustedEventToSelf method.

@@ +96,5 @@
> +{
> +  if (!strcmp(aTopic, kCellBroadcastReceivedObserverTopic)) {
> +    nsCOMPtr<nsIDOMMozCellBroadcastMessage> message = do_QueryInterface(aSubject);
> +    if (!message) {
> +      NS_ERROR("Got a 'cellbroadcast-message-received' topic without a valid message!");

I think you could simply use:
NS_ASSERTION(message, "Got a 'cellbroadcast-message-received' topic without a valid message!");

@@ +101,5 @@
> +      return NS_OK;
> +    }
> +
> +    DispatchTrustedEventToSelf(RECEIVED_EVENT_NAME, message);
> +    return NS_OK;

return DispatchTrustedEventToSelf(...);

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +7,5 @@
>  #define mozilla_dom_cellbroadcast_CellBroadcast_h__
>  
>  #include "nsDOMEventTargetHelper.h"
>  #include "nsIDOMCellBroadcast.h"
> +#include "nsIDOMCellBroadcastMessage.h"

Don't include nsIDOMCellBroadcastMessage. This has to be forward declared.

::: dom/cellbroadcast/src/CellBroadcastEvent.cpp
@@ +59,5 @@
> +
> +nsresult
> +NS_NewDOMCellBroadcastEvent(nsIDOMEvent** aInstancePtrResult,
> +                            nsPresContext* aPresContext,
> +                            nsEvent* aEvent)

Why are you defining this method here? It's declared and used nowhere.
Remove it and add it if you happen to need it.

::: dom/cellbroadcast/src/CellBroadcastEvent.h
@@ +12,5 @@
> +#include "mozilla/Attributes.h"
> +
> +namespace mozilla {
> +namespace dom {
> +namespace cellbroadcast {

Don't need 'cellbroadcast' namespace.

::: dom/cellbroadcast/src/Constants.h
@@ +9,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace cellbroadcast {
> +
> +extern const char* kCellBroadcastReceivedObserverTopic;  // Defined in the .cpp.

I do not see the point in declaring this in another file if the file isn't exported and if it only has one consumer.
Attachment #665178 - Flags: review?(mounir) → review-
Comment on attachment 665173 [details] [diff] [review]
Part 2: add navigator.mozCellBroadcast

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

I think you forgot mobile/android/installer/package-manifest.in

This is mostly boilerplate and the boilerplate is generally fine here.
However, there are things I would like to see changed, fixed or moved so I would prefer to have a review of the modified boilerplate.

::: dom/base/nsDOMClassInfo.cpp
@@ +1499,5 @@
>    NS_DEFINE_CLASSINFO_DATA(MozSmsCursor, nsDOMGenericSH,
>                             DOM_DEFAULT_SCRIPTABLE_FLAGS)
>  
> +  NS_DEFINE_CLASSINFO_DATA(MozCellBroadcast, nsDOMGenericSH,
> +                           DOM_DEFAULT_SCRIPTABLE_FLAGS)

Could you put that between #ifdef MOZ_B2G_RIL?

@@ +4145,5 @@
>    DOM_CLASSINFO_MAP_END
>  
> +  DOM_CLASSINFO_MAP_BEGIN(MozCellBroadcast, nsIDOMMozCellBroadcast)
> +     DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozCellBroadcast)
> +  DOM_CLASSINFO_MAP_END

Same here.

::: dom/base/nsDOMClassInfoClasses.h
@@ +406,5 @@
>  DOMCI_CLASS(MozSmsFilter)
>  DOMCI_CLASS(MozSmsCursor)
>  
> +#ifdef MOZ_B2G_RIL
> +DOMCI_CLASS(MozCellBroadcast)

But that close to MozMobileConnection below.

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +11,5 @@
> +#include "mozilla/Attributes.h"
> +
> +namespace mozilla {
> +namespace dom {
> +namespace cellbroadcast {

Remove that namespace.

@@ +25,5 @@
> +
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(CellBroadcast,
> +                                           nsDOMEventTargetHelper)
> +
> +  void Init(nsPIDOMWindow *aWindow);

Remove this Init() method and just make the ctor takes a nsPIDOMWindow.
You can even do:
CellBroadcast() MOZ_DELETE;
CellBroadcast(nsPIDOMWindow* aWindow);
that way, you will know, the default ctor will never be used.

@@ +26,5 @@
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(CellBroadcast,
> +                                           nsDOMEventTargetHelper)
> +
> +  void Init(nsPIDOMWindow *aWindow);
> +  void Shutdown();

Do you really need Shutdown() here? I understand it will be used later but even later. Is there anything that has to be done *before* the dtor is called? or ASAP when shutdown is called?
This is a real question. Do not feel like you have to remove Shutdown() just think about the real usage.

::: dom/cellbroadcast/src/Makefile.in
@@ +30,5 @@
> +  -I$(topsrcdir)/dom/base \
> +  $(NULL)
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

You do not need the IPC mk file.

::: dom/dom-config.mk
@@ +41,5 @@
>    dom/system/gonk \
>    dom/telephony \
>    dom/wifi \
>    dom/icc/src \
> +  dom/cellbroadcast/src \

We should stop polluting dom/.
I'm not sure where to put this though. telephony?
Attachment #665173 - Flags: review?(mounir) → review-
I have a very partial view of what this feature is about and that is not helping reviewing it. I understand what this new interface is doing but I'm not sure why that should be exposed to content. Also, what exactly is mandatory to have the phone validated? I guess not just having the interface exposed. Do we have to provide a UI or something?

My main concern is that I doubt anyone except the system will really want/be able to get informed and I wonder if that wouldn't be better to simply have this event being a notification for the b2g chrome code and the chrome code would do what has to be done.

So, currently, what are the planned consumers of this event?
(In reply to Mounir Lamouri (:mounir) from comment #34)
> I have a very partial view of what this feature is about and that is not
> helping reviewing it. I understand what this new interface is doing but I'm
> not sure why that should be exposed to content. Also, what exactly is
> mandatory to have the phone validated?

Brazil radio regulation(something alike, I'm not Brazil law expert, that's all I know) requires every mobile phone shows roaming information on the screen, that widget is called NetworkInfo. This widget consists of two lines of text messages, both provide some kind of information of the carrier you've registered with. The second line in that widget, in 2G network, must be the text message retrieved from Cell Broadcast.

We have already completed the Gecko parts in bug 780558 and first stage Gaia UI in https://github.com/mozilla-b2g/gaia/issues/3176 for 3G network.

> I guess not just having the interface exposed. Do we have to provide a UI or something?
https://github.com/mozilla-b2g/gaia/issues/4559 for lock screen.
https://github.com/mozilla-b2g/gaia/issues/4545 for status bar.

Please feel free to raise more questions, there are people know the situation more than I do.
(In reply to Mounir Lamouri (:mounir) from comment #33)
> ::: dom/dom-config.mk
> @@ +41,5 @@
> >    dom/system/gonk \
> >    dom/telephony \
> >    dom/wifi \
> >    dom/icc/src \
> > +  dom/cellbroadcast/src \
> 
> We should stop polluting dom/.

Why? Is there a rule for what can go into dom/ and what can't?

> I'm not sure where to put this though. telephony?

Telephony is the wrong place, this feature has nothing to do with making phone calls.

(In reply to Mounir Lamouri (:mounir) from comment #34)
> I have a very partial view of what this feature is about and that is not
> helping reviewing it. I understand what this new interface is doing but I'm
> not sure why that should be exposed to content. Also, what exactly is
> mandatory to have the phone validated? I guess not just having the interface
> exposed. Do we have to provide a UI or something?

Yes. As Vicamo said, this is required by Brazil law.

> My main concern is that I doubt anyone except the system will really want/be
> able to get informed and I wonder if that wouldn't be better to simply have
> this event being a notification for the b2g chrome code and the chrome code
> would do what has to be done.
> 
> So, currently, what are the planned consumers of this event?

When you say "system", do you mean just Gecko? We definitely need to bubble this to the UI, but we might not need to do this using a DOM event. I think we could get by just using a system message. I think that would definitely be way simpler as it doesn't require any IDL changes and no C++ boilerplate code. Would that work for you guys, Vicamo and Mounir?
(In reply to Mounir Lamouri (:mounir) from comment #32)
> Comment on attachment 665178 [details] [diff] [review]
> Part 4: add MozCellBroadcastEvent and complete onreceived event handling
> ::: dom/cellbroadcast/src/CellBroadcast.cpp
> @@ +65,5 @@
> > +}
> > +
> > +nsresult
> > +CellBroadcast::DispatchTrustedEventToSelf(const nsAString& aEventName,
> > +                                          nsIDOMMozCellBroadcastMessage* aMessage)
> 
> Please, add a nsContentUtils method doing that. It's the nth class with a
> DispatchTrustedEventToSelf method.

I filed bug 794991 to track this. There are several modules having the same code, and I think replacing them here is probably beyond the purpose of this issue.

> @@ +96,5 @@
> > +{
> > +  if (!strcmp(aTopic, kCellBroadcastReceivedObserverTopic)) {
> > +    nsCOMPtr<nsIDOMMozCellBroadcastMessage> message = do_QueryInterface(aSubject);
> > +    if (!message) {
> > +      NS_ERROR("Got a 'cellbroadcast-message-received' topic without a valid message!");
> 
> I think you could simply use:
> NS_ASSERTION(message, "Got a 'cellbroadcast-message-received' topic without
> a valid message!");

Considering I have message interface implemented in Javascript, type casting check here can help capturing implementation errors. I can remove it if you still disagree.
(In reply to Philipp von Weitershausen [:philikon] from comment #36)
> (In reply to Mounir Lamouri (:mounir) from comment #34)
> > So, currently, what are the planned consumers of this event?
> 
> When you say "system", do you mean just Gecko? We definitely need to bubble
> this to the UI, but we might not need to do this using a DOM event. I think
> we could get by just using a system message. I think that would definitely
> be way simpler as it doesn't require any IDL changes and no C++ boilerplate
> code. Would that work for you guys, Vicamo and Mounir?

Ahhhhhhhh .... (fallen into a endless whirlpool)
Attached patch Part 1: IDL changes : v2 (obsolete) — Splinter Review
Address review comment #29
Attachment #665171 - Attachment is obsolete: true
Attachment #665513 - Flags: superreview?(mounir)
address review comment #33
Attachment #665173 - Attachment is obsolete: true
Attachment #665515 - Flags: review?(mounir)
Comment on attachment 665176 [details] [diff] [review]
Part 3: add switch perference dom.cellbroadcast.enabled

Obsolete as suggested in review comment #30
Attachment #665176 - Attachment is obsolete: true
Address review comment #32, but still keep type-casting check in dom/cellbroadcast/src/CellBroadcast.cpp
Attachment #665178 - Attachment is obsolete: true
Attachment #665518 - Flags: review?(mounir)
Comment on attachment 665179 [details] [diff] [review]
Part 5: add to global javascript namespace test list

Obsolete as suggested in review comment #31. But I remember removing these entries might cause that test case busted in tpbl. I'm having a try for this: https://tbpl.mozilla.org/?tree=Try&rev=cd7b07fb6cac .
Attachment #665179 - Attachment is obsolete: true
Attached patch Part N: test cases : WIP 2 (obsolete) — Splinter Review
Attachment #665181 - Attachment is obsolete: true
(In reply to Philipp von Weitershausen [:philikon] from comment #36)
> (In reply to Mounir Lamouri (:mounir) from comment #34)
> > So, currently, what are the planned consumers of this event?
> 
> When you say "system", do you mean just Gecko? We definitely need to bubble
> this to the UI, but we might not need to do this using a DOM event. I think
> we could get by just using a system message. I think that would definitely
> be way simpler as it doesn't require any IDL changes and no C++ boilerplate
> code. Would that work for you guys, Vicamo and Mounir?

I'm going to use this API in Gaia lock screen and status bar, both belong to system app, so there is no 'background services wasting memory' problem as SMS & Telephony do.

I've implemented a part of CBS functionality. Some features like duplicate detection will need a database to accomplish. And we'll certainly need a database to allow reviewing all the messages received from CBS. We just discard them after received and parsed now. DOM API code is still a must. It's unlikely to be removed in near future.

Besides, it will be definitely easier for me to write all the test scripts with DOM API. I've spent most of the time these days working on writing test scripts to find out what's probably wrong. It is really helpful to verify PDU parsing logic because in Taiwan we don't have Cell Broadcast.
Attached patch Part 1: IDL changes : v3 (obsolete) — Splinter Review
CellBroadcastMessage.timestamp shouldn't have `implicit_jscontext` mark because we're going to implement it in javascript.
Attachment #665513 - Attachment is obsolete: true
Attachment #665513 - Flags: superreview?(mounir)
Attachment #665881 - Flags: superreview?(mounir)
Attached patch Part 1: IDL changes : v4 (obsolete) — Splinter Review
re-exported in hg format
Attachment #665881 - Attachment is obsolete: true
Attachment #665881 - Flags: superreview?(mounir)
rebase
Attachment #665515 - Attachment is obsolete: true
Attachment #665515 - Flags: review?(mounir)
rebase
Attachment #665518 - Attachment is obsolete: true
Attachment #665518 - Flags: review?(mounir)
Attached patch Part 4: RIL event delivery : v1 (obsolete) — Splinter Review
rebase
Attachment #665818 - Attachment is obsolete: true
rebase
Attachment #665819 - Attachment is obsolete: true
Attachment #665820 - Attachment is obsolete: true
Attachment #665180 - Attachment is obsolete: true
Attached patch Part 11: test cases : v1 (obsolete) — Splinter Review
Attached patch [DEBUG] Gecko (obsolete) — Splinter Review
Debug patch for Gecko
Attached patch [DEBUG] Gecko-Marionette (obsolete) — Splinter Review
Debug patch for Gecko Marionette tests.
Attachment #665182 - Attachment is obsolete: true
I still have no luck in working on Cell Broadcast on Otoro. The SET_BROADCAST_SMS_CONFIG and SMS_BROADCAST_ACTIVATION request ends with success, but still have no messages comes up from rild.

The device had connected to a 2G network as shown at line 1488 in attachment #667629 [details]. CB related parcel transactions begin at line 1556 in attachment #667629 [details], and the corresponding radio parts begin at line 2246 in attachment #667630 [details]. I was trying to enable full range(0..65535) reception. The results of both commands were SUCCESS.
I'm going to have a try on Galaxy S2, which uses Broadcom chip.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #65)
> I'm going to have a try on Galaxy S2, which uses Broadcom chip.

I can activate Cell Broadcast in Galaxy S2 with above patches but still have the same problem: no broadcast message comes up from rild.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #66)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #65)
> > I'm going to have a try on Galaxy S2, which uses Broadcom chip.
> 
> I can activate Cell Broadcast in Galaxy S2 with above patches but still have
> the same problem: no broadcast message comes up from rild.

Finally!!!!!! Galaxy S2!!!!!!

The patches are not modified at all, waiting like 10+ minutes, it's seems not broadcasting periodically.

I/Gecko   ( 1823): RIL Worker: New incoming parcel of size 100
I/Gecko   ( 1823): RIL Worker: Parcel (size 100): 1,0,0,0,253,3,0,0,88,0,0,0,0,0,0,50,1,17,214,164,245,9,146,42,65,178,88,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,104,52,26,141,70,163,209,0
I/Gecko   ( 1823): RIL Worker: We have at least one complete parcel.
I/Gecko   ( 1823): RIL Worker: Unsolicited response for request type 1021
I/Gecko   ( 1823): RIL Worker: Handling parcel as UNSOLICITED_RESPONSE_NEW_BROADCAST_SMS
I/Gecko   ( 1823): RIL Worker: PDU: read CBS dcs: 1
I/Gecko   ( 1823): -*- RadioInterfaceLayer: Received message from worker: {"serial":0,"updateNumber":0,"dcs":1,"encoding":0,"hasLanguageIndicator":false,"data":null,"pageIndex":1,"numPages":1,"format":0,"geographicalScope":0,"messageCode":0,"messageId":50,"language":"en","etws":{"warningType":null,"popup":false,"emergencyUserAlert":false},"fullBody":"VIVO RJ 21\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r\r","rilMessageType":"cellbroadcast-received"}
I/GeckoDump( 1823): Cell Broadcast Message timestamp: 1349367286636
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #67)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #66)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #65)
> > > I'm going to have a try on Galaxy S2, which uses Broadcom chip.
> > 
> > I can activate Cell Broadcast in Galaxy S2 with above patches but still have
> > the same problem: no broadcast message comes up from rild.
> 
> Finally!!!!!! Galaxy S2!!!!!!
> 
> The patches are not modified at all, waiting like 10+ minutes, it's seems
> not broadcasting periodically.

Since there code is proven working, I'll do some minor updates and request for review later.
rebase & add to PermissionTable
Attachment #667672 - Attachment is obsolete: true
Attached patch Part 4: RIL event delivery : v2 (obsolete) — Splinter Review
1. fix no ETWS info in GSM messages with ETWS messageIds
2. fix invalid body value when dcs is 8-bits
Attachment #667674 - Attachment is obsolete: true
1. fix no ETWS info in GSM messages with ETWS messageIds
2. fix invalid body value when dcs is 8-bits
Attachment #667677 - Attachment is obsolete: true
1. fix missing ETWS warning type in GSM messages with ETWS messageIds
2. fix invalid body value when dcs is 8-bits
3. fix UCS2 body parsing
4. support coding group 0x90
5. fix wrong encoding value in coding group 0xF0
6. rename readCbData to readGsmCbData for latter readUtmsCbData
Attachment #667679 - Attachment is obsolete: true
”this.cell“ had been moved to “this.voiceRegistrationState.cell”
Attachment #667680 - Attachment is obsolete: true
extract UMTS CB data reading parts to a function for unit testing
Attachment #667682 - Attachment is obsolete: true
1. update cell broadcast config upon radio state becomes ready
2. update cell broadcast config on rild connected
3. move non-mmi-settable ranges to ril_consts
4. rewrite settings change event handling
Attachment #667683 - Attachment is obsolete: true
update cb config upon EFcbmi & EFcbmir read event
Attachment #667685 - Attachment is obsolete: true
Attached patch Part 11: test cases : v2 (obsolete) — Splinter Review
1. add xpcshell unit test cases for important functions
2. add more marionette test cases for GSM messages
3. refactor ETWS test cases
4. we have no "dom.cellbroadcast.enabled" preference anymore, remove.
5. remove mochitest case for it's not locally tested
Attachment #667686 - Attachment is obsolete: true
Current status: have to fix Gaia permission, and cooperate with WIP patch in bug 802121.
Priority: -- → P1
We're marking this bug with the C1 milestone since it follows the criteria of "unfinished feature work" (see https://etherpad.mozilla.org/b2g-convergence-schedule).

If this work is not finished by Nov19, this bug will need an exception and will be called out at the upcoming Exec Review.
Target Milestone: --- → B2G C1 (to 19nov)
Vicamo, any update on the status of this work?
(In reply to Dietrich Ayala (:dietrich) from comment #80)
> Vicamo, any update on the status of this work?

I was previously blocked by bug 809717 during the work week. It fails all xpcshell tests in dom/system/gonk. I didn't have a work-around until Thursday daybreak of the SF work week. Cell Broadcast is not widely deployed, and emulator/marionette/xpcshell are the only tools I get verify my changes. If I just find they're broken yet again, I'll either turn to other issues and come back in a few days, or spend time to survey the root cause and possible work-arounds.

Yes, I have fully functional emulator now and am working on this.
Attachment #667688 - Attachment is obsolete: true
Attachment #667689 - Attachment is obsolete: true
Attached patch Part 1: IDL changes : v5 (obsolete) — Splinter Review
1) Use DOMString rather than ACString for js impl
2) add Message Class as we did in bug 797277. Cell Broadcast message has also Message-Class attribute as SMS does, but they're basically two different formats.
3) Comments
Attachment #667670 - Attachment is obsolete: true
Attachment #681984 - Flags: superreview?(mounir)
Rebase only
Attachment #672908 - Attachment is obsolete: true
Attachment #681985 - Flags: review?(mounir)
Attached patch Part 4: RIL event delivery : v3 (obsolete) — Splinter Review
1) rebase
2) add Message-Class
Attachment #672910 - Attachment is obsolete: true
Attachment #681989 - Flags: review?(htsai)
1) rebase
2) fix undefined messageClass DOM attribute for ETWS messages
Attachment #672911 - Attachment is obsolete: true
Attachment #681991 - Flags: review?(htsai)
1) rebase
2) Message-Class
Attachment #672913 - Attachment is obsolete: true
Attachment #681992 - Flags: review?(htsai)
rebase only
Attachment #672916 - Attachment is obsolete: true
Attachment #681996 - Flags: review?(htsai)
Move complex calculation into ril_worker
Attachment #672917 - Attachment is obsolete: true
Attachment #681999 - Flags: review?(htsai)
Move complex calculation into ril_worker
Attachment #672918 - Attachment is obsolete: true
Attachment #682001 - Flags: review?(htsai)
Attached patch Part 11: test cases : v3 (obsolete) — Splinter Review
1) Add Message-Class tests
2) Don't wait for MobileConnection
3) Fix test cases after all the changes made
Attachment #672921 - Attachment is obsolete: true
Attachment #682002 - Flags: review?(htsai)
Attachment #667673 - Flags: review?(mounir)
Attachment #672914 - Flags: review?(htsai)
Attached patch Part 1: IDL changes : v5 (obsolete) — Splinter Review
Oops, previous patch is a completely wrong one. Thanks HsinYi.
Attachment #681984 - Attachment is obsolete: true
Attachment #681984 - Flags: superreview?(mounir)
Attachment #682300 - Flags: review?(mounir)
Attachment #682300 - Flags: review?(mounir) → superreview?(mounir)
Comment on attachment 667673 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v3

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

How about use event codegen here? Bug 780142 is a good example.
Comment on attachment 681989 [details] [diff] [review]
Part 4: RIL event delivery : v3

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

This patch is the same as Part 3. Seems you submitted the wrong file here.
Attachment #681989 - Flags: review?(htsai)
Attached patch Part 4: RIL event delivery : v3 (obsolete) — Splinter Review
Attachment #681989 - Attachment is obsolete: true
Attachment #682317 - Flags: review?(htsai)
Comment on attachment 682317 [details] [diff] [review]
Part 4: RIL event delivery : v3

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

Overall the patch looks good. Thanks, Vicamo! r- simply because we shouldn't broadcast this message to all content processes.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +527,5 @@
>          return;
> +      case "cellbroadcast-received":
> +        message.timestamp = Date.now();
> +        ppmm.broadcastAsyncMessage("RIL:CellBroadcastReceived", message);
> +        break;

We don't really want to broadcast this message to every content process. Simply send this message to content with 'cellbroadcast' permission. 

You may refer to the way that 'telephony' is doing:
1) asking for registering in RILContentHelper [1]
2) registering targets and sending message back to only registered targets in RadioInterfaceLayer [2] [3]

[1] https://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RILContentHelper.js#602
[2] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#630
[3] http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/RadioInterfaceLayer.js#674
Attachment #682317 - Flags: review?(htsai) → review-
Comment on attachment 681991 [details] [diff] [review]
Part 5: support basic ETWS Primary Notification message : v3

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

Great! r=me with the comments addressed/responded. Thanks!

::: dom/system/gonk/ril_worker.js
@@ +6585,5 @@
> +   * @param msg
> +   *        message object for output.
> +   *
> +   * @see 3GPP TS 23.041 section 9.4.1.2.4
> +   */

should be 9.4.1.2.2?

@@ +6631,5 @@
> +        warningType:        null,                              //  X   O    X
> +        popup:              false,                             //  X   O    X
> +        emergencyUserAlert: false,                             //  X   O    X
> +      }*/
> +    };

nit: Please remove the comments "warningType, popup, emergencyUserAlert" here.

@@ +6658,5 @@
> +    // Warning Security Information.
> +    // `The UE shall ignore this parameter.`
> +
> +    // No body/data for ETWS messages.
> +

These three comment sentences are not clear enough to me. Would you please clarify them? Thanks.
Attachment #681991 - Flags: review?(htsai) → review+
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #96)
> Comment on attachment 681991 [details] [diff] [review]
> Part 5: support basic ETWS Primary Notification message : v3
> 
> Review of attachment 681991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great! r=me with the comments addressed/responded. Thanks!
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +6585,5 @@
> > +   * @param msg
> > +   *        message object for output.
> > +   *
> > +   * @see 3GPP TS 23.041 section 9.4.1.2.4
> > +   */
> 
> should be 9.4.1.2.2?
> 
> @@ +6631,5 @@
> > +        warningType:        null,                              //  X   O    X
> > +        popup:              false,                             //  X   O    X
> > +        emergencyUserAlert: false,                             //  X   O    X
> > +      }*/
> > +    };
> 
> nit: Please remove the comments "warningType, popup, emergencyUserAlert"
> here.

Second thought: let's keep them.

> 
> @@ +6658,5 @@
> > +    // Warning Security Information.
> > +    // `The UE shall ignore this parameter.`
> > +
> > +    // No body/data for ETWS messages.
> > +
> 
> These three comment sentences are not clear enough to me. Would you please
> clarify them? Thanks.
Comment on attachment 681992 [details] [diff] [review]
Part 6: support base GSM CBS message : v3

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

Thanks, Vicamo. I am cancelling the review until the comments are addressed/responded.

::: dom/system/gonk/ril_consts.js
@@ +837,5 @@
>  
> +// GSM Cell Broadcast Message Identifiers
> +// see 3GPP TS 23.041 clause 9.4.1.2.2
> +this.CB_GSM_MESSAGEID_ETWS_BEGIN = 0x1100;
> +this.CB_GSM_MESSAGEID_ETWS_END   = 0x1108;

I would prefer replace '0x1108' with *0x1107*, where the latter is more intuitive to me when I read the spec.

::: dom/system/gonk/ril_worker.js
@@ +278,5 @@
> +
> +    let array = new Uint8Array(length);
> +    for (let i = 0; i < length; i++) {
> +      array[i] = this.readUint8Unchecked();
> +    }

We should have "this.readAvailable--" in the function, right?

@@ +6604,5 @@
>      msg.messageId = Buf.readUint8() << 8 | Buf.readUint8();
> +
> +    if ((msg.format != CB_FORMAT_ETWS)
> +        && (msg.messageId >= CB_GSM_MESSAGEID_ETWS_BEGIN)
> +        && (msg.messageId < CB_GSM_MESSAGEID_ETWS_END)) {

s/</<= since I prefer applying 0x1107 to 'CB_GSM_MESSAGEID_ETWS_END'

@@ +6605,5 @@
> +
> +    if ((msg.format != CB_FORMAT_ETWS)
> +        && (msg.messageId >= CB_GSM_MESSAGEID_ETWS_BEGIN)
> +        && (msg.messageId < CB_GSM_MESSAGEID_ETWS_END)) {
> +      // `In the case of transmitting CBS message for ETWS, ... a part of

I think we can just remove '...' here, right?

@@ +6633,5 @@
> +    let dcs = Buf.readUint8();
> +    if (DEBUG) debug("PDU: read CBS dcs: " + dcs);
> +
> +    let language = null, hasLanguageIndicator = false;
> +    // `Any reserved codings shall be assumed to be the GSM 7bit default alphabet`

nit: Please place a period at the end of this line, inside the quotation mark.

@@ +6661,5 @@
> +
> +      case 0x40: // 01xx
> +      case 0x50:
> +      //case 0x60:
> +      //case 0x70:

Why comment these two? I thought they represent the same meaning as 0x40 and 0x50, no?

@@ +6712,5 @@
> +    msg.pageIndex = (octet >>> 4) & 0x0F;
> +    msg.numPages = octet & 0x0F;
> +    if (!msg.pageIndex || !msg.numPages) {
> +      // `If a mobile receives the code 0000 in either the first field or the
> +      // second field the it shall treat the CBS message exactly the same as a

nit: second field 's/the/then' it shall ...

@@ +6713,5 @@
> +    msg.numPages = octet & 0x0F;
> +    if (!msg.pageIndex || !msg.numPages) {
> +      // `If a mobile receives the code 0000 in either the first field or the
> +      // second field the it shall treat the CBS message exactly the same as a
> +      // CBS message with page parameter 0001 0001 (i.e. a single page message)`

nit: please place a period at the end of a line.

@@ +6778,5 @@
> +                                                       PDU_NL_IDENTIFIER_DEFAULT,
> +                                                       PDU_NL_IDENTIFIER_DEFAULT);
> +          --length;
> +        }
> +        msg.body = this.readUCS2String.call(bufAdapter, length);

The parameter of 'readUCS2String' stands for the number of *octets* to be read as UCS2 string, but it looks like you already apply the number of UCS2 string, which is wrong.
Attachment #681992 - Flags: review?(htsai)
Comment on attachment 672914 [details] [diff] [review]
Part 7: support GSM CBS message paging : v2

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

Looks great.
Attachment #672914 - Flags: review?(htsai) → review+
Comment on attachment 681996 [details] [diff] [review]
Part 8: support UMTS CBS message : v3

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

Cancelling the review until the comments addressed/responded. Thanks!

::: dom/system/gonk/ril_consts.js
@@ +850,5 @@
>    "other"
>  ];
>  
> +// UMTS Message Type
> +// see 3GPP TS 25.324 section 11.1

nit: Capital 'S'ee

@@ +853,5 @@
> +// UMTS Message Type
> +// see 3GPP TS 25.324 section 11.1
> +this.CB_UMTS_MESSAGE_TYPE_CBS      = 1;
> +this.CB_UMTS_MESSAGE_TYPE_SCHEDULE = 2;
> +this.CB_UMTS_MESSAGE_TYPE_CBS41    = 3;

What does CBS41 stand for?

::: dom/system/gonk/ril_worker.js
@@ +6919,5 @@
> +      for (let i = 0, j = 0; i < numOfPages; i++) {
> +        for (length = pageLengths[i]; length > 0; length--) {
> +          msg.data[j++] = Buf.readUint8();
> +        }
> +

pageLengths[i] isn't necessarily equal to CB_MAX_CONTENT_8BIT and there might be padding at the end of every page, right?

If yes, then shouldn't you get padding of every page before you read the length or the data of the next page?

@@ +6936,5 @@
> +      body += msg.body;
> +
> +      // Read off page length again. We might read a padding octet instead of
> +      // the expected `length` one at the last round, but that's fine here.
> +      Buf.readUint8();

ditto.
Attachment #681996 - Flags: review?(htsai)
Comment on attachment 682300 [details] [diff] [review]
Part 1: IDL changes : v5

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

I tried to understand the IDL with my non-existent knowledge of what cell broadcast is. It was generally good. It is commented and that is really appreciable.

sr+ with the comments below being taken care of. Especially, add a comment for nsIDOMMozCellBroadcast.

::: dom/cellbroadcast/interfaces/nsIDOMCellBroadcast.idl
@@ +4,5 @@
> +
> +#include "nsIDOMEventTarget.idl"
> +
> +[scriptable, builtinclass, uuid(06bf2607-cd01-4307-9063-b6eac13b4613)]
> +interface nsIDOMMozCellBroadcast : nsIDOMEventTarget

Please, add a comment explaining what this interface does. The general purpose of nsIDOMMozCelLBroadcast.

::: dom/cellbroadcast/interfaces/nsIDOMCellBroadcastMessage.idl
@@ +11,5 @@
> +{
> +  /**
> +   * Indication of the geographical area over which the Message Code is unique,
> +   * and the display mode. Valid values are: "cell-immediate", "plmn",
> +   * "location-area" and "cell".

"Vaid values" is incorrect, it's readonly so, hopefully, only valid values will be shown ;)
I would recommend using "Possible values are: " or "Values are: ".

nit: a line break before this line would also be better IMO.

@@ +17,5 @@
> +  readonly attribute DOMString geographicalScope;
> +
> +  /**
> +   * The Message Code differentiates between CBS messages from the same source
> +   * and type(i.e, with the same Message Identifier).

What does CBS means? Please, disambiguate this.

nit: "and type(ie, with" is missing a space between "type" and the parenthesis.

@@ +22,5 @@
> +   */
> +  readonly attribute unsigned short messageCode;
> +
> +  /**
> +   * Source and type of the CBS message.

This isn't clear. What is the link between messageId and "Source and type". Please, give more details. Also, |messageCode| mentions |messageId|. I think it would be better to have |messageId| before |messageCode| in the idl to improve the readability.

@@ +54,5 @@
> +  readonly attribute nsIDOMMozCellBroadcastEtwsInfo etws;
> +};
> +
> +/**
> + * ETWS(Earthquake and Tsunami Warning service) Primary Notification message

nit: missing a space before the parenthesis.

@@ +62,5 @@
> +interface nsIDOMMozCellBroadcastEtwsInfo : nsISupports
> +{
> +  /**
> +   * Warning type. Valid values are "earthquake", "tsunami",
> +   * "earthquake-tsunami", "test" and "other".

"Valid" should be replaced by "Possible" or removed.
Attachment #682300 - Flags: superreview?(mounir) → superreview+
Comment on attachment 681985 [details] [diff] [review]
Part 2: add navigator.mozCellBroadcast : v5

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

The patch looks generally good but there are a few stuff that need to be changed and I would like to see the new version of the patch.

::: dom/base/Navigator.cpp
@@ +1186,5 @@
> +
> +    mCellBroadcast = new CellBroadcast(window);
> +  }
> +
> +  nsCOMPtr<nsIDOMMozCellBroadcast> cbs(mCellBroadcast);

nit: nsCOMPtr<nsIDOMMozCellBroadcast> cbs = mCellBroadcast; is nicer to read ;)

::: dom/base/Navigator.h
@@ +100,5 @@
>  #endif
>                  , public nsIDOMMozNavigatorNetwork
>  #ifdef MOZ_B2G_RIL
>                  , public nsIMozNavigatorMobileConnection
> +                , public nsIDOMMozNavigatorCellBroadcast

We have two MOZ_B2G_RIL blocks... quite sad. No need to fix that in this patch but could you open a follow-up?

::: dom/base/nsDOMClassInfo.cpp
@@ +4107,5 @@
>    DOM_CLASSINFO_MAP_END
> +
> +  DOM_CLASSINFO_MAP_BEGIN(MozCellBroadcast, nsIDOMMozCellBroadcast)
> +     DOM_CLASSINFO_MAP_ENTRY(nsIDOMMozCellBroadcast)
> +  DOM_CLASSINFO_MAP_END

nsIDOMMozCellBroadcast inherits from nsIDOMEventTarget, right?
You need to add DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget).

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +5,5 @@
> +
> +#include "CellBroadcast.h"
> +#include "nsDOMClassInfo.h"
> +
> +#define RECEIVED_EVENT_NAME NS_LITERAL_STRING("received")

You don't need that for the moment, do you? Please, remove this if you don't happen to need it in another patch.

@@ +25,5 @@
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(CellBroadcast)
> +  NS_INTERFACE_MAP_ENTRY(nsIDOMMozCellBroadcast)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIDOMMozCellBroadcast)
> +  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(MozCellBroadcast)

You forgot nsIDOMEventTarget there.

@@ +38,5 @@
> +}
> +
> +void
> +CellBroadcast::Shutdown()
> +{

So, this method is useless but I see you use it in part 3.
It's usually recommended to not have code in a file part that is there only because it is used elsewhere. It makes the reading harder because you have to switch between multiple patches. The main idea of small chunks of patches is to actually have simple changes to review. Having to switch between different patches is worse than having only one huge patch. It's no big deal, but better to have that in mind ;)

@@ +41,5 @@
> +CellBroadcast::Shutdown()
> +{
> +}
> +
> +// nsIDOMMozCellBroadcast

What's that? Can you remove this line?

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_cellbroadcast_CellBroadcast_h__
> +#define mozilla_dom_cellbroadcast_CellBroadcast_h__

nit: should be mozilla_dom_CellBroadcast_h__

::: dom/cellbroadcast/src/Makefile.in
@@ +18,5 @@
> +
> +EXPORTS_NAMESPACES = mozilla/dom
> +
> +EXPORTS_mozilla/dom = \
> +  CellBroadcast.h \

Is that actually used somewhere?

::: mobile/android/installer/package-manifest.in
@@ +135,5 @@
>  @BINPATH@/components/dom_xul.xpt
>  @BINPATH@/components/dom_loadsave.xpt
> +#ifdef MOZ_B2G_RIL
> +@BINPATH@/components/dom_cellbroadcast.xpt
> +#endif

I doubt we support B2G on Android. No need for that.

::: mobile/xul/installer/package-manifest.in
@@ +159,5 @@
>  #ifdef MOZ_B2G_RIL
>  @BINPATH@/components/dom_telephony.xpt
>  @BINPATH@/components/dom_wifi.xpt
>  @BINPATH@/components/dom_system_gonk.xpt
> +@BINPATH@/components/dom_cellbroadcast.xpt

Ditto.

I'm actually quite surprised we have so many MOZ_B2G_RIL specific stuff in mobile/xul/. Which is something we do not even support anymore...
Attachment #681985 - Flags: review?(mounir) → review-
Comment on attachment 667673 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v3

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

r=me with the two comments applied.

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +72,5 @@
> +                                                                    false, aMessage);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = event->SetTrusted(true);
> +  NS_ENSURE_SUCCESS(rv, rv);

Those two lines and the followings should be replaced by:
return DispatchTrustedEvent(event);

::: dom/cellbroadcast/src/CellBroadcastEvent.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_cellbroadcast_CellBroadcastEvent_h
> +#define mozilla_dom_cellbroadcast_CellBroadcastEvent_h

nit: should be mozilla_dom_CellBroadcastEvent_h
Attachment #667673 - Flags: review?(mounir) → review+
Comment on attachment 681999 [details] [diff] [review]
Part 9: config via Settings API : v3

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

Amazing work, thank you, Vicamo! r=me but please address/respond my comments below.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +256,5 @@
>    // Read the 'time.nitz.automatic-update.enabled' setting to see if
>    // we need to adjust the system clock time and time zone by NITZ.
>    lock.get(kTimeNitzAutomaticUpdateEnabled, this);
>  
> +  // Read the Cell Broadcast Search List setting from DB.

Could you please say a little more about what Cell Broadcast Search List is used for?

::: dom/system/gonk/ril_consts.js
@@ +811,5 @@
>  
> +// See 3GPP TS 23.041 v11.2.0 section 9.4.1.2.2
> +this.CB_NON_MMI_SETTABLE_RANGES = [
> +  /*0x1000 - 0x107F*/4096,  4224,  /*0x1080 - 0x10FF*/4224,  4352,
> +  /*0x1112 - 0x1112*/4370,  4371,  /*0x111F - 0x111F*/4383,  4384,

Should be 0x111F - 0x112F, i.e. 4838 - 4400?

::: dom/system/gonk/ril_worker.js
@@ +810,2 @@
>    },
>    

Would you mind helping remove this whitespace in your patch? :)

@@ +4222,5 @@
>  
>      return options;
>    },
>  
> +  _mergeCellBroadcastConfig: function _mergeCellBroadcastConfig(list, from, to) {

I might be too picky about 'naming', sorry about that. However, you sometimes use singular 'Config' but sometimes use plural 'Configs.' ex. |cellBroadcastConfigs|, |mergedCellBroadcastConfig|, |_mergeCellBroadcastConfig()|, |_mergeAllCellBroadcastConfig()|, etc.

How about we examining the names again to make sure they are used appropriately? For example, isn't |_mergeCellBroadcastConfigs| better than |_mergeCellBroadcastConfig| ?

@@ +4251,5 @@
> +        // ...[f1]...(t1)[from] or ...[f1]...(t1)...[from]
> +        continue;
> +      }
> +
> +      // Have overlap or merge-able adjacency with [f1]...(t1). Will replace it

s/Will replace/Replace

@@ +4252,5 @@
> +        continue;
> +      }
> +
> +      // Have overlap or merge-able adjacency with [f1]...(t1). Will replace it
> +      // with [min(from, f1)]...(max(to, t1))

Place a period, please.

@@ +4268,5 @@
> +        // Can't have further merge-able adjacency. Return.
> +        return list;
> +      }
> +
> +      // Try merging possible next adjacent range

Don't forget a period.
"Try merging a possible next adjacent range."

@@ +4314,5 @@
> +    list.push(from);
> +    list.push(to);
> +
> +    return list;
> +  },

Great job! It's really complicated here. I spent some time figuring out what you were trying to do. I got it, eventually, and it looks great, though I'm wondering whether we could come up with cleaner or simpler implementation here. Right now I don't have any better idea ;-)

@@ +4379,5 @@
> +      // Match "12" or "12-34". The result will be ["12", "12", null] or
> +      // ["12-34", "12", "34"].
> +      result = range.match(/^(\d+)(?:-(\d+))?$/);
> +      if (!result) {
> +        throw "invalid format";

nit: s/invalid/Invalid

@@ +4385,5 @@
> +
> +      from = parseInt(result[1]);
> +      to = (result[2] != null) ? parseInt(result[2]) + 1 : from + 1;
> +      if (!this._checkCellBroadcastMMISettable(from, to)) {
> +        throw "invalid range";

Ditto.
Attachment #681999 - Flags: review?(htsai) → review+
Comment on attachment 682001 [details] [diff] [review]
Part 10: support EFcbmi and EFcbmir : v3

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

Good job! Just some nits below.

::: dom/system/gonk/ril_worker.js
@@ +1475,5 @@
>      });
>    },
>  
>    /**
> +   * Read EFcbmi (Cell Broadcast Message identifier selection)

nit: s/identifier/Identifier

@@ +1490,5 @@
> +      if (numIds) {
> +        let list = [], id;
> +        for (let i = 0; i < numIds; i++) {
> +          id = GsmPDUHelper.readHexOctet() << 8 | GsmPDUHelper.readHexOctet();
> +          // `Unused entries shall be set to 'FF FF'`

nit: I would like to see a period at the end of a line. Am I too picky? QQ

@@ +1546,5 @@
> +        let list = [], from, to;
> +        for (let i = 0; i < numIds; i++) {
> +          // `bytes one and two of each range identifier equal the lower value
> +          // of a cell broadcast range, bytes three and four equal the upper
> +          // value of a cell broadcast range`

nit: s/bytes/Bytes and place a period. please.

@@ +1594,5 @@
>     *
>     *  @param options
>     *         The 'options' object passed from RIL.iccIO
>     *  @param addCallback
>     *         The function should be invoked when the ICC record is processed 

Would you mind helping remove the whitespace in your patch? :)
Attachment #682001 - Flags: review?(htsai) → review+
Comment on attachment 681992 [details] [diff] [review]
Part 6: support base GSM CBS message : v3

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

::: dom/system/gonk/ril_consts.js
@@ +837,5 @@
>  
> +// GSM Cell Broadcast Message Identifiers
> +// see 3GPP TS 23.041 clause 9.4.1.2.2
> +this.CB_GSM_MESSAGEID_ETWS_BEGIN = 0x1100;
> +this.CB_GSM_MESSAGEID_ETWS_END   = 0x1108;

Hmmm, I think it would be better using 0x1108 here that makes consistency in range convention. Sorry about my changing thought.
Comment on attachment 682002 [details] [diff] [review]
Part 11: test cases : v3

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

I am holding off on reviewing test cases for now, because I would like to see my questions/concerns about implementation being addressed first. Also, since we are still waiting for review on qemu and hardware-ril changes, I am thinking of filing a separate bug for test cases, in order to speed up landing this feature. How do you think about this, Vicamo?
Attachment #682002 - Flags: review?(htsai)
Blocks: 813596
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #107)
> I am thinking of filing a separate bug for test cases, in order to speed up
> landing this feature. How do you think about this, Vicamo?

That's ok. But would you still review xpcshell tests? They're not related to emulator codes.
https://bugzilla.mozilla.org/show_bug.cgi?id=813596
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #108)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #107)
> > I am thinking of filing a separate bug for test cases, in order to speed up
> > landing this feature. How do you think about this, Vicamo?
> 
> That's ok. But would you still review xpcshell tests? They're not related to
> emulator codes.
> https://bugzilla.mozilla.org/show_bug.cgi?id=813596

Thanks for reminding this. We should keep xpcshell tests here. I'll do the review on that today.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #109)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #108)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #107)
> > > I am thinking of filing a separate bug for test cases, in order to speed up
> > > landing this feature. How do you think about this, Vicamo?
> > 
> > That's ok. But would you still review xpcshell tests? They're not related to
> > emulator codes.
> > https://bugzilla.mozilla.org/show_bug.cgi?id=813596
> 
> Thanks for reminding this. We should keep xpcshell tests here. I'll do the
> review on that today.
I typed too fast... 

I noticed there seem errors in implementation, though the test case looks great and it passed. I shall do the review after my concerns about the previous patches are resolved. Thanks.
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #105)
> Comment on attachment 682001 [details] [diff] [review]
> ::: dom/system/gonk/ril_worker.js
> @@ +1594,5 @@
> >     *
> >     *  @param options
> >     *         The 'options' object passed from RIL.iccIO
> >     *  @param addCallback
> >     *         The function should be invoked when the ICC record is processed 
> 
> Would you mind helping remove the whitespace in your patch? :)

I'd rather keep every change in the patch serves the same purpose. We can always fire another bug for this.
Keywords: dev-doc-needed
Blocks: 813893
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #100)
> Comment on attachment 681996 [details] [diff] [review]
> ::: dom/system/gonk/ril_worker.js
> @@ +6919,5 @@
> > +      for (let i = 0, j = 0; i < numOfPages; i++) {
> > +        for (length = pageLengths[i]; length > 0; length--) {
> > +          msg.data[j++] = Buf.readUint8();
> > +        }
> > +
> 
> pageLengths[i] isn't necessarily equal to CB_MAX_CONTENT_8BIT and there
> might be padding at the end of every page, right?

I would say this probably never happens, and Android assume the same thing here. However, since we don't need UMTS format for V1 and I have neither marionette test cases for it, I'll remove this patch from patch queue for now. See bug 813893.
Attached patch Part 1: IDL changes : v6 (obsolete) — Splinter Review
Address review comment #101. Already sr+.
Attachment #682300 - Attachment is obsolete: true
0) Massive changes come from HsinYi's request in previous Part 4 (see comment #95). In order not to broadcast messages to every content process message manager, I have to rewrite event handling in Voicemail/Telephony's way. That is, register a callback to RILContentHelper and request for message received events.

1) No longer need |mCellBroadcast->Shutdown()| because now it automatically calls unregisterCellBroadcastMsg(). See 0)
2) Use utility function CheckPermission() instead.
3) Use |nsCOMPtr<nsIDOMMozCellBroadcast>| instead. See 0)
4) Add NS_NewCellBroadcast(). See 0)
5) Don't override LOCAL_INCLUDES, need inclusion path for nsRadioInterfaceLayer.h
6) For scattered MOZ_B2G_RIL blocks, see bug 814556
7) Address comment #102
Attachment #681985 - Attachment is obsolete: true
Attachment #684922 - Flags: review?(mounir)
0) Rewrite using event gen & don't broadcast messages to all content process message managers. Basically this patch merges function of previous part 3 & 4, but the changes are completely different.
Attachment #667673 - Attachment is obsolete: true
Attachment #682317 - Attachment is obsolete: true
Attachment #684923 - Flags: review?(mounir)
Attachment #684923 - Flags: review?(htsai)
Address review comment #96. Already r+.
Attachment #681991 - Attachment is obsolete: true
1) Address review comment #98.
2) Add new xpcshell test case for Buf.readUint8Array() in latter patch
3) Correct test condition for GSM Cell Broadcast Message with UCS2 body string encoding in bug 813596. Thank HsinYi!
Attachment #681992 - Attachment is obsolete: true
Attachment #684929 - Flags: review?(htsai)
Rebase only. Already r+.
Attachment #672914 - Attachment is obsolete: true
Address review comment #104. Already r+.
Attachment #681999 - Attachment is obsolete: true
Address review comment #105. Already r+.
Attachment #682001 - Attachment is obsolete: true
Attached patch Part 9: xpcshell test cases : v4 (obsolete) — Splinter Review
1) Move marionette test cases to bug 813596
2) Add test cases for Buf.readUint8Array()
3) accommodate to name changes in part 7
Attachment #682002 - Attachment is obsolete: true
Attachment #684948 - Flags: review?(htsai)
Comment on attachment 681996 [details] [diff] [review]
Part 8: support UMTS CBS message : v3

Obsoleted for now and moved to bug 813893. See comment #112.
Attachment #681996 - Attachment is obsolete: true
Try run for 699b8313538f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=699b8313538f
Results (out of 310 total builds):
    success: 292
    warnings: 17
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vyang@mozilla.com-699b8313538f
Comment on attachment 684923 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v4

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

RIL part looks great, thanks! r=me

::: dom/system/gonk/ril_consts.js
@@ +936,5 @@
> +  "cell"
> +];
> +
> +// ETWS Warning-Type
> +// see 3GPP TS 23.041 clause 9.3.24

s/see/See
Attachment #684923 - Flags: review?(htsai) → review+
Comment on attachment 684929 [details] [diff] [review]
Part 5: support base GSM CBS message : v4

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

Good job!
Attachment #684929 - Flags: review?(htsai) → review+
Comment on attachment 684948 [details] [diff] [review]
Part 9: xpcshell test cases : v4

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

::: dom/system/gonk/tests/test_ril_worker_cellbroadcast.js
@@ +126,5 @@
> +  for (let group of [0x30, 0x80, 0xA0, 0xB0, 0xC0]) {
> +    for (let i = 0; i < 16; i++) {
> +      test_dcs_throws(group + i);
> +    }
> +  }

group should be [0x60, 0x70, 0xD0, 0xE0] here. It's not supposed to have the test passed. Seems some problem with the test case. Please re-examine it, thanks!

@@ +175,5 @@
> +
> +  // PDU_DCS_MSG_CODING_7BITS_ALPHABET
> +  test_data([PDU_DCS_MSG_CODING_7BITS_ALPHABET, null, false,
> +              [0]],
> +            ["@", null, null]);

Could you please add a comment explaining why msg.body should equal "@"? Thanks!
Attachment #684948 - Flags: review?(htsai)
Comment on attachment 684921 [details] [diff] [review]
Part 1: IDL changes : v6

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

::: dom/cellbroadcast/interfaces/nsIDOMNavigatorCellBroadcast.idl
@@ +6,5 @@
> +
> +interface nsIDOMMozCellBroadcast;
> +
> +[scriptable, uuid(010af082-22c5-471a-be21-0d738c50df67)]
> +interface nsIDOMMozNavigatorCellBroadcast : nsISupports

Sorry Vicamo, I should have seen that before but could you remove "DOM" from nsIDOMMozNavigatorCellBroadcast? That way, we will not add the interface name to the global scope (ie window.MozNavigatorCellBroadcast).
Comment on attachment 684922 [details] [diff] [review]
Part 2: add navigator.mozCellBroadcast : v6

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

r=me with the comments taken into account.

::: browser/installer/package-manifest.in
@@ +169,5 @@
>  @BINPATH@/components/dom_telephony.xpt
>  @BINPATH@/components/dom_wifi.xpt
>  @BINPATH@/components/dom_system_gonk.xpt
>  @BINPATH@/components/dom_icc.xpt
> +@BINPATH@/components/dom_cellbroadcast.xpt

Do we really enable MOZ_B2G_RIL for desktop? Even if we don't have a RIL on desktop?

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +48,5 @@
> +    aWindow :
> +    aWindow->GetCurrentInnerWindow();
> +
> +  nsRefPtr<mozilla::dom::CellBroadcast> cb =
> +    new mozilla::dom::CellBroadcast(innerWindow, ril);

ril? What's that?

@@ +53,5 @@
> +  cb.forget(aCellBroadcast);
> +
> +  return NS_OK;
> +}
> +

nit: remove the last empty line

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +8,5 @@
> +
> +#include "nsDOMEventTargetHelper.h"
> +#include "nsIDOMMozCellBroadcast.h"
> +#include "mozilla/Attributes.h"
> +#include "nsPIDOMWindow.h"

Do not #include "nsPIDOMWindow.h"
It seems like you could do:
class nsPIDOMWindow;

::: dom/cellbroadcast/src/Makefile.in
@@ +19,5 @@
> +
> +EXPORTS_NAMESPACES = mozilla/dom
> +
> +EXPORTS_mozilla/dom = \
> +  CellBroadcast.h \

I'm not sure if you answered that: is this used somewhere? (the fact that the header is exported)
Actually, it's even odd to have CellBroadcast.h exported *and* have NS_NewCellBroadCast(). With the exported header, callers could just use the ctor.
Attachment #684922 - Flags: review?(mounir) → review+
Comment on attachment 684923 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v4

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

DOM code shouldn't have platform-specific code like that. I know that CellBroadcast is B2G-only for the moment but unless you have a very strong argument, I do not think this really counts. You should whether abstract this or find a way to move that code somewhere else, unfortunately.

IIRC, the DOM code didn't contain B2G-RIL specific code before, right?

::: dom/base/nsDOMClassInfo.cpp
@@ +4124,1 @@
>  #endif

nit: could you add // MOZ_B2G_RIL, it will make it easier later to know what is added without opening the file to check what the #ifdef is actually checking.
Attachment #684923 - Flags: review?(mounir) → review-
(In reply to Mounir Lamouri (:mounir) from comment #128)
> Comment on attachment 684921 [details] [diff] [review]
> ::: dom/cellbroadcast/interfaces/nsIDOMNavigatorCellBroadca
> > +interface nsIDOMMozNavigatorCellBroadcast : nsISupports
> 
> Sorry Vicamo, I should have seen that before but could you remove "DOM" from
> nsIDOMMozNavigatorCellBroadcast? That way, we will not add the interface
> name to the global scope (ie window.MozNavigatorCellBroadcast).

Sure, but then we can't tell internally used interfaces from DOM ones. Is this a new naming rule? Should I also file a bug to rename all RIL related interfaces?
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #131)
> (In reply to Mounir Lamouri (:mounir) from comment #128)
> > Comment on attachment 684921 [details] [diff] [review]
> > ::: dom/cellbroadcast/interfaces/nsIDOMNavigatorCellBroadca
> > > +interface nsIDOMMozNavigatorCellBroadcast : nsISupports
> > 
> > Sorry Vicamo, I should have seen that before but could you remove "DOM" from
> > nsIDOMMozNavigatorCellBroadcast? That way, we will not add the interface
> > name to the global scope (ie window.MozNavigatorCellBroadcast).
> 
> Sure, but then we can't tell internally used interfaces from DOM ones. Is
> this a new naming rule? Should I also file a bug to rename all RIL related
> interfaces?

Actually, you should consider interfaces that extend nsIDOMNavigator to be internal interfaces. The web content doesn't have to know there are multiple interfaces, for it, it's simply |Navigator| that has attributes. The fact that |Navigator| is a mix of multiple interfaces is an implementation detail.
This is a bit tricky and we simply have been quite loose about that in the past.

Sorry again :( Hopefully, this is mostly going to be a sed call + a rename.
(In reply to Mounir Lamouri (:mounir) from comment #129)
> Comment on attachment 684922 [details] [diff] [review]
> ::: browser/installer/package-manifest.in
> @@ +169,5 @@
> >  @BINPATH@/components/dom_telephony.xpt
> >  @BINPATH@/components/dom_wifi.xpt
> >  @BINPATH@/components/dom_system_gonk.xpt
> >  @BINPATH@/components/dom_icc.xpt
> > +@BINPATH@/components/dom_cellbroadcast.xpt
> 
> Do we really enable MOZ_B2G_RIL for desktop? Even if we don't have a RIL on
> desktop?

Ok, I'll remove it.

> ::: dom/cellbroadcast/src/Makefile.in
> @@ +19,5 @@
> > +
> > +EXPORTS_NAMESPACES = mozilla/dom
> > +
> > +EXPORTS_mozilla/dom = \
> > +  CellBroadcast.h \
> 
> I'm not sure if you answered that: is this used somewhere? (the fact that
> the header is exported)

This header file is used in dom/base/Navigator.cpp and has to be exported here in order not to pollute dom-config.mk.

> Actually, it's even odd to have CellBroadcast.h exported *and* have
> NS_NewCellBroadCast(). With the exported header, callers could just use the
> ctor.

We can hide some implementation details from the caller. For RIL stuff, an additional interface nsIRILContentHelper is involved in instantiation process. We'll have to include nsRadioInterfaceLayer.h in dom/base/Navigation.cpp if NS_NewCellBroadCast() should be removed here.
(In reply to Mounir Lamouri (:mounir) from comment #130)
> Comment on attachment 684923 [details] [diff] [review]
> -----------------------------------------------------------------
> DOM code shouldn't have platform-specific code like that. I know that
> CellBroadcast is B2G-only for the moment but unless you have a very strong
> argument, I do not think this really counts. You should whether abstract
> this or find a way to move that code somewhere else, unfortunately.
> 
> IIRC, the DOM code didn't contain B2G-RIL specific code before, right?

Actually, there are. Both MozTelephony and MozVoicemail are implemented in this way. And, I can fully understand your concern, that's why the whole dom/cellbroadcast folder is bounded in MOZ_B2G_RIL. Maybe this can ease you a little bit? Besides, I have also a private branch for rewrite all these RILContentHelper stuff in IPDL[1], and I can guarantee you that's much cleaner but much much much complex than the way it is now.

[1]: https://github.com/vicamo/mozilla-central/tree/bugzilla/778093/ipdl
Comment on attachment 684947 [details] [diff] [review]
Part 8: support EFcbmi and EFcbmir : v4

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

Some typos.

::: dom/system/gonk/ril_worker.js
@@ +1490,5 @@
> +        this.getCBMIR();
> +      } else {
> +        this.cellBroadcastConfigs.CBMIR = null;
> +      }
> +      this._mergeAllCellBroadcastConfig();

s/_mergeAllCellBroadcastConfig/_mergeAllCellBroadcastConfigs

@@ +1540,5 @@
> +
> +      if (DEBUG) {
> +        debug("CBMI: " + JSON.stringify(this.cellBroadcastConfigs.CBMI));
> +      }
> +      this._mergeAllCellBroadcastConfig();

s/_mergeAllCellBroadcastConfig/_mergeAllCellBroadcastConfigs

@@ +1604,5 @@
> +    }
> +
> +    function onerror() {
> +      this.cellBroadcastConfigs.CBMIR = null;
> +      this._mergeAllCellBroadcastConfig();

s/_mergeAllCellBroadcastConfig/_mergeAllCellBroadcastConfigs
Comment on attachment 684923 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v4

Per discussion in IRC, just request review again.
Attachment #684923 - Flags: review- → review?
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #135)
> Comment on attachment 684947 [details] [diff] [review]

Thank you :) Then we should populate emulator SIM card with the two file in order to test it.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #133)
> (In reply to Mounir Lamouri (:mounir) from comment #129)
> > Comment on attachment 684922 [details] [diff] [review]
> > ::: browser/installer/package-manifest.in
> > @@ +169,5 @@
> > >  @BINPATH@/components/dom_telephony.xpt
> > >  @BINPATH@/components/dom_wifi.xpt
> > >  @BINPATH@/components/dom_system_gonk.xpt
> > >  @BINPATH@/components/dom_icc.xpt
> > > +@BINPATH@/components/dom_cellbroadcast.xpt
> > 
> > Do we really enable MOZ_B2G_RIL for desktop? Even if we don't have a RIL on
> > desktop?
> 
> Ok, I'll remove it.

I would prefer if you could file a follow-up for this instead of removing it in this patch.
(In reply to Mounir Lamouri (:mounir) from comment #138)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #133)
> > (In reply to Mounir Lamouri (:mounir) from comment #129)
> > > Comment on attachment 684922 [details] [diff] [review]
> > > ::: browser/installer/package-manifest.in
> > > @@ +169,5 @@
> > > >  @BINPATH@/components/dom_telephony.xpt
> > > >  @BINPATH@/components/dom_wifi.xpt
> > > >  @BINPATH@/components/dom_system_gonk.xpt
> > > >  @BINPATH@/components/dom_icc.xpt
> > > > +@BINPATH@/components/dom_cellbroadcast.xpt
> > > 
> > > Do we really enable MOZ_B2G_RIL for desktop? Even if we don't have a RIL on
> > > desktop?
> > 
> > Ok, I'll remove it.
> 
> I would prefer if you could file a follow-up for this instead of removing it
> in this patch.

MOZ_B2G_RIL is a configure option that allows you to build desktop firefox with B2G RIL components. The main reason for it might came from developing RIL function without installing B2G to the device but just redirect the traffic by using rilproxy. I think nobody depends on this feature nowadays and we can do some more clean-ups. What about amending bug 814556 into a more generic clean-up bug for MOZ_B2G_RIL code snippets?
Address review comment #128, rename nsIDOMNavigatorCellBroadcast to nsINavigatorCellBroadcast.
Attachment #684921 - Attachment is obsolete: true
Attachment #685516 - Flags: superreview?(mounir)
Address review comment #129. "ril" parameter in CellBroadcast's constructor in previous revision should have been moved to part 3. Already r+.
Attachment #684922 - Attachment is obsolete: true
Address review comment #129, 130
Attachment #684923 - Attachment is obsolete: true
Attachment #684923 - Flags: review?
Attachment #685520 - Flags: review?(mounir)
Address comment #135. New emulator SIM file CBMI & CBMIR added in my external/qemu pull request[1] to verify this problem. Thanks Jose!

[1]: https://github.com/mozilla-b2g/platform_external_qemu/pull/5
Attachment #684947 - Attachment is obsolete: true
Address review comment #127. Somehow the do_check_throw() was actually broken and is also fixed in this patch.
Attachment #684948 - Attachment is obsolete: true
Attachment #685523 - Flags: review?(htsai)
Comment on attachment 685523 [details] [diff] [review]
Part 9: xpcshell test cases : v5

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

:D

::: dom/system/gonk/tests/header_helpers.js
@@ +165,1 @@
>    }

Really good, thank you!
Attachment #685523 - Flags: review?(htsai) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #139)
> MOZ_B2G_RIL is a configure option that allows you to build desktop firefox
> with B2G RIL components. The main reason for it might came from developing
> RIL function without installing B2G to the device but just redirect the
> traffic by using rilproxy. I think nobody depends on this feature nowadays
> and we can do some more clean-ups. What about amending bug 814556 into a
> more generic clean-up bug for MOZ_B2G_RIL code snippets?

Sounds good to me. You don't even have to do anything if this thing is needed. I was just pointing out that because it looked odd to me.
Comment on attachment 685516 [details] [diff] [review]
Part 1: IDL changes : v7

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

sr=me, assuming you only changed nsINavigatorCellBroadcast.idl
Attachment #685516 - Flags: superreview?(mounir) → superreview+
Certification requirement - granting an exception here and moving into the C2 milestone.
Target Milestone: B2G C1 (to 19nov) → B2G C2 (20nov-10dec)
Comment on attachment 685520 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v5

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

I do not understand why you need this CellBroadcastCallback thing. According to our IRC discussion bent might know the reasons of this. I will let him decide if that is actually needed. 

r=me for the changes outside of dom/system/gonk with all the nits fixed and with bent's comment regarding CellBroadcastCallback applied.

::: dom/cellbroadcast/src/CellBroadcast.cpp
@@ +21,5 @@
> + */
> +
> +class CellBroadcastCallback : public nsIRILCellBroadcastCallback
> +{
> +  CellBroadcast* mCellBroadcast;

nit: add |private:|, better to be explicit.

@@ +35,5 @@
> +
> +CellBroadcastCallback::CellBroadcastCallback(CellBroadcast* aCellBroadcast)
> +  : mCellBroadcast(aCellBroadcast)
> +{
> +  NS_ASSERTION(mCellBroadcast, "Null pointer!");

MOZ_ASSERT

@@ +68,4 @@
>  {
>    BindToOwner(aWindow);
> +
> +  mCallback = new CellBroadcastCallback(this);

I think some compilers will warn because we try to use |this| in the ctor. Though, we only keep a ref of the pointer, we don't actually use the object so it's fine IMO.

@@ +71,5 @@
> +  mCallback = new CellBroadcastCallback(this);
> +
> +  nsresult rv = mRIL->RegisterCellBroadcastCallback(mCallback);
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed registering Cell Broadcast callback with RIL");

NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed registering Cell Broadcast callback with RIL");

@@ +76,5 @@
> +  }
> +
> +  rv = mRIL->RegisterCellBroadcastMsg();
> +  if (NS_FAILED(rv)) {
> +    NS_WARNING("Failed registering Cell Broadcast messages with RIL");

ditto

@@ +83,5 @@
> +
> +CellBroadcast::~CellBroadcast()
> +{
> +  if (mRIL && mCallback) {
> +    mRIL->UnregisterCellBroadcastCallback(mCallback);

|mRIL| and |mCallback| can't be null, right?
If that's the case, please add a MOZ_ASSERT check.

@@ +89,5 @@
>  }
>  
>  NS_IMPL_EVENT_HANDLER(CellBroadcast, received)
>  
> +// nsIRILCellBroadcastCallback methods

I think this comment is confusing given that CellBroadcast doesn't inherit from nsIRILCellBroadcast.

@@ +101,5 @@
> +  nsCOMPtr<nsIDOMMozCellBroadcastEvent> ce = do_QueryInterface(event);
> +  nsresult rv = ce->InitMozCellBroadcastEvent(NS_LITERAL_STRING("received"),
> +                                              true, false, aMessage);
> +  if (NS_FAILED(rv)) {
> +    return rv;

NS_ENSURE_SUCCESS(rv, rv);

@@ +105,5 @@
> +    return rv;
> +  }
> +
> +  bool dummy;
> +  return DispatchEvent(event, &dummy);

Don't you want to use |DispatchTrustedEvent| instead?

::: dom/cellbroadcast/src/CellBroadcast.h
@@ +23,5 @@
>  {
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIDOMMOZCELLBROADCAST
> +  NS_DECL_NSIRILCELLBROADCASTCALLBACK

The class isn't inheriting from nsIRillCellBroadcast. I think it would be better to explicitly list the methods that you want to declare.

Alternatively, you could make CellBroadcast to inherits from nsIRILCellBroadcastCallback. I don't know what would be the best idea. Maybe Ben has an opinion.
Attachment #685520 - Flags: review?(mounir)
Attachment #685520 - Flags: review?(bent.mozilla)
Attachment #685520 - Flags: review+
(In reply to Mounir Lamouri (:mounir) from comment #149)
> Comment on attachment 685520 [details] [diff] [review]
> -----------------------------------------------------------------
> 
> I do not understand why you need this CellBroadcastCallback thing. According
> to our IRC discussion bent might know the reasons of this. I will let him
> decide if that is actually needed. 

Can we move these discussion to bug 815526? All of design here comes from other existing RIL elements Telephony/Voicemail/... They may be not perfect, but they've been there for a while and are functioning well. So IMHO, just let this go and let's file new bugs to correct them.

> r=me for the changes outside of dom/system/gonk with all the nits fixed and
> with bent's comment regarding CellBroadcastCallback applied.
> 
> ::: dom/cellbroadcast/src/CellBroadcast.cpp
> @@ +68,4 @@
> >  {
> >    BindToOwner(aWindow);
> > +
> > +  mCallback = new CellBroadcastCallback(this);
> 
> I think some compilers will warn because we try to use |this| in the ctor.
> Though, we only keep a ref of the pointer, we don't actually use the object
> so it's fine IMO.

Thank you. I had also noticed this problem. But I also wish we have the same code layout at least between all these RIL related elements. I've seen several different ways to do IPC and they're really confusing for me. Maybe that's just the evidence that shows human's great creativity.

> @@ +83,5 @@
> > +
> > +CellBroadcast::~CellBroadcast()
> > +{
> > +  if (mRIL && mCallback) {
> > +    mRIL->UnregisterCellBroadcastCallback(mCallback);
> 
> |mRIL| and |mCallback| can't be null, right?
> If that's the case, please add a MOZ_ASSERT check.

True. We should assert instead.

> @@ +89,5 @@
> >  }
> >  
> >  NS_IMPL_EVENT_HANDLER(CellBroadcast, received)
> >  
> > +// nsIRILCellBroadcastCallback methods
> 
> I think this comment is confusing given that CellBroadcast doesn't inherit
> from nsIRILCellBroadcast.

How about prefix with "Forwarded"?

> @@ +105,5 @@
> > +    return rv;
> > +  }
> > +
> > +  bool dummy;
> > +  return DispatchEvent(event, &dummy);
> 
> Don't you want to use |DispatchTrustedEvent| instead?

Oops.

> ::: dom/cellbroadcast/src/CellBroadcast.h
> @@ +23,5 @@
> >  {
> >  public:
> >    NS_DECL_ISUPPORTS
> >    NS_DECL_NSIDOMMOZCELLBROADCAST
> > +  NS_DECL_NSIRILCELLBROADCASTCALLBACK
> 
> The class isn't inheriting from nsIRillCellBroadcast. I think it would be
> better to explicitly list the methods that you want to declare.
> 
> Alternatively, you could make CellBroadcast to inherits from
> nsIRILCellBroadcastCallback. I don't know what would be the best idea. Maybe
> Ben has an opinion.

How about some comments outlining the purpose & design of nsIRILCellBroadcastCallback before the line?
Comment on attachment 685520 [details] [diff] [review]
Part 3: add MozCellBroadcastEvent and complete onreceived event handling : v5

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

I really don't know what I'm supposed to be looking for here. Please provide some more info and re-request review, otherwise file a followup or something.
Attachment #685520 - Flags: review?(bent.mozilla)
Whiteboard: [LOE:M] [WebAPI:P0] [vivo-lab] → [LOE:M] [WebAPI:P0] [vivo-lab] [needs new patch]
Rebase only
Attachment #685518 - Attachment is obsolete: true
Address Mounir's comment #149.

Hi Bent,

Mounir has much concern about the nsIRILCellBroadcastCallback thing. He wonders why we cannot make CellBroadcast inherit that interface directly, instead of having a mCallback member. I know from bug 775997 that that we can't pass a dom object directly to RadioInterfaceLayer, and the solution was provided by you. After some more digging, I find the design came from your previous work in bug 674726 for WebTelephony, also some renames in bug 711671.

I've also created bug 815526 for deprecating RILContentHelper. If you don't mind, please r+ this patch and let's discuss it there.
Attachment #685520 - Attachment is obsolete: true
Attachment #687604 - Flags: review?(bent.mozilla)
rebase only
Attachment #684924 - Attachment is obsolete: true
rebase only
Attachment #685522 - Attachment is obsolete: true
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #153)
> I know from bug 775997 that that we
> can't pass a dom object directly to RadioInterfaceLayer

Well, not really. The issue there was that SMS has two request interfaces, one for "internal" use and one for the DOM, but both were implemented by the same object. mrbkap laid the problem out specifically in bug 775997 comment 51.

It looks like you've created something similar here, right? MozCellBroadcast, the object, has nsIDOMMozCellBroadcast in its classinfo, but not nsIRILCellBroadcastCallback, even though nsIRILContentHelper takes arguments of type nsIRILCellBroadcastCallback. Since it's a DOM object it will refuse to QI to any interface not on its classinfo map.

Given that, yes, you'll need a forwarding object. However, let's try to remove this craziness in bug 815526.
Attachment #687604 - Flags: review?(bent.mozilla) → review+
Whiteboard: [LOE:M] [WebAPI:P0] [vivo-lab] [needs new patch] → [LOE:M] [WebAPI:P0] [vivo-lab]
Try run for 699b8313538f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=699b8313538f
Results (out of 311 total builds):
    success: 292
    warnings: 17
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vyang@mozilla.com-699b8313538f
Blocks: 865970
Blocks: 879630
Blocks: 874805
You need to log in before you can comment on or make changes to this bug.