864484 - B2G RIL: use ipdl as IPC in MozCellBroadcast

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Comment 1

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

WIP branch https://github.com/vicamo/b2g_releases-mozilla-central/tree/bugzilla/864484/ipdl-cellbroadcast

Comment 2

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

Should WebIDL-ize first then we can create native message instance in CellBroadcast IPC child.

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Per offline discussion, I'd like to take this bug with bug 906398.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Part 1: Create new CellBroadcastService.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Part 2: Replace CellBroadcastProvider with CellBroadcastService.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Part 3: Replace nsIDOMMozCellBroadcastMessage/nsIMozCellBroadcastEtwsInfo with native implementation.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: WIP Part 4: Add IPDL Protocol Implementation for CellBroadcast.

IPC Connection is not verified yet.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v1: Create new CellBroadcastService.

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v1: Replace CellBroadcastProvider with CellBroadcastService.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v1: Replace nsIDOMMozCellBroadcastMessage/nsIMozCellBroadcastEtwsInfo with native implementation.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v1: Add IPDL Protocol Implementation for CellBroadcast.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v1: Build CellBroadcast by default.

Comment 13

[Bevis Tseng[:bevis][:btseng](Exited)]

Need to figure out why we got failure when running mochitest/JSRefTest in Android platform.
https://tbpl.mozilla.org/?tree=Try&rev=4bf5f93ae75d

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) 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.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v1: Create new CellBroadcastService.

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!

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v1: Replace CellBroadcastProvider with CellBroadcastService.

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!

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v1: Move MozCellBroadcastMessage to webidl implementation.

Part 3 is to Move MozCellBroadcastMessage to webidl implementation.

Hi Vicamo,

May I have your review for this change?

Thanks!

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v1: Add IPDL Protocol Implementation for CellBroadcast.

Part 4 is to add the IPC implementation of CellBroadcastService.

Hi Vicamo,

May I have your review for this change?

Thanks!

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v1: Build CellBroadcast by default.

Part 5 is to have CellBroadcast enabled without MOZ_B2G_RIL flag.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v1: Add IPDL Protocol Implementation for CellBroadcast.

Replace correct patch for Part 4.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v1: Move MozCellBroadcastMessage to webidl implementation.

Update correct patch for Part 3.

Comment 23

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=151e79c777cd

Comment 24

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

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.

Comment 25

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

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.

Comment 26

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

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.

Comment 27

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 28

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 29

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

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

Comment 30

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 31

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v2: Create new CellBroadcastService. r=vyang

rebase Patch Part1.

Comment 32

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v2: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

rebase patch part2.

Comment 33

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v2: Move MozCellBroadcastMessage to webidl implementation.

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!

Comment 34

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v2: Add IPDL Protocol Implementation for CellBroadcast.

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!

Comment 35

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v2: Build CellBroadcast by default. r=vyang,bugs

rebase patch part 5.

Comment 36

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v2: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

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!

Comment 37

[Bevis Tseng[:bevis][:btseng](Exited)]

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.

Comment 38

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

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.

Comment 39

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

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 = {
    ...
  };
}

Comment 40

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v3: Create new CellBroadcastService. r=vyang

To reduce the complexity of nullable |cdmaServiceCategory|, we use |long| instead of |nsIVariant| for |cdmaServiceCategory| and treat value -1 as invalid.

Comment 41

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v3: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

To reduce the complexity of nullable |cdmaServiceCategory|, we use |long| instead of |nsIVariant| for |cdmaServiceCategory| and treat value -1 as invalid.

Comment 42

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v3: Move MozCellBroadcastMessage to webidl implementation.

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!

Comment 43

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v3: Add IPDL Protocol Implementation for CellBroadcast.

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!

Comment 44

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v3: Build CellBroadcast by default. r=vyang,bugs

rebase.

Comment 45

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v3: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

Address nits in comment 39.

Comment 46

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

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.

Comment 47

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 48

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

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.

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 50

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v4: Create new CellBroadcastService. r=vyang

update patch v4.

Comment 51

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v4: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

update patch v4: unsigned long aCdmaServiceCategory

Comment 52

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v4: Move MozCellBroadcastMessage to webidl implementation. r=vyang,bug

address suggestion in comment 46 and comment 49.

Comment 53

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v4: Add IPDL Protocol Implementation for CellBroadcast.

address suggestion in comment 48.

Comment 54

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v4: Build CellBroadcast by default. r=vyang,bugs

update patch v4.

Comment 55

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v4: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

update patch v4.

Comment 56

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=269b5c9930e1

Comment 57

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

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.

Comment 58

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 59

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1 v5: Create new CellBroadcastService. r=vyang

update revision 5.

Comment 60

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2 v5: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

update revision#5.

Comment 61

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3 v5: Move MozCellBroadcastMessage to webidl implementation. r=vyang,bug

update revision#5.

Comment 62

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4 v5: Add IPDL Protocol Implementation for CellBroadcast. r=vyang

update revision#5.

Comment 63

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5 v5: Build CellBroadcast by default. r=vyang,bugs

update revision#5 for part 5.

Comment 64

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6 v5: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

update revision#5 for part 6.

Comment 65

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 7 v5: Narrow Down the DOMString values to predefined constants in .idl.

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!

Comment 66

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

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

Comment 67

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 68

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 7 v6: Narrow Down the DOMString values to predefined constants in .idl.

Address suggestion in comment 66.

Comment 69

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

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;

Comment 70

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1: Create new CellBroadcastService. r=vyang

update final patch.

Comment 71

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

update final patch.

Comment 72

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3: Move MozCellBroadcastMessage to webidl implementation. r=vyang,bug

update final patch for part 3.

Comment 73

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4: Add IPDL Protocol Implementation for CellBroadcast. r=vyang

update final patch for patch part 4.

Comment 74

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5: Build CellBroadcast by default. r=vyang,bugs

Update final patch for patch part 5.

Comment 75

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

update final patch part 6.

Comment 76

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 7: Narrow Down the DOMString values to predefined constants in .idl. r=vyang

update final patch for part 7.

Comment 77

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1: Create new CellBroadcastService. r=vyang

rebase due to merge conflict.

Comment 78

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2: Replace CellBroadcastProvider with CellBroadcastService. r=vyang

rebase due to merge conflict.

Comment 79

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3: Move MozCellBroadcastMessage to webidl implementation. r=vyang,bug

rebase due to merge conflict.

Comment 80

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 4: Add IPDL Protocol Implementation for CellBroadcast. r=vyang

rebase due to merge conflict.

Comment 81

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 5: Build CellBroadcast by default. r=vyang,bugs

rebase due to merge conflict.

Comment 82

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 6: Move broadcastCbsSystemMessage() from RadioInterfaceLayer to CellBroadcastService. r=vyang

rebase due to merge conflict.

Comment 83

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 7: Narrow Down the DOMString values to predefined constants in .idl. r=vyang

rebase due to merge conflict.

Comment 84

[Bevis Tseng[:bevis][:btseng](Exited)]

update try server result:
https://tbpl.mozilla.org/?tree=Try&rev=701d17e7cb0a

Comment 85

[Ryan VanderMeulen [:RyanVM]]

Part 5 needs to be reviewed by a build system peer, no?
https://wiki.mozilla.org/Modules/Core#Build_Config

Comment 86

[Ryan VanderMeulen [:RyanVM]]

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.

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

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/37fc37deaf33
https://hg.mozilla.org/mozilla-central/rev/fe5ef66ed15d
https://hg.mozilla.org/mozilla-central/rev/4090b8c651ab
https://hg.mozilla.org/mozilla-central/rev/2fe6e70071ff
https://hg.mozilla.org/mozilla-central/rev/861971a6b28c
https://hg.mozilla.org/mozilla-central/rev/028249514d59
https://hg.mozilla.org/mozilla-central/rev/c91b29906f9c

Comment 88

[Hsin-Yi Tsai (she/her) [:hsinyi]]

\o/

Comment 89

[Bevis Tseng[:bevis][:btseng](Exited)]

(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