Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 12 years ago

See 3GPP TS 23.041 [http://www.3gpp.org/ftp/Specs/html-info/23041.htm ]

	Philipp von Weitershausen [:philikon]
	Comment 1 • 12 years ago

Vicamo, can you please outline what this is needed for and whether it blocks any feature in v1?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 2 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 3 • 12 years ago

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.

	Philipp von Weitershausen [:philikon]
	Comment 4 • 12 years ago

Thanks, Vicamo. I talked to Andreas, apparently this feature is necessary for device certification. Nom'ing for basecamp.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 5 • 12 years ago

for public interfaces, add nsIDOMMozSmsCbMessage/nsIDOMMozSmsCbEvent and new APIs to SmsManager

	Dietrich Ayala (:dietrich)
	Comment 8 • 12 years ago

Per comment #4.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 12 years ago

Mostly done, but need test cases to ensure basic functions work as expected.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 12 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 15 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 16 • 12 years ago

See also https://github.com/mozilla-b2g/gaia/issues/4559

	Andrew Overholt [:overholt]
	Comment 17 • 12 years ago

Can we get an update on progress here?  Thanks.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 18 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 19 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 12 years ago

Gonk pull requests for supporting Cell Broadcast in emulator:
https://github.com/mozilla-b2g/platform_hardware_ril/pull/3
https://github.com/mozilla-b2g/platform_external_qemu/pull/5

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 21 • 12 years ago

For now, we supports GSM & ETWS Cell Broadcast messages only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 24 • 12 years ago

Implementation of CellBroadcastMessage is in later patch written in javascript.

	Mounir Lamouri (:mounir)
	Comment 29 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 30 • 12 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 31 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 32 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 33 • 12 years ago

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?

	Mounir Lamouri (:mounir)
	Comment 34 • 12 years ago

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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 35 • 12 years ago

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

	Philipp von Weitershausen [:philikon]
	Comment 36 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 37 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 38 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 39 • 12 years ago

Address review comment #29

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 40 • 12 years ago

address review comment #33

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 41 • 12 years ago

Comment on attachment 665176 [details] [diff] [review]
Part 3: add switch perference dom.cellbroadcast.enabled

Obsolete as suggested in review comment #30

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 42 • 12 years ago

Address review comment #32, but still keep type-casting check in dom/cellbroadcast/src/CellBroadcast.cpp

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 43 • 12 years ago

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 .

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 47 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 48 • 12 years ago

CellBroadcastMessage.timestamp shouldn't have `implicit_jscontext` mark because we're going to implement it in javascript.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 51 • 12 years ago

re-exported in hg format

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 52 • 12 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 53 • 12 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 54 • 12 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 55 • 12 years ago

rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 62 • 12 years ago

Debug patch for Gecko

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 63 • 12 years ago

Debug patch for Gecko Marionette tests.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 64 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 65 • 12 years ago

I'm going to have a try on Galaxy S2, which uses Broadcom chip.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 66 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 67 • 12 years ago

(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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 68 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 69 • 12 years ago

rebase & add to PermissionTable

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 70 • 12 years ago

1. fix no ETWS info in GSM messages with ETWS messageIds
2. fix invalid body value when dcs is 8-bits

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 71 • 12 years ago

1. fix no ETWS info in GSM messages with ETWS messageIds
2. fix invalid body value when dcs is 8-bits

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 72 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 73 • 12 years ago

”this.cell“ had been moved to “this.voiceRegistrationState.cell”

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 74 • 12 years ago

extract UMTS CB data reading parts to a function for unit testing

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 75 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 76 • 12 years ago

update cb config upon EFcbmi & EFcbmir read event

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 77 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 78 • 12 years ago

Current status: have to fix Gaia permission, and cooperate with WIP patch in bug 802121.

	Alex Keybl [:akeybl]
	Comment 79 • 12 years ago

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.

	Dietrich Ayala (:dietrich)
	Comment 80 • 12 years ago

Vicamo, any update on the status of this work?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 81 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 82 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 83 • 12 years ago

Rebase only

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 84 • 12 years ago

1) rebase
2) add Message-Class

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 85 • 12 years ago

1) rebase
2) fix undefined messageClass DOM attribute for ETWS messages

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 86 • 12 years ago

1) rebase
2) Message-Class

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 87 • 12 years ago

rebase only

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 88 • 12 years ago

Move complex calculation into ril_worker

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 89 • 12 years ago

Move complex calculation into ril_worker

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 90 • 12 years ago

1) Add Message-Class tests
2) Don't wait for MobileConnection
3) Fix test cases after all the changes made

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 91 • 12 years ago

Oops, previous patch is a completely wrong one. Thanks HsinYi.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 92 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 93 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 95 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 96 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 97 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 98 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 99 • 12 years ago

Comment on attachment 672914 [details] [diff] [review]
Part 7: support GSM CBS message paging : v2

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

Looks great.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 100 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 101 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 102 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 103 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 104 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 105 • 12 years ago

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? :)

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 106 • 12 years ago

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.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 107 • 12 years ago

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?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 108 • 12 years ago

(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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 109 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 110 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 111 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 112 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 113 • 12 years ago

Address review comment #101. Already sr+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 114 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 115 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 116 • 12 years ago

Address review comment #96. Already r+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 117 • 12 years ago

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!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 118 • 12 years ago

Rebase only. Already r+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 119 • 12 years ago

Address review comment #104. Already r+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 120 • 12 years ago

Address review comment #105. Already r+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 121 • 12 years ago

1) Move marionette test cases to bug 813596
2) Add test cases for Buf.readUint8Array()
3) accommodate to name changes in part 7

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 122 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 123 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=699b8313538f

	Mozilla RelEng Bot
	Comment 124 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 125 • 12 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 126 • 12 years ago

Comment on attachment 684929 [details] [diff] [review]
Part 5: support base GSM CBS message : v4

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

Good job!

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 127 • 12 years ago

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!

	Mounir Lamouri (:mounir)
	Comment 128 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 129 • 12 years ago

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.

	Mounir Lamouri (:mounir)
	Comment 130 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 131 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 132 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 133 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 134 • 12 years ago

(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

	José Antonio Olivera Ortega [:jaoo]
	Comment 135 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 136 • 12 years ago

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.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 137 • 12 years ago

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

	Mounir Lamouri (:mounir)
	Comment 138 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 139 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 140 • 12 years ago

Address review comment #128, rename nsIDOMNavigatorCellBroadcast to nsINavigatorCellBroadcast.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 141 • 12 years ago

Address review comment #129. "ril" parameter in CellBroadcast's constructor in previous revision should have been moved to part 3. Already r+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 142 • 12 years ago

Address review comment #129, 130

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 143 • 12 years ago

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

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 144 • 12 years ago

Address review comment #127. Somehow the do_check_throw() was actually broken and is also fixed in this patch.

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 145 • 12 years ago

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!

	Mounir Lamouri (:mounir)
	Comment 146 • 12 years ago

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