B2G RIL: use ipdl as IPC in MozCellBroadcast

RESOLVED FIXED in 2.1 S5 (26sep)

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: vicamo, Assigned: bevis)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 49 obsolete attachments)

13.59 KB, patch
bevis
: review+
Details | Diff | Splinter Review
24.11 KB, patch
bevis
: review+
Details | Diff | Splinter Review
36.28 KB, patch
bevis
: review+
Details | Diff | Splinter Review
37.45 KB, patch
bevis
: review+
Details | Diff | Splinter Review
12.81 KB, patch
bevis
: review+
Details | Diff | Splinter Review
7.12 KB, patch
bevis
: review+
Details | Diff | Splinter Review
32.33 KB, patch
bevis
: review+
Details | Diff | Splinter Review
No description provided.
Reporter

Updated

6 years ago
Blocks: 865403
Reporter

Comment 2

6 years ago
Should WebIDL-ize first then we can create native message instance in CellBroadcast IPC child.
No longer blocks: 865403
Depends on: 865403
Reporter

Updated

6 years ago
Blocks: 865403
No longer depends on: 865403
Reporter

Updated

6 years ago
Depends on: 906398
Reporter

Updated

6 years ago
No longer blocks: 865403
Reporter

Updated

6 years ago
Component: DOM: Device Interfaces → RIL
Product: Core → Boot2Gecko
Reporter

Updated

5 years ago
Blocks: 906398
Depends on: 1020108
No longer depends on: 906398
Per offline discussion, I'd like to take this bug with bug 906398.
Assignee: vyang → btseng
IPC Connection is not verified yet.
Depends on: 1055994
Attachment #8472264 - Attachment is obsolete: true
Attachment #8472265 - Attachment is obsolete: true
Attachment #8473519 - Attachment is obsolete: true
Attachment #8475813 - Attachment is obsolete: true
Need to figure out why we got failure when running mochitest/JSRefTest in Android platform.
https://tbpl.mozilla.org/?tree=Try&rev=4bf5f93ae75d
(In reply to Bevis Tseng [:bevistseng] (btseng@mozilla.com) from comment #13)
> Need to figure out why we got failure when running mochitest/JSRefTest in
> Android platform.
> https://tbpl.mozilla.org/?tree=Try&rev=4bf5f93ae75d

To pass these tests cases in Android,
dom_cellbroadcast.xpt has to be included in package-manifest.in for android if addExternalIface('MozCellBroadcastMessage') is added in dom/bindings/Binding.conf.

To prevent double effort for implementation/review of these modification in bug 906398, I decide to move nsIDOMMozCellBroadcastMessage.idl to webidl in Patch Part 2 instead.
Duplicate of this bug: 906398
Reporter

Updated

5 years ago
Depends on: 1064231
Part 1 is to define a CellBrodacastService with a gonk back-end implementation to ensure the functionality in non-oop mode.
The IPC implementation will be addressed later in Part 4.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8478172 - Attachment is obsolete: true
Attachment #8486892 - Flags: review?(vyang)
Part 2 is to deprecate the old CellBroadcastProvider with the new CellBroadcastService define in Part 1.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8478173 - Attachment is obsolete: true
Attachment #8486894 - Flags: review?(vyang)
Attachment #8486892 - Attachment description: Patch Part 1 v1: Create new CellBroadcastService. r=vyang → Patch Part 1 v1: Create new CellBroadcastService.
Attachment #8486894 - Attachment description: Part 2 v1: Replace CellBroadcastProvider with CellBroadcastService. → Patch Part 2 v1: Replace CellBroadcastProvider with CellBroadcastService.
Part 3 is to Move MozCellBroadcastMessage to webidl implementation.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8478174 - Attachment is obsolete: true
Attachment #8486896 - Flags: review?(vyang)
Part 4 is to add the IPC implementation of CellBroadcastService.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8478175 - Attachment is obsolete: true
Attachment #8486898 - Flags: review?(vyang)
Part 5 is to have CellBroadcast enabled without MOZ_B2G_RIL flag.
Attachment #8478176 - Attachment is obsolete: true
Attachment #8486899 - Flags: review?(vyang)
Replace correct patch for Part 4.
Attachment #8486898 - Attachment is obsolete: true
Attachment #8486898 - Flags: review?(vyang)
Attachment #8486902 - Flags: review?(vyang)
Update correct patch for Part 3.
Attachment #8486896 - Attachment is obsolete: true
Attachment #8486896 - Flags: review?(vyang)
Attachment #8486903 - Flags: review?(vyang)
Reporter

Updated

5 years ago
Attachment #8486899 - Flags: review?(vyang)
Attachment #8486899 - Flags: review?(bugs)
Attachment #8486899 - Flags: review+
Reporter

Comment 24

5 years ago
Comment on attachment 8486892 [details] [diff] [review]
Patch Part 1 v1: Create new CellBroadcastService.

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

::: dom/cellbroadcast/gonk/CellBroadcastService.js
@@ +31,5 @@
> +function debug(s) {
> +  dump("CellBroadcastService: " + s);
> +}
> +
> +function CellBroadcastMessage(aServiceId,

These classes were added only to be removed in part 3. A little bit confusing.

@@ +156,5 @@
> +  unregisterListener: function(aListener) {
> +    let index = this._listeners.indexOf(aListener);
> +
> +    if (index < 0) {
> +      return Cr.NS_ERROR_FAILURE;

nit: please use NS_ERROR_UNEXPECTED as well. The exception is thrown because there is supposed to be an existing listener to be removed but there is not. Somehow there must have been unregistered sometime before so the second call is unexpected.
Attachment #8486892 - Flags: review?(vyang) → review+
Reporter

Comment 25

5 years ago
Comment on attachment 8486894 [details] [diff] [review]
Patch Part 2 v1: Replace CellBroadcastProvider with CellBroadcastService.

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

::: dom/cellbroadcast/interfaces/nsICellBroadcastService.idl
@@ +6,5 @@
>  
>  interface nsIDOMMozCellBroadcastMessage;
> +
> +[scriptable, uuid(e70bb4cc-2297-11e4-aecd-9f8fb3d2b646)]
> +interface nsICellBroadcastListener : nsISupports

nit: you don't have to change interface uuid with only moving it around.
Attachment #8486894 - Flags: review?(vyang) → review+
Reporter

Comment 26

5 years ago
Comment on attachment 8486903 [details] [diff] [review]
Patch Part 3 v1: Move MozCellBroadcastMessage to webidl implementation.

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

::: dom/webidl/MozCellBroadcastMessage.webidl
@@ +17,5 @@
> +   * and the display mode.
> +   *
> +   * Possible values are: "cell-immediate", "plmn", "location-area" and "cell".
> +   */
> +  readonly attribute DOMString? gsmGeographicalScope;

Please turn it into an enum.

@@ +46,5 @@
> +  /**
> +   * Possible values are "normal", "class-0", "class-1", "class-2", "class-3",
> +   * "user-1", and "user-2".
> +   */
> +  readonly attribute DOMString messageClass;

enum, please.

@@ +61,5 @@
> +
> +  /**
> +   * Service Category.
> +   */
> +  readonly attribute long cdmaServiceCategory;

long? cdmaServiceCategory;

@@ +71,5 @@
> +  /**
> +   * Warning type. Possible values are "earthquake", "tsunami",
> +   * "earthquake-tsunami", "test" and "other".
> +   */
> +  readonly attribute ByteString? warningType;

enum.
Attachment #8486903 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #24)
> > +  unregisterListener: function(aListener) {
> > +    let index = this._listeners.indexOf(aListener);
> > +
> > +    if (index < 0) {
> > +      return Cr.NS_ERROR_FAILURE;
> 
> nit: please use NS_ERROR_UNEXPECTED as well. The exception is thrown because
> there is supposed to be an existing listener to be removed but there is not.
> Somehow there must have been unregistered sometime before so the second call
> is unexpected.

Will do.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #26)
> Comment on attachment 8486903 [details] [diff] [review]
> Patch Part 3 v1: Move MozCellBroadcastMessage to webidl implementation.
> 
> Review of attachment 8486903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/webidl/MozCellBroadcastMessage.webidl
> @@ +17,5 @@
> > +   * and the display mode.
> > +   *
> > +   * Possible values are: "cell-immediate", "plmn", "location-area" and "cell".
> > +   */
> > +  readonly attribute DOMString? gsmGeographicalScope;
> 
> Please turn it into an enum.
> 
> @@ +46,5 @@
> > +  /**
> > +   * Possible values are "normal", "class-0", "class-1", "class-2", "class-3",
> > +   * "user-1", and "user-2".
> > +   */
> > +  readonly attribute DOMString messageClass;
> 
> enum, please.
> 
> @@ +61,5 @@
> > +
> > +  /**
> > +   * Service Category.
> > +   */
> > +  readonly attribute long cdmaServiceCategory;
> 
> long? cdmaServiceCategory;
> 
> @@ +71,5 @@
> > +  /**
> > +   * Warning type. Possible values are "earthquake", "tsunami",
> > +   * "earthquake-tsunami", "test" and "other".
> > +   */
> > +  readonly attribute ByteString? warningType;
> 
> enum.

Thanks! I'll turn these attributes into enum type in next update.
Reporter

Comment 29

5 years ago
Comment on attachment 8486902 [details] [diff] [review]
Patch Part 4 v1: Add IPDL Protocol Implementation for CellBroadcast.

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

::: dom/cellbroadcast/CellBroadcast.cpp
@@ +57,5 @@
>    MOZ_ASSERT(aWindow);
>    MOZ_ASSERT(aWindow->IsInnerWindow());
>  
>    nsCOMPtr<nsICellBroadcastService> service =
> +    do_GetService(CELLBROADCAST_SERVICE_CONTRACTID);

nit: you may keep the original naming.

::: dom/cellbroadcast/CellBroadcastServiceFactory.cpp
@@ +8,5 @@
> +#include "nsIGonkCellBroadcastService.h"
> +#endif
> +#include "nsServiceManagerUtils.h"
> +#include "nsXULAppAPI.h"
> +#include "ipc/CellBroadcastIPCService.h"

nit: move this to line 7.

::: dom/cellbroadcast/CellBroadcastServiceFactory.h
@@ +16,5 @@
> +
> +class CellBroadcastServiceFactory
> +{
> +public:
> +  static already_AddRefed<nsICellBroadcastService> GetSingleton();

nit: 'GetSingleton' sounds like getting the singleton of class CellBroadcastServiceFactory. A better name should be 'CreateCellBroadcastService'.

  static already_AddRefed<nsICellBroadcastService>
  CreateCellBroadcastService();

::: dom/cellbroadcast/ipc/CellBroadcastChild.cpp
@@ +55,5 @@
> +CellBroadcastChild::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  if (mService) {
> +    mService->NotifyActorDestroyed();
> +    mService = nullptr;

I feel that you can simply have CellBroadcastIPCService inherit nsICellBroadcastService and PCellBroadcastChild, just like have CellBroadcastParent inherit nsICellBroadcastListener and PCellBroadcastParent.

::: dom/cellbroadcast/ipc/CellBroadcastChild.h
@@ +22,5 @@
> +
> +  CellBroadcastChild(CellBroadcastIPCService* aService);
> +
> +private:
> +  virtual ~CellBroadcastChild() {};

nit: remove 'virtual' and add a comment:

  // MOZ_FINAL suppresses -Werror,-Wdelete-non-virtual-dtor

::: dom/cellbroadcast/ipc/CellBroadcastIPCService.h
@@ +28,5 @@
> +
> +  void NotifyActorDestroyed();
> +
> +private:
> +  ~CellBroadcastIPCService();

// MOZ_FINAL suppresses -Werror,-Wdelete-non-virtual-dtor

::: dom/cellbroadcast/ipc/CellBroadcastParent.h
@@ +24,5 @@
> +
> +  bool Init();
> +
> +private:
> +  virtual ~CellBroadcastParent() {};

remove 'virtual'.

::: dom/ipc/ContentChild.cpp
@@ +1338,5 @@
> +
> +bool
> +ContentChild::DeallocPCellBroadcastChild(PCellBroadcastChild* aCellBroadcast)
> +{
> +    delete aCellBroadcast;

Let CellBroadcastIPCService inherit PCellBroadcastChild and:

  static_cast<CellBroadcastIPCService*>(aActor)->Release();

::: dom/ipc/ContentParent.cpp
@@ +3194,5 @@
> +        return nullptr;
> +    }
> +
> +    CellBroadcastParent* actor = new CellBroadcastParent();
> +    NS_ADDREF(actor);

actor->AddRef();
Attachment #8486902 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #29)
> Comment on attachment 8486902 [details] [diff] [review]
> Patch Part 4 v1: Add IPDL Protocol Implementation for CellBroadcast.
> 
> Review of attachment 8486902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cellbroadcast/CellBroadcast.cpp
> @@ +57,5 @@
> >    MOZ_ASSERT(aWindow);
> >    MOZ_ASSERT(aWindow->IsInnerWindow());
> >  
> >    nsCOMPtr<nsICellBroadcastService> service =
> > +    do_GetService(CELLBROADCAST_SERVICE_CONTRACTID);
> 
> nit: you may keep the original naming.
> 
> ::: dom/cellbroadcast/CellBroadcastServiceFactory.cpp
> @@ +8,5 @@
> > +#include "nsIGonkCellBroadcastService.h"
> > +#endif
> > +#include "nsServiceManagerUtils.h"
> > +#include "nsXULAppAPI.h"
> > +#include "ipc/CellBroadcastIPCService.h"
> 
> nit: move this to line 7.
will do.
> 
> ::: dom/cellbroadcast/CellBroadcastServiceFactory.h
> @@ +16,5 @@
> > +
> > +class CellBroadcastServiceFactory
> > +{
> > +public:
> > +  static already_AddRefed<nsICellBroadcastService> GetSingleton();
> 
> nit: 'GetSingleton' sounds like getting the singleton of class
> CellBroadcastServiceFactory. A better name should be
> 'CreateCellBroadcastService'.
I met some linking problem in window build if I name it as CellBroadcastServiceFactory::CreateService().
:(

I'd try to rename it shorter to CellBroadcastFactory::CreateService() in next update.
> 
>   static already_AddRefed<nsICellBroadcastService>
>   CreateCellBroadcastService();
> 
> ::: dom/cellbroadcast/ipc/CellBroadcastChild.cpp
> @@ +55,5 @@
> > +CellBroadcastChild::ActorDestroy(ActorDestroyReason aWhy)
> > +{
> > +  if (mService) {
> > +    mService->NotifyActorDestroyed();
> > +    mService = nullptr;
> 
> I feel that you can simply have CellBroadcastIPCService inherit
> nsICellBroadcastService and PCellBroadcastChild, just like have
> CellBroadcastParent inherit nsICellBroadcastListener and
> PCellBroadcastParent.

Will simplify this in next update. :)
> 
> ::: dom/cellbroadcast/ipc/CellBroadcastChild.h
> @@ +22,5 @@
> > +
> > +  CellBroadcastChild(CellBroadcastIPCService* aService);
> > +
> > +private:
> > +  virtual ~CellBroadcastChild() {};
> 
> nit: remove 'virtual' and add a comment:
> 
>   // MOZ_FINAL suppresses -Werror,-Wdelete-non-virtual-dtor

will do

> 
> ::: dom/cellbroadcast/ipc/CellBroadcastIPCService.h
> @@ +28,5 @@
> > +
> > +  void NotifyActorDestroyed();
> > +
> > +private:
> > +  ~CellBroadcastIPCService();
> 
> // MOZ_FINAL suppresses -Werror,-Wdelete-non-virtual-dtor
> 
will do.
> ::: dom/cellbroadcast/ipc/CellBroadcastParent.h
> @@ +24,5 @@
> > +
> > +  bool Init();
> > +
> > +private:
> > +  virtual ~CellBroadcastParent() {};
> 
> remove 'virtual'.
> 
will do.
> ::: dom/ipc/ContentChild.cpp
> @@ +1338,5 @@
> > +
> > +bool
> > +ContentChild::DeallocPCellBroadcastChild(PCellBroadcastChild* aCellBroadcast)
> > +{
> > +    delete aCellBroadcast;
> 
> Let CellBroadcastIPCService inherit PCellBroadcastChild and:
> 
>   static_cast<CellBroadcastIPCService*>(aActor)->Release();
> 
I'll simplify this in next update.
> ::: dom/ipc/ContentParent.cpp
> @@ +3194,5 @@
> > +        return nullptr;
> > +    }
> > +
> > +    CellBroadcastParent* actor = new CellBroadcastParent();
> > +    NS_ADDREF(actor);
> 
> actor->AddRef();
Will do.
Attachment #8486899 - Flags: review?(bugs) → review+
rebase Patch Part1.
Attachment #8486892 - Attachment is obsolete: true
Attachment #8489292 - Flags: review+
rebase patch part2.
Attachment #8486894 - Attachment is obsolete: true
Attachment #8489293 - Flags: review+
This update is to apply the enum for gsmGeographicalScope, messageClass, warningType and mark cdmaServiceCategory as nullable.

Hi Vicamo,

May I have your review again for this change?

Thanks!
Attachment #8486903 - Attachment is obsolete: true
Attachment #8489295 - Flags: review?(vyang)
The mainly update of this patch is 
1. to simplify CellBroadcastChild into CellBroadcastIPCService.
2. Rename CellBroadcastServiceFactory::GetSingleton() to CellBroadcastFactory::CreateService().

Hi Vicamo,

May I have your review again for this change?

Thanks!
Attachment #8486902 - Attachment is obsolete: true
Attachment #8489296 - Flags: review?(vyang)
rebase patch part 5.
Attachment #8486899 - Attachment is obsolete: true
Attachment #8489297 - Flags: review+
This new patch is to move the broadcastCbsSystemMessage() From RadioInterfaceLayer to CellBroadcastService.
This is optional for this bug but is handy to include it while creating new Gonk-Backend of CellBroadcastService.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8489298 - Flags: review?(vyang)
Comment on attachment 8489296 [details] [diff] [review]
Patch Part 4 v2: Add IPDL Protocol Implementation for CellBroadcast.

Sorry, just got compiler error from try server.
I'll update this patch again after these error is fixed.
Attachment #8489296 - Flags: review?(vyang)
Reporter

Comment 38

5 years ago
Comment on attachment 8489295 [details] [diff] [review]
Patch Part 3 v2: Move MozCellBroadcastMessage to webidl implementation.

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

::: dom/cellbroadcast/CellBroadcast.cpp
@@ +112,5 @@
> +                                     const nsAString& aLanguage,
> +                                     const nsAString& aBody,
> +                                     const nsAString& aMessageClass,
> +                                     DOMTimeStamp aTimestamp,
> +                                     nsIVariant* aCdmaServiceCategory,

I'd really want to avoid complex types. We know this service category field is a 16 bits number, so can we simply use 0xFFFFFFFF instead of a nsIVariant? And please define a constant CDMA_SERVICE_CATEGORY_INVALID for this magic number.

::: dom/cellbroadcast/CellBroadcastMessage.h
@@ +51,5 @@
> +
> +  uint32_t ServiceId() const { return mServiceId; }
> +
> +  Nullable<CellBroadcastGsmGeographicalScope>
> +  GetGsmGeographicalScope() const { return mGsmGeographicalScope; }

const Nullable<CellBroadcastGsmGeographicalScope>&

@@ +62,5 @@
> +
> +  void GetBody(nsString& aBody) const { aBody = mBody; }
> +
> +  Nullable<CellBroadcastMessageClass>
> +  GetMessageClass() const { return mMessageClass; }

const Nullable<CellBroadcastMessageClass>&

@@ +69,5 @@
> +
> +  // Mark this as resultNotAddRefed to return raw pointers
> +  already_AddRefed<CellBroadcastEtwsInfo> GetEtws() const;
> +
> +  Nullable<int32_t> GetCdmaServiceCategory() const { return mCdmaServiceCategory; };

const Nullable<int32_t>&

@@ +111,5 @@
> +
> +  // WebIDL interface
> +
> +  Nullable<CellBroadcastEtwsWarningType>
> +  GetWarningType() const { return mWarningType; }

const Nullable<CellBroadcastEtwsWarningType>&

::: dom/cellbroadcast/interfaces/nsIDOMMozCellBroadcastMessage.idl
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

Please use `hg mv` to rename this idl file to webidl file instead. This way we may know what changes in this patch.
Attachment #8489295 - Flags: review?(vyang)
Reporter

Comment 39

5 years ago
Comment on attachment 8489298 [details] [diff] [review]
Patch Part 6 v2: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

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

::: dom/cellbroadcast/gonk/CellBroadcastService.js
@@ +124,5 @@
> +      messageClass: aMessageClass,
> +      timestamp: aTimestamp,
> +      cdmaServiceCategory: aCdmaServiceCategory,
> +      etws: etws
> +    };

let systemMessage = {
  ...
  etws: null
};
if (aHasEtwsInfo) {
  systemMessage.etws = {
    ...
  };
}
Attachment #8489298 - Flags: review?(vyang) → review+
To reduce the complexity of nullable |cdmaServiceCategory|, we use |long| instead of |nsIVariant| for |cdmaServiceCategory| and treat value -1 as invalid.
Attachment #8489292 - Attachment is obsolete: true
Attachment #8489936 - Flags: review+
To reduce the complexity of nullable |cdmaServiceCategory|, we use |long| instead of |nsIVariant| for |cdmaServiceCategory| and treat value -1 as invalid.
Attachment #8489293 - Attachment is obsolete: true
Attachment #8489938 - Flags: review+
To reduce the complexity of nullable |cdmaServiceCategory|, we use |long| instead of |nsIVariant| for |cdmaServiceCategory| and treat value -1 as invalid.

Hi Vicamo,

May I have your review again for this update?

Thanks!
Attachment #8489295 - Attachment is obsolete: true
Attachment #8489940 - Flags: review?(vyang)
Attachment #8489940 - Attachment description: 864484_part3_v3.patchPatch Part 4 v3: Add IPDL Protocol Implementation for CellBroadcast. → Patch Part 4 v3: Add IPDL Protocol Implementation for CellBroadcast.
Attachment #8489940 - Attachment description: Patch Part 4 v3: Add IPDL Protocol Implementation for CellBroadcast. → Patch Part 3 v3: Move MozCellBroadcastMessage to webidl implementation.
The update of this patch is 
1. to simplify CellBroadcastChild into CellBroadcastIPCService.
2. Rename CellBroadcastServiceFactory::GetSingleton() to CellBroadcastFactory::CreateCellBroadcastService().

Hi Vicamo,

May I have your review again for this change?

Thanks!
Attachment #8489296 - Attachment is obsolete: true
Attachment #8489941 - Flags: review?(vyang)
rebase.
Attachment #8489297 - Attachment is obsolete: true
Attachment #8489942 - Flags: review+
Reporter

Comment 46

5 years ago
Comment on attachment 8489940 [details] [diff] [review]
Patch Part 3 v3: Move MozCellBroadcastMessage to webidl implementation.

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

Looks good to me, but we'll need DOM peer's review. Olli, could you help us?

::: dom/cellbroadcast/CellBroadcast.cpp
@@ +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/. */
>  
>  #include "CellBroadcast.h"
> +#include "CellBroadcastMessage.h"

nit: mozilla/dom/CellBroadcast.h and mozilla/dom/CellBroadcastMessage.h

::: dom/cellbroadcast/CellBroadcastMessage.cpp
@@ +20,5 @@
> +    }                                                                   \
> +  }                                                                     \
> +}
> +
> +#define CDMA_SERVICE_CATEGORY_INVALID -1

I meant:

  interface nsICellBraodcastService
  {
    const unsigned long CDMA_SERVICE_CATEGORY_INVALID = 0xFFFFFFFF;
  }

::: dom/cellbroadcast/interfaces/nsICellBroadcastService.idl
@@ +6,3 @@
>  #include "nsISupports.idl"
>  
> +interface nsIVariant;

nit: we don't need this.

@@ +21,5 @@
> +                             in DOMString aLanguage,
> +                             in DOMString aBody,
> +                             in DOMString aMessageClass,
> +                             in DOMTimeStamp aTimestamp,
> +                             in long aCdmaServiceCategory, /* -1 if invalid. */

in unsigned long aCdmaServiceCategory,

::: dom/webidl/MozCellBroadcastMessage.webidl
@@ +68,5 @@
>  
>    /**
>     * Service Category.
>     */
> +  readonly attribute long? cdmaServiceCategory;

From the spec, I think this is actually an unsigned short integer.
Attachment #8489940 - Flags: review?(vyang)
Attachment #8489940 - Flags: review?(bugs)
Attachment #8489940 - Flags: review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #46)
> Comment on attachment 8489940 [details] [diff] [review]
> Patch Part 3 v3: Move MozCellBroadcastMessage to webidl implementation.
> 
> Review of attachment 8489940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, but we'll need DOM peer's review. Olli, could you help us?
> 
> ::: dom/cellbroadcast/CellBroadcast.cpp
> @@ +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/. */
> >  
> >  #include "CellBroadcast.h"
> > +#include "CellBroadcastMessage.h"
> 
> nit: mozilla/dom/CellBroadcast.h and mozilla/dom/CellBroadcastMessage.h
> 
> ::: dom/cellbroadcast/CellBroadcastMessage.cpp
> @@ +20,5 @@
> > +    }                                                                   \
> > +  }                                                                     \
> > +}
> > +
> > +#define CDMA_SERVICE_CATEGORY_INVALID -1
> 
> I meant:
> 
>   interface nsICellBraodcastService
>   {
>     const unsigned long CDMA_SERVICE_CATEGORY_INVALID = 0xFFFFFFFF;
>   }
> 

Sorry, I didn't explain why I used '-1' instead.
The reason is to not worry about that |CdmaServiceCategory| is defined in long or short for an invalid value.
Is it acceptable to you?

> ::: dom/cellbroadcast/interfaces/nsICellBroadcastService.idl
> @@ +6,3 @@
> >  #include "nsISupports.idl"
> >  
> > +interface nsIVariant;
> 
> nit: we don't need this.
> 
> @@ +21,5 @@
> > +                             in DOMString aLanguage,
> > +                             in DOMString aBody,
> > +                             in DOMString aMessageClass,
> > +                             in DOMTimeStamp aTimestamp,
> > +                             in long aCdmaServiceCategory, /* -1 if invalid. */
> 
> in unsigned long aCdmaServiceCategory,
> 
> ::: dom/webidl/MozCellBroadcastMessage.webidl
> @@ +68,5 @@
> >  
> >    /**
> >     * Service Category.
> >     */
> > +  readonly attribute long? cdmaServiceCategory;
> 
> From the spec, I think this is actually an unsigned short integer.
Reporter

Comment 48

5 years ago
Comment on attachment 8489941 [details] [diff] [review]
Patch Part 4 v3: Add IPDL Protocol Implementation for CellBroadcast.

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

::: dom/cellbroadcast/CellBroadcastFactory.cpp
@@ +15,5 @@
> +namespace dom {
> +namespace cellbroadcast {
> +
> +/* static */ already_AddRefed<nsICellBroadcastService>
> +CellBroadcastFactory::CreateCellBroadcastService()

I'm sorry to inform you that we have landed bug 1064231, which removes all the tiny factory classes for RIL internal services. Please have an update for this.

::: dom/cellbroadcast/ipc/CellBroadcastIPCService.cpp
@@ +76,5 @@
> +                                              const nsString& aEtwsWarningType,
> +                                              const bool& aEtwsEmergencyUserAlert,
> +                                              const bool& aEtwsPopup)
> +{
> +  for (uint32_t i = 0; i < mListeners.Length(); i++) {

Please have a copy of mListeners first because the listeners may unregister themselves in the |NotifyMessageReceived| call.

::: dom/ipc/ContentChild.h
@@ +220,5 @@
>              PBrowserChild* aBrowser) MOZ_OVERRIDE;
>      virtual bool DeallocPExternalHelperAppChild(PExternalHelperAppChild *aService) MOZ_OVERRIDE;
>  
> +    virtual PCellBroadcastChild* AllocPCellBroadcastChild() MOZ_OVERRIDE;
> +    virtual PCellBroadcastChild* SendPCellBroadcastConstructor(PCellBroadcastChild* aActor);

SendPCellBroadcastConstructor doesn't has to be a virtual function.
Attachment #8489941 - Flags: review?(vyang)
Comment on attachment 8489940 [details] [diff] [review]
Patch Part 3 v3: Move MozCellBroadcastMessage to webidl implementation.

>+++ b/dom/webidl/MozCellBroadcastMessage.webidl
>@@ -1,36 +1,36 @@
>+/* -*- Mode: c++; c-basic-offset: 2; indent-tabs-mode: nil; tab-width: 40 -*- */
>+/* vim: set ts=2 et sw=2 tw=40: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * 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/. */
> 
>-#include "domstubs.idl"
>-#include "nsISupports.idl"
>+enum CellBroadcastGsmGeographicalScope {"cell-immediate", "plmn",
>+                                        "location-area", "cell"};
>+enum CellBroadcastMessageClass {"normal", "class-0", "class-1", "class-2",
>+                                "class-3", "user-1", "user-2"};
>+enum CellBroadcastEtwsWarningType {"earthquake", "tsunami",
>+                                   "earthquake-tsunami", "test", "other"};
> 
>-interface nsIDOMMozCellBroadcastEtwsInfo;
>-
>-/**
>- * MozCellBroadcastMessage encapsulates Cell Broadcast short message service
>- * (CBS) messages.
>- */
>-[scriptable, uuid(dc729df4-f1d8-11e3-b00d-d3332542c557)]
>-interface nsIDOMMozCellBroadcastMessage : nsISupports
>+[Pref="dom.cellbroadcast.enabled"]
>+interface MozCellBroadcastMessage
> {
>   /**
>    * The Service Id in the device where the message is received from.
>    */
>   readonly attribute unsigned long serviceId;
> 
>   /**
>    * Indication of the geographical area over which the Message Code is unique,
>    * and the display mode.
>    *
>    * Possible values are: "cell-immediate", "plmn", "location-area" and "cell".
>    */
null is also a possible value, based on the type.

>-  readonly attribute DOMString gsmGeographicalScope;
>+  readonly attribute CellBroadcastGsmGeographicalScope? gsmGeographicalScope;


>   /**
>    * Possible values are "normal", "class-0", "class-1", "class-2", "class-3",
>    * "user-1", and "user-2".
>    */
same here.


>+interface MozCellBroadcastEtwsInfo
> {
>   /**
>    * Warning type. Possible values are "earthquake", "tsunami",
>    * "earthquake-tsunami", "test" and "other".
>    */

... and null
Attachment #8489940 - Flags: review?(bugs) → review+
update patch v4.
Attachment #8489936 - Attachment is obsolete: true
Attachment #8491390 - Flags: review+
update patch v4: unsigned long aCdmaServiceCategory
Attachment #8489938 - Attachment is obsolete: true
Attachment #8491391 - Flags: review+
address suggestion in comment 48.
Attachment #8489941 - Attachment is obsolete: true
Attachment #8491396 - Flags: review?(vyang)
update patch v4.
Attachment #8489942 - Attachment is obsolete: true
Attachment #8489943 - Attachment is obsolete: true
Attachment #8491399 - Flags: review+
Reporter

Comment 57

5 years ago
Comment on attachment 8491396 [details] [diff] [review]
Patch Part 4 v4: Add IPDL Protocol Implementation for CellBroadcast.

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

::: dom/cellbroadcast/ipc/CellBroadcastIPCService.cpp
@@ +14,5 @@
> +
> +CellBroadcastIPCService::CellBroadcastIPCService()
> +  : mActorDestroyed(false)
> +{
> +  ContentChild::GetSingleton()->SendPCellBroadcastConstructor(this);

Generally, you should not call virtual functions in constructor, but since CellBroadcastIPCService is marked as MOZ_FINAL, this is fine.

::: dom/cellbroadcast/ipc/CellBroadcastParent.cpp
@@ +55,5 @@
> +                                           bool aEtwsEmergencyUserAlert,
> +                                           bool aEtwsPopup)
> +{
> +  return SendNotifyReceivedMessage(aServiceId,
> +                                   nsString(aGsmGeographicalScope),

nit: use nsDependentString and we don't have to copy the string again.
Attachment #8491396 - Flags: review?(vyang) → review+
Blocks: 873351
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #57)
> > +{
> > +  return SendNotifyReceivedMessage(aServiceId,
> > +                                   nsString(aGsmGeographicalScope),
> 
> nit: use nsDependentString and we don't have to copy the string again.

No luck. :(
After trying nsDependentString locally, 
it seems no possible to identify the case when the nsString is null from caller and causes test case failure.
update revision 5.
Attachment #8491390 - Attachment is obsolete: true
Attachment #8494384 - Flags: review+
update revision#5.
Attachment #8491391 - Attachment is obsolete: true
Attachment #8494385 - Flags: review+
update revision#5.
Attachment #8491394 - Attachment is obsolete: true
Attachment #8491396 - Attachment is obsolete: true
Attachment #8494387 - Flags: review+
update revision#5 for part 5.
Attachment #8494388 - Flags: review+
update revision#5 for part 6.
Attachment #8491397 - Attachment is obsolete: true
Attachment #8491399 - Attachment is obsolete: true
Attachment #8494389 - Flags: review+
This is to restrict the string values of the attributes in CellBroadcastMessage to predefined constants.

Hi Vicamo,

May I have your review for this change?

Thanks!
Attachment #8494390 - Flags: review?(vyang)
Reporter

Comment 66

5 years ago
Comment on attachment 8494390 [details] [diff] [review]
Patch Part 7 v5: Narrow Down the DOMString values to predefined constants in .idl.

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

For the conversion thing,

  template<T> struct EnumConverter {};

  template<T>
  struct StaticEnumConverter
  {
    typedef T WebidlEnumType;
    typedef uint32_t XpidlEnumType;

    static MOZ_CONSTEXPR WebidlEnumType
    x2w(XpidlEnumType aXpidlEnum) { return static_cast<WebidlEnumType>(aXpidlEnum); }

    static MOZ_CONSTEXPR XpidlEnumType
    w2x(WebidlEnumType aWebidlEnum) { return static_cast<XpidlEnumType>(aWebidlEnum); }
  };

  template <> struct EnumConverter<CellBroadcastGsmGeographicalScope> : public StaticEnumConverter<CellBroadcastGsmGeographicalScope> {};
  template <> struct EnumConverter<CellBroadcastMessageClass> : public StaticEnumConverter<CellBroadcastMessageClass> {};

  template<T>
  MOZ_CONSTEXPR uint32_t
  ToXpidlEnum(T aWebidlEnum) { return EnumConverter::w2x(aWebidlEnum); }

  template<T>
  MOZ_CONSTEXPR T
  ToWebidlEnum(uint32_t aXpidlEnum) { return EnumConverter::x2w(aXpidlEnum); }

Now you may:

  mGsmGeographicalScope.SetValue(ToWebidlEnum<CellBroadcastGsmGeographicalScope>(aGsmGeographicalScope));

Another simplified but less flexible version:

  mGsmGeographicalScope.SetValue(static_cast<CellBroadcastGsmGeographicalScope>(aGsmGeographicalScope));
Attachment #8494390 - Flags: review?(vyang)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #66)
> Comment on attachment 8494390 [details] [diff] [review]
> Patch Part 7 v5: Narrow Down the DOMString values to predefined constants in
> .idl.
> 
> Review of attachment 8494390 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For the conversion thing,
> 
>   template<T> struct EnumConverter {};
> 
>   template<T>
>   struct StaticEnumConverter
>   {
>     typedef T WebidlEnumType;
>     typedef uint32_t XpidlEnumType;
> 
>     static MOZ_CONSTEXPR WebidlEnumType
>     x2w(XpidlEnumType aXpidlEnum) { return
> static_cast<WebidlEnumType>(aXpidlEnum); }
> 
>     static MOZ_CONSTEXPR XpidlEnumType
>     w2x(WebidlEnumType aWebidlEnum) { return
> static_cast<XpidlEnumType>(aWebidlEnum); }
>   };
> 
>   template <> struct EnumConverter<CellBroadcastGsmGeographicalScope> :
> public StaticEnumConverter<CellBroadcastGsmGeographicalScope> {};
>   template <> struct EnumConverter<CellBroadcastMessageClass> : public
> StaticEnumConverter<CellBroadcastMessageClass> {};
> 
>   template<T>
>   MOZ_CONSTEXPR uint32_t
>   ToXpidlEnum(T aWebidlEnum) { return EnumConverter::w2x(aWebidlEnum); }
> 
>   template<T>
>   MOZ_CONSTEXPR T
>   ToWebidlEnum(uint32_t aXpidlEnum) { return EnumConverter::x2w(aXpidlEnum);
> }
> 
> Now you may:
> 
>  
> mGsmGeographicalScope.
> SetValue(ToWebidlEnum<CellBroadcastGsmGeographicalScope>(aGsmGeographicalScop
> e));
> 
> Another simplified but less flexible version:
> 
>  
> mGsmGeographicalScope.
> SetValue(static_cast<CellBroadcastGsmGeographicalScope>(aGsmGeographicalScope
> ));

Thanks for better suggestion of these enum conversion.
I'll give it a trial. :)
Reporter

Comment 69

5 years ago
Comment on attachment 8495076 [details] [diff] [review]
Patch Part 7 v6: Narrow Down the DOMString values to predefined constants in .idl.

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

r+ with followings addressed. Please also have a test on Flame Gonk 123 before landing.

::: dom/cellbroadcast/CellBroadcast.cpp
@@ +110,5 @@
>  // Forwarded nsICellBroadcastListener methods
>  
>  NS_IMETHODIMP
>  CellBroadcast::NotifyMessageReceived(uint32_t aServiceId,
> +                                     uint16_t aGsmGeographicalScope,

We always use uint32_t as native type of enums.

::: dom/cellbroadcast/gonk/CellBroadcastService.js
@@ +69,5 @@
>  
> +  _convertCbGsmGeographicalScope: function(aGeographicalScope) {
> +    switch (aGeographicalScope) {
> +      case Ci.nsICellBroadcastService.GSM_GEOGRAPHICAL_SCOPE_CELL_IMMEDIATE:
> +        return RIL.CB_GSM_GEOGRAPHICAL_SCOPE_NAMES[0];

if (aGeographicalScope >= Ci.nsICellBroadcastService.GSM_GEOGRAPHICAL_SCOPE_INVALID) {
  return null;
}

return RIL.CB_GSM_GEOGRAPHICAL_SCOPE_NAMES[aGeographicalScope];

@@ +84,5 @@
> +
> +  _convertCbMessageClass: function(aMessageClass) {
> +    switch (aMessageClass) {
> +      case Ci.nsICellBroadcastService.GSM_MESSAGE_CLASS_NORMAL:
> +        return RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_NORMAL];

ditto.

@@ +105,5 @@
> +
> +  _convertCbEtwsWarningType: function(aWarningType) {
> +    switch (aWarningType) {
> +      case Ci.nsICellBroadcastService.GSM_ETWS_WARNING_EARTHQUAKE:
> +        return RIL.CB_ETWS_WARNING_TYPE_NAMES[0];

ditto.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3007,5 @@
>  
> +  _convertCbGsmGeographicalScope: function(aGeographicalScope) {
> +    switch (aGeographicalScope) {
> +      case RIL.CB_GSM_GEOGRAPHICAL_SCOPE_CELL_WIDE_IMMEDIATE:
> +        return Ci.nsICellBroadcastService.GSM_GEOGRAPHICAL_SCOPE_CELL_IMMEDIATE;

return aGeographicalScope != null ? aGeographicalScope
                                  : Ci.nsICellBroadcastService.GSM_GEOGRAPHICAL_SCOPE_INVALID;

@@ +3022,5 @@
> +
> +  _convertCbMessageClass: function(aMessageClass) {
> +    switch (aMessageClass) {
> +      case "normal":
> +        return Ci.nsICellBroadcastService.GSM_MESSAGE_CLASS_NORMAL;

let index = RIL.GECKO_SMS_MESSAGE_CLASSES.indexOf(aMessageClass);
return index != -1 ? index
                   : Ci.nsICellBroadcastService.GSM_MESSAGE_CLASS_INVALID;

@@ +3043,5 @@
> +
> +  _convertCbEtwsWarningType: function(aWarningType) {
> +    switch (aWarningType) {
> +      case 0:
> +        return Ci.nsICellBroadcastService.GSM_ETWS_WARNING_EARTHQUAKE;

return aWarningType != null ? aWarningType
                            : Ci.nsICellBroadcastService.GSM_ETWS_WARNING_INVALID;
Attachment #8495076 - Flags: review?(vyang) → review+
update final patch.
Attachment #8494384 - Attachment is obsolete: true
Attachment #8494385 - Attachment is obsolete: true
Attachment #8494386 - Attachment is obsolete: true
Attachment #8494387 - Attachment is obsolete: true
Attachment #8494388 - Attachment is obsolete: true
Attachment #8494389 - Attachment is obsolete: true
Attachment #8494390 - Attachment is obsolete: true
Attachment #8495076 - Attachment is obsolete: true
Attachment #8495803 - Flags: review+
update final patch for patch part 4.
Attachment #8495810 - Flags: review+
Update final patch for patch part 5.
Attachment #8495812 - Flags: review+
Blocks: 1074092
rebase due to merge conflict.
Attachment #8496771 - Flags: review+
rebase due to merge conflict.
Attachment #8495803 - Attachment is obsolete: true
Attachment #8495806 - Attachment is obsolete: true
Attachment #8496773 - Flags: review+
Attachment #8496773 - Attachment description: 864484_part2_final.patch → Patch Part 2: Replace CellBroadcastProvider with CellBroadcastService. r=vyang
rebase due to merge conflict.
Attachment #8495808 - Attachment is obsolete: true
Attachment #8495810 - Attachment is obsolete: true
Attachment #8496776 - Flags: review+
rebase due to merge conflict.
Attachment #8495812 - Attachment is obsolete: true
Attachment #8496777 - Flags: review+
Part 5 needs to be reviewed by a build system peer, no?
https://wiki.mozilla.org/Modules/Core#Build_Config
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #86)
> I got an IRC r+ from ted on Part 5. Also, Part 7 had webidl changes. I added
> r=smaug to satisfy the commit hook since it appears to just be rearranging
> previously-reviewed code.
Thanks for helping on this.
I'll be more careful to have review request for these kind of changes in the future.
> 
> https://hg.mozilla.org/integration/b2g-inbound/rev/37fc37deaf33
> https://hg.mozilla.org/integration/b2g-inbound/rev/fe5ef66ed15d
> https://hg.mozilla.org/integration/b2g-inbound/rev/4090b8c651ab
> https://hg.mozilla.org/integration/b2g-inbound/rev/2fe6e70071ff
> https://hg.mozilla.org/integration/b2g-inbound/rev/861971a6b28c
> https://hg.mozilla.org/integration/b2g-inbound/rev/028249514d59
> https://hg.mozilla.org/integration/b2g-inbound/rev/c91b29906f9c
You need to log in before you can comment on or make changes to this bug.