Closed Bug 1194721 Opened 9 years ago Closed 8 years ago

[GonkSensor] Move Android Sensors driver into separate address space

Categories

(Firefox OS Graveyard :: General, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Tracking Status
firefox47 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(9 files, 28 obsolete files)

5.04 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
3.40 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
3.44 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
12.07 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
17.93 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
11.71 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
22.33 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
20.28 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
17.73 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
Bug 1187799, comment 0 lists a number of reasons why Android drivers should be moved out of Gecko and out of B2G's parent process.

One of the candidates is Android's Sensors driver. We already developed the infrastructure as part of the Bluetooth daemon. All we have to do is to adapt it for the Sensors API.
I work on this and will post patches soon.
Depends on: 1196046
Depends on: 1196221
Hi Nathan,

I saw that you're peer for MFBT. Can you review this code? This is the saturation arithmetics I mentioned on irc. The code is used patch [09].
Attachment #8649831 - Flags: review?(nfroyd)
Hi Gabriele,

The IPC code is quite complex. I suggest to start with patch [09] to get the big picture and afterwards go into the other patches that implement the details. Helpers and socket code is located in ipc/hal and shared with Bluetooth, were we use it extensively.

Let's talk if you have questions.
Attachment #8649841 - Flags: review?(gsvelto)
Comment on attachment 8649831 [details] [diff] [review]
[01] Bug 1194721: Add |Sature| template for saturation arithmetics

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

Thanks for writing this.  Lots of comments below.

Please add some tests, checking for both overflow and underflow, on all the types defined herein.

::: mfbt/Saturate.h
@@ +3,5 @@
> +/* 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/. */
> +
> +#ifndef mozilla_Saturate_h

MFBT headers typically have a one-line comment prior to the include guard giving a short description; this description shows up in DXR/MXR.  Please add one.

@@ +20,5 @@
> + *
> + *    ++SaturateUint32(value); // value is 2
> + *    --SaturateUint32(value); // value is 1
> + *    --SaturateUint32(value); // value is 0
> + *    --SaturateUint32(value); // value is still 0

This was not the approach I was expecting, but I do like how the saturation is clear at the point of usage.  Is that the only reason you went with this approach, rather than making SaturateUint32 a value type, or were there other considerations?

My sense is that you really do want Saturate<T> to be a value type, rather than a wrapper, because then the types enforce that you're always manipulating the variable according to the saturation constraints.  I think the style in this patch would make it more challenging to review code, because once you have a single Saturate() on a given value, you have to start asking the question, "are all the other operations on this value *without* Saturate() correct, or do they need Saturate, too?"  (For instance, in patch [09], I can't tell if all the operations on sSensorsTypeActivated are correct once it gets passed through GonkSensorsConnectResultHandler to GonkSensorsRegisterModuleResultHandler::RegisterModule without some serious thought.)  Which makes for a much more tedious review.

I don't know the full extent of prior art in this area, but I see that GCC's fixed point extensions (https://gcc.gnu.org/wiki/FixedPointArithmetic) put the saturation qualifier on the type, and not the operations.

@@ +25,5 @@
> + *
> + * Please add new operators and typedefs when required.
> + */
> +template <typename T, T Min, T Max>
> +class Saturate

If you expect people to only use the typedefs, it's probably better to put this in the mozilla::detail namespace.

I think a static_assert on IsIntegral<T> is warranted.

@@ +28,5 @@
> +template <typename T, T Min, T Max>
> +class Saturate
> +{
> +public:
> +  Saturate(T& aValue)

This constructor is going to need an |explicit| to pass static analysis checks.

@@ +34,5 @@
> +  { }
> +
> +  // Add and subtract operators
> +
> +  inline T operator + (const T& aRhs) const

MFBT style is:

T operator+(const T& aRhs) const

Please fix up the spacing for this and other definitions.

Also, the |inline| is superfluous; please remove it here and elsewhere.

@@ +50,5 @@
> +  // Compound operators
> +
> +  inline const T& operator += (const T& aRhs) const
> +  {
> +    if (aRhs < static_cast<T>(0)) {

This comparison is going to trigger warnings-as-errors for unsigned types.

@@ +51,5 @@
> +
> +  inline const T& operator += (const T& aRhs) const
> +  {
> +    if (aRhs < static_cast<T>(0)) {
> +      mValue = (Min - aRhs) > mValue ? Min : mValue + aRhs;

This check can trigger undefined behavior if Min > 0 for signed integers.  It would be nice to support fully general Min and Max bounds, but I'm not sure that getting all the corner cases right is worth it.  Perhaps having just:

template<typename T>
class Saturate
{
  static const T sMax = NumericLimits<T>::max();
  ...
};

would work better?

@@ +53,5 @@
> +  {
> +    if (aRhs < static_cast<T>(0)) {
> +      mValue = (Min - aRhs) > mValue ? Min : mValue + aRhs;
> +    } else {
> +      mValue = (Max - aRhs) < mValue ? Max : mValue + aRhs;

Likewise here for Max < 0.

@@ +61,5 @@
> +
> +  inline const T& operator -= (const T& aRhs) const
> +  {
> +    if (aRhs < static_cast<T>(0)) {
> +      mValue = (Max + aRhs) < mValue ? Max : mValue - aRhs;

Likewise for the comparison here, and likewise for Max < 0.

@@ +63,5 @@
> +  {
> +    if (aRhs < static_cast<T>(0)) {
> +      mValue = (Max + aRhs) < mValue ? Max : mValue - aRhs;
> +    } else {
> +      mValue = (Min + aRhs) > mValue ? Min : mValue - aRhs;

And likewise here for Min > 0.

@@ +72,5 @@
> +  // Increment and decrement operators
> +
> +  inline const T& operator ++ () const // prefix
> +  {
> +    return operator += (1);

I think no spaces around |+=| here and likewise below.  Please change them.

@@ +95,5 @@
> +    return value;
> +  }
> +
> +private:
> +  T& mValue;

I think it's worth |= delete|'ing the copy/move constructor/assignment operators here.
Attachment #8649831 - Flags: review?(nfroyd)
Comment on attachment 8649832 [details] [diff] [review]
[02] Bug 1194721: Add PDU_ prefix to daemon PDU constants

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

It looks good to me.
Attachment #8649832 - Flags: review?(shuang) → review+
No longer depends on: 1196046
Comment on attachment 8649833 [details] [diff] [review]
[03] Bug 1194721: Add |DaemonRunnable8|

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

::: ipc/hal/DaemonRunnables.h
@@ +731,5 @@
> +  template <typename InitOp>
> +  static already_AddRefed<SelfType> Create(
> +    Res (ObjectType::*aMethod)(
> +      Arg1, Arg2, Arg3, Arg4, Arg5, Arg6, Arg7, Arg8),
> +    const InitOp& aInitOp)

nit: indent for aInitOp

@@ +745,5 @@
> +  static void
> +  Dispatch(
> +    Res (ObjectType::*aMethod)(
> +      Arg1, Arg2, Arg3, Arg4, Arg5, Arg6, Arg7, Arg8),
> +    const InitOp& aInitOp)

nit: ditto
Attachment #8649833 - Flags: review?(shuang) → review+
Comment on attachment 8649835 [details] [diff] [review]
[05] Bug 1194721: Add helpers for Gonk sensors daemon

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

Looks good to me overall, only nits here.

::: hal/gonk/SensorsHelpers.cpp
@@ +10,5 @@
> +namespace mozilla {
> +namespace hal {
> +
> +//
> +// Conversion

nit: Please elaborate on this comment briefly describing what the functions below do

@@ +16,5 @@
> +
> +nsresult
> +Convert(int32_t aIn, SensorsStatus& aOut)
> +{
> +  switch (aIn) {

nit: Why using a switch here and array look ups in the other functions?

@@ +17,5 @@
> +nsresult
> +Convert(int32_t aIn, SensorsStatus& aOut)
> +{
> +  switch (aIn) {
> +  case -1:

nit: Case labels should be indented as per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +52,5 @@
> +    [0x05] = SENSORS_ERROR_DONE,
> +    [0x06] = SENSORS_ERROR_UNSUPPORTED,
> +    [0x07] = SENSORS_ERROR_PARM_INVALID
> +  };
> +  if (NS_WARN_IF(aIn >= MOZ_ARRAY_LENGTH(sError))) {

nit: I'd put a warning message like in Convert(int32_t, SensorsStatus&) here rather than using NS_WARN_IF(), I wonder if we should consider asserting on this in debug mode. Is this something we expect to happen (because of version mismatch between gecko and sensorsd) or not? If it's not the case then I'd definitely MOZ_ASSERT() here.

@@ +68,5 @@
> +    [0x01] = SENSORS_MODE_ON_CHANGE,
> +    [0x02] = SENSORS_MODE_ONE_SHOT,
> +    [0x03] = SENSORS_MODE_SPECIAL
> +  };
> +  if (NS_WARN_IF(aIn >= MOZ_ARRAY_LENGTH(sMode))) {

nit: Likewise

@@ +107,5 @@
> +    [0x18] = SENSORS_TYPE_GLANCE_GESTURE,
> +    [0x19] = SENSORS_TYPE_PICK_UP_GESTURE,
> +    [0x1a] = SENSORS_TYPE_WRIST_TILT_GESTURE
> +  };
> +  if (NS_WARN_IF(!aIn) || NS_WARN_IF(aIn >= MOZ_ARRAY_LENGTH(sType))) {

nit: Likewise

@@ +130,5 @@
> +  return NS_OK;
> +}
> +
> +//
> +// Unpacking

nit: Same as the previous comment

::: hal/gonk/SensorsHelpers.h
@@ +22,5 @@
> +
> +using namespace mozilla::ipc::DaemonSocketPDUHelpers;
> +
> +//
> +// Conversion

nit: This comment is a little short as it is, better make it into something more descriptive (e.g. "Function to convert between serialized values and their equivalent enums") or remove it altogether.

@@ +41,5 @@
> +nsresult
> +Convert(nsresult aIn, SensorsError& aOut);
> +
> +//
> +// Packing

nit: Likewise, besides I can't see any packing functions below

@@ +45,5 @@
> +// Packing
> +//
> +
> +//
> +// Unpacking

nit: Likewise

@@ +93,5 @@
> +// Init operators
> +//
> +
> +// |UnpackPDUInitOp| is a general-purpose init operator for all variants
> +// of |BluetoothResultRunnable| and |BluetoothNotificationRunnable|. The

nit: This comment looks like a leftover from the equivalent bluetooth code, possibly a copy/paste. BTW the code below looks quite generic, isn't there a way to share it with the bluetooth equivalents?

::: hal/gonk/SensorsTypes.h
@@ +120,5 @@
> +    mMaxValue = std::max(aSensor.mRange, mMaxValue);
> +  }
> +
> +  uint32_t mActivated;
> +  float mMinValue;

mMinValue seems to be always set to 0 during construction and never updated afterwards, is that really the case? I think we should make it const if it is
Attachment #8649835 - Flags: review?(gsvelto) → review+
Comment on attachment 8649836 [details] [diff] [review]
[06] Bug 1194721: Add registry interface and module for Gonk sensors

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

This mostly looks good to me but there's a few snags:

- The ownership management of certain bits looks a bit awkward to me, I've added more notes inline on the topic.

- We should really have more documentation in the .h file at the very least. I know that gecko is mostly devoid of in-code comments but I'd like to break with that tradition for the good of future developers and reviewers :) The implementation is fairly trivial and straighforward so I don't think we need too much there but the public interface should really be documented.

::: hal/gonk/SensorsRegistryInterface.cpp
@@ +14,5 @@
> +using namespace mozilla::ipc;
> +
> +//
> +// SensorsRegistryResultHandler
> +//

Since these methods are either short or empty why not define them directly in the header file?

@@ +42,5 @@
> +SensorsRegistryModule::Send(DaemonSocketPDU* aPDU,
> +                            SensorsRegistryResultHandler* aRes)
> +{
> +  if (aRes) {
> +    aRes->AddRef(); // Keep reference for response

You're manually incrementing this and I suppose this gets decremented automatically later SensorsRegistryModule::HandleSvc() and friends but I can't help to find this awkward. Ownership of the object is not very clear.

@@ +44,5 @@
> +{
> +  if (aRes) {
> +    aRes->AddRef(); // Keep reference for response
> +  }
> +  return Send(aPDU, static_cast<void*>(aRes));

Ouch. Casting to void* a reference-counted object sounds like trouble to me, especially since you're later storing this into a nsTArray. Why not passing a ref-counted object instead possibly wrapping around the raw pointer? That's especially true since you're casting back to an actual class later. You'll probably need some wrapper type to use as a base class for every different user data object but that looks less bad than un-typing it like this.

@@ +60,5 @@
> +    [OPCODE_REGISTER_MODULE] = &SensorsRegistryModule::RegisterModuleRsp,
> +    [OPCODE_UNREGISTER_MODULE] = &SensorsRegistryModule::UnregisterModuleRsp
> +  };
> +
> +  if (NS_WARN_IF(aHeader.mOpcode >= MOZ_ARRAY_LENGTH(HandleRsp)) ||

nit: I'm not fond of using NS_WARN_IF(), a descriptive warning would be much better (e.g. "Unknown or unsupported opcode").

@@ +97,5 @@
> +  rv = Send(pdu, aRes);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  unused << pdu.forget();

Why the explicit forget() call? IIRC that should be done automatically when the AutoPtr<> goes out of scope.

@@ +119,5 @@
> +  rv = Send(pdu, aRes);
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +  unused << pdu.forget();

Same question as above.

::: hal/gonk/SensorsRegistryInterface.h
@@ +30,5 @@
> +class SensorsRegistryResultHandler
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SensorsRegistryResultHandler);
> +
> +  virtual inline void OnError(SensorsError Error);

nit: aError
Attachment #8649836 - Flags: review?(gsvelto) → review-
Hi Gabriele,

Just some quick notes to answer your questions.


> @@ +42,5 @@
> > +SensorsRegistryModule::Send(DaemonSocketPDU* aPDU,
> > +                            SensorsRegistryResultHandler* aRes)
> > +{
> > +  if (aRes) {
> > +    aRes->AddRef(); // Keep reference for response
> 
> You're manually incrementing this and I suppose this gets decremented
> automatically later SensorsRegistryModule::HandleSvc() and friends but I
> can't help to find this awkward. Ownership of the object is not very clear.

We fixed this in Bluetooth's code after these patches where posted. (bug 1195729) Will be fixed in the next iteration.


> @@ +44,5 @@
> > +{
> > +  if (aRes) {
> > +    aRes->AddRef(); // Keep reference for response
> > +  }
> > +  return Send(aPDU, static_cast<void*>(aRes));
> 
> Ouch. Casting to void* a reference-counted object sounds like trouble to me,
> especially since you're later storing this into a nsTArray. Why not passing
> a ref-counted object instead possibly wrapping around the raw pointer?
> That's especially true since you're casting back to an actual class later.
> You'll probably need some wrapper type to use as a base class for every
> different user data object but that looks less bad than un-typing it like
> this.

You mean introducing a base class that only does the ref-counting? Can be done. We'd still require a cast in |HandleSrv|.


> @@ +60,5 @@
> > +    [OPCODE_REGISTER_MODULE] = &SensorsRegistryModule::RegisterModuleRsp,
> > +    [OPCODE_UNREGISTER_MODULE] = &SensorsRegistryModule::UnregisterModuleRsp
> > +  };
> > +
> > +  if (NS_WARN_IF(aHeader.mOpcode >= MOZ_ARRAY_LENGTH(HandleRsp)) ||
> 
> nit: I'm not fond of using NS_WARN_IF(), a descriptive warning would be much
> better (e.g. "Unknown or unsupported opcode").
> 
> @@ +97,5 @@
> > +  rv = Send(pdu, aRes);
> > +  if (NS_FAILED(rv)) {
> > +    return rv;
> > +  }
> > +  unused << pdu.forget();
> 
> Why the explicit forget() call? IIRC that should be done automatically when
> the AutoPtr<> goes out of scope.

When AutoPtr<> goes out of scope it won't forget its object, but delete it.

In |Send|, we hand over the PDU to the I/O thread where gets send over the socket. When |Send| returns, the ownership of the PDU has moved to the I/O thread's data structures.

The AutoPtr is convenient to delete the PDU if we return in an error branch before calling |Send|. After |Send| returns, we call |forget| to tell the AutoPtr that its pointer is no longer valid.
How about handling OOMs this way:

 * Failing during startup will already quit the daemon immediately. So we should be fine here.

 * If we fail to allocate memory while processing commands and notifications, let's first try to send out an error response or notification. If that already fails, let the daemon itself.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> We fixed this in Bluetooth's code after these patches where posted. (bug
> 1195729) Will be fixed in the next iteration.

Excellent, waiting for that, I'll be reviewing the other patches in the meantime. Sorry for taking a while but I've got my hands full.

> You mean introducing a base class that only does the ref-counting? Can be
> done. We'd still require a cast in |HandleSrv|.

Yes, we'd only down-cast in |HandleSrv| but we wouldn't need to cast to |void*| anymore.

> When AutoPtr<> goes out of scope it won't forget its object, but delete it.
> 
> In |Send|, we hand over the PDU to the I/O thread where gets send over the
> socket. When |Send| returns, the ownership of the PDU has moved to the I/O
> thread's data structures.
> 
> The AutoPtr is convenient to delete the PDU if we return in an error branch
> before calling |Send|. After |Send| returns, we call |forget| to tell the
> AutoPtr that its pointer is no longer valid.

Got it, since this is not immediately obvious please leave a short comment to make the ownership transfer clear.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> How about handling OOMs this way:
> 
>  * Failing during startup will already quit the daemon immediately. So we
> should be fine here.
> 
>  * If we fail to allocate memory while processing commands and
> notifications, let's first try to send out an error response or
> notification. If that already fails, let the daemon itself.

+1
Comment on attachment 8649837 [details] [diff] [review]
[07] Bug 1194721: Add poll interface and module for Gonk sensors

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

I'm minusing this for practically the same reasons of attachment 8649836 [details] [diff] [review]. The code's logic in and by itself is OK but I'd like to see:

- Documentation for the public interfaces in the header
- A removal of the casts to |void*|
- Having the NS_WARN_IF() instances replaced with more descriptive warning messages

I supposed that the ref-counting bits will be already partially solved because of the changes you made in bug 1195729.

::: hal/gonk/SensorsPollInterface.cpp
@@ +14,5 @@
> +
> +//
> +// SensorsPollResultHandler
> +//
> +

As with my previous review, why not making these methods inline in the header since they're empty/short?

@@ +39,5 @@
> +
> +//
> +// SensorsPollNotificationHandler
> +//
> +

Likewise

::: hal/gonk/SensorsPollInterface.h
@@ +25,5 @@
> +class SensorsInterface;
> +
> +using mozilla::ipc::DaemonSocketPDU;
> +using mozilla::ipc::DaemonSocketPDUHeader;
> +

Same as with the other files let's describe these interfaces in the header with comments for the whole classes plus each method.

@@ +30,5 @@
> +class SensorsPollResultHandler
> +{
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(SensorsPollResultHandler);
> +
> +  virtual inline void OnError(SensorsError Error);

nit: aError

@@ +99,5 @@
> +  //
> +  // Responses
> +  //
> +
> +  typedef mozilla::ipc::DaemonResultRunnable0<

Let's use mozilla::ipc globally, it will make this code easier on the eyes and I don't think it's going to introduce name clashes.
Attachment #8649837 - Flags: review?(gsvelto) → review-
Comment on attachment 8649838 [details] [diff] [review]
[08] Bug 1194721: Add interface for Gonk sensors daemon

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

This LGTM provided it will pick up the changes to the two previous patches, my only two general nits are more documentation and the use of descriptive warnings instead of NS_WARN_IF().

::: hal/gonk/SensorsInterface.cpp
@@ +19,5 @@
> +
> +//
> +// SensorsResultHandler
> +//
> +

nit: Inline the empty/short methods if possible.

@@ +190,5 @@
> +/* returns the container structure of a variable; _t is the container's
> + * type, _v the name of the variable, and _m is _v's field within _t
> + */
> +#define container(_t, _v, _m) \
> +  ( (_t*)( ((const unsigned char*)(_v)) - offsetof(_t, _m) ) )

This looks unused, let's remove it

::: hal/gonk/SensorsInterface.h
@@ +25,5 @@
> +
> +class SensorsPollInterface;
> +class SensorsProtocol;
> +class SensorsRegistryInterface;
> +

Please document the public interfaces.
Attachment #8649838 - Flags: review?(gsvelto) → review+
Comment on attachment 8649841 [details] [diff] [review]
[09] Bug 1194721: Support Gonk sensors daemon

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

LGTM save for a few minor nits.

::: hal/gonk/GonkSensor.cpp
@@ +217,5 @@
>        break;
>      }
>  
>      for (int i = 0; i < n; ++i) {
> +

nit: Why adding an empty line here?

@@ +403,5 @@
> +private:
> +  SensorType HardwareSensorToHalSensor(SensorsType aType)
> +  {
> +    switch (aType) {
> +    case SENSORS_TYPE_ORIENTATION:

nit: case labels must be indented

@@ +532,5 @@
> +
> +  void EventNotification(int32_t aId, const SensorsEvent& aEvent) override
> +  {
> +    ssize_t i = FindSensorIndexById(aId);
> +    if (NS_WARN_IF(i == -1)) {

nit: Add a descriptive warning

@@ +607,5 @@
> +
> +    // Init, step 3: set notification handler for poll service and vice versa
> +    SensorsPollInterface* pollInterface =
> +      mInterface->GetSensorsPollInterface();
> +    if (NS_WARN_IF(!pollInterface)) {

nit: Likewise

@@ +650,5 @@
> +
> +    // Init, step 2: register poll service
> +    SensorsRegistryInterface* registryInterface =
> +      mInterface->GetSensorsRegistryInterface();
> +    if (NS_WARN_IF(!registryInterface)) {

nit: Likewise

@@ +694,5 @@
> +void
> +EnableSensorNotificationsDaemon(SensorType aSensor)
> +{
> +  if (NS_WARN_IF(aSensor < 0) ||
> +      NS_WARN_IF(aSensor > static_cast<ssize_t>(MOZ_ARRAY_LENGTH(sSensorsType)))) {

nit: Likewise

@@ +699,5 @@
> +    return; // Unsupported sensor type
> +  }
> +
> +  SensorsInterface* interface = SensorsInterface::GetInstance();
> +  if (NS_WARN_IF(!interface)) {

nit: From what I can tell SensorsInterface::GetInstance() never fails, shouldn't we be asserting here instead?

@@ +734,5 @@
> +void
> +DisableSensorNotificationsDaemon(SensorType aSensor)
> +{
> +  if (NS_WARN_IF(aSensor < 0) ||
> +      NS_WARN_IF(aSensor > static_cast<ssize_t>(MOZ_ARRAY_LENGTH(sSensorsType)))) {

nite: Add a descriptive warning

@@ +747,5 @@
> +
> +  // We might be in the middle of a startup; decrement type's ref-counter.
> +  --SaturateUint32(sSensorsTypeActivated[sSensorsType[aSensor]]);
> +
> +  // TODO: stop sensorsd if all sensors are disabled

nit: File a bug for this and reference it in the code

@@ +754,5 @@
> +//
> +// Public interface
> +//
> +
> +// TODO: Remove in-Gecko sensors code. Until all devices' base

nit: Likewise, it's very important that we track this
Attachment #8649841 - Flags: review?(gsvelto) → review+
Changes since v1:

  - incorporate protocol changes from bug 1196221
Attachment #8649836 - Attachment is obsolete: true
Attachment #8657129 - Flags: review-
Changes since v1:

  - incorporate protocol changes from bug 1196221
Attachment #8657130 - Flags: review-
Changes since v1:

  - incorporate protocol changes from bug 1196221
Attachment #8649841 - Attachment is obsolete: true
Attachment #8657131 - Flags: review+
Attachment #8649837 - Attachment is obsolete: true
Hi

(In reply to Gabriele Svelto [:gsvelto] from comment #14)

> 
> @@ +52,5 @@
> > +    [0x05] = SENSORS_ERROR_DONE,
> > +    [0x06] = SENSORS_ERROR_UNSUPPORTED,
> > +    [0x07] = SENSORS_ERROR_PARM_INVALID
> > +  };
> > +  if (NS_WARN_IF(aIn >= MOZ_ARRAY_LENGTH(sError))) {
> 
> nit: I'd put a warning message like in Convert(int32_t, SensorsStatus&) here
> rather than using NS_WARN_IF(), I wonder if we should consider asserting on
> this in debug mode. Is this something we expect to happen (because of
> version mismatch between gecko and sensorsd) or not? If it's not the case
> then I'd definitely MOZ_ASSERT() here.

I currently work on logging macros for the IPC code. I'll also convert all code to these macros.

We don't expect invalid input from the daemons, but it could happen of course. I'd rather not default to asserting for debug builds. If we see an incorrect PDU, Gecko has to deal with it. If we assert in this case we'd take away the option of testing this code automatically. I don't mind adding an assert-on-errors build switch to the logging macros though.
Depends on: 1202386
Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #13)
> ::: ipc/hal/DaemonRunnables.h
> @@ +731,5 @@
> > +  template <typename InitOp>
> > +  static already_AddRefed<SelfType> Create(
> > +    Res (ObjectType::*aMethod)(
> > +      Arg1, Arg2, Arg3, Arg4, Arg5, Arg6, Arg7, Arg8),
> > +    const InitOp& aInitOp)
> 
> nit: indent for aInitOp

The indention should be correct here. After|aMethod|, |aInitOp| is another argument to |Create|, so it has the same indention as |aMethod|. The |Arg*|s are parameters to |aMethod| and therefore indented one additional level.
> ::: hal/gonk/SensorsHelpers.h
> @@ +22,5 @@
> > +
> > +using namespace mozilla::ipc::DaemonSocketPDUHelpers;
> > +
> > +//
> > +// Conversion
> 
> nit: This comment is a little short as it is, better make it into something
> more descriptive (e.g. "Function to convert between serialized values and
> their equivalent enums") or remove it altogether.

All these functions extend the generic PDU framework in hal/ipc. I'll add short comments for each block and point to further documentation in the generic code.

> 
> @@ +93,5 @@
> > +// Init operators
> > +//
> > +
> > +// |UnpackPDUInitOp| is a general-purpose init operator for all variants
> > +// of |BluetoothResultRunnable| and |BluetoothNotificationRunnable|. The
> 
> nit: This comment looks like a leftover from the equivalent bluetooth code,
> possibly a copy/paste. BTW the code below looks quite generic, isn't there a
> way to share it with the bluetooth equivalents?

Yes, it's the same as for Bluetooth, but I have not yet found a way to share the code. Namespaces, ordering of declarations, and include statements always prevented C++ to resolve all symbols. But I'll do another try before uploading the next iteration of this patch.


> ::: hal/gonk/SensorsTypes.h
> @@ +120,5 @@
> > +    mMaxValue = std::max(aSensor.mRange, mMaxValue);
> > +  }
> > +
> > +  uint32_t mActivated;
> > +  float mMinValue;
> 
> mMinValue seems to be always set to 0 during construction and never updated
> afterwards, is that really the case? I think we should make it const if it is

That's the case, but not intentionally. I'll set it to const and leave a comment.

|mMinValue| should be updated from the values of a SensorDetectedNotification (via |SensorsSensor|). But all sensors seem to have a minimum range of 0 and Android's API doesn't support minimum ranges.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28)
> > 
> > mMinValue seems to be always set to 0 during construction and never updated
> > afterwards, is that really the case? I think we should make it const if it is
> 
> That's the case, but not intentionally. I'll set it to const and leave a
> comment.
> 
> |mMinValue| should be updated from the values of a
> SensorDetectedNotification (via |SensorsSensor|). But all sensors seem to
> have a minimum range of 0 and Android's API doesn't support minimum ranges.

Just tried, but setting this field to const collides with patch [09] where we assign a clear instance of |SensorsClass| in |ClearSensorClasses|. I guess it's easiest to keep the field as it is.
(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> ::: hal/gonk/SensorsPollInterface.cpp
> @@ +14,5 @@
> > +
> > +//
> > +// SensorsPollResultHandler
> > +//
> > +
> 
> As with my previous review, why not making these methods inline in the
> header since they're empty/short?
> 
> @@ +39,5 @@
> > +
> > +//
> > +// SensorsPollNotificationHandler
> > +//
> > +
> 
> Likewise

These are virtual methods: the compiler cannot inline them.

The actually important implementations are in patch [09] in GonkSensors.cpp, where result and notification handlers overload these methods.

The methods here are just placeholders. I don't expect them to get called. Without them we'd have missing symbols in the linker. Inlining them would mean that the compiler would create them in every object file that instantiates the class and the linker would sort out the duplicates later on. That would only add work for both compiler and linker.

This single case is probably irrelevant, but simple headers generally lead to short compile times. Since inlining doesn't improve runtime performance in this case, I'd like to keep the methods in the source file.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #26)
> We don't expect invalid input from the daemons, but it could happen of
> course. I'd rather not default to asserting for debug builds. If we see an
> incorrect PDU, Gecko has to deal with it. If we assert in this case we'd
> take away the option of testing this code automatically.

Nah, it's fine as it is.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28)
> All these functions extend the generic PDU framework in hal/ipc. I'll add
> short comments for each block and point to further documentation in the
> generic code.

A single comment at the top should suffice.

> Yes, it's the same as for Bluetooth, but I have not yet found a way to share
> the code. Namespaces, ordering of declarations, and include statements
> always prevented C++ to resolve all symbols. But I'll do another try before
> uploading the next iteration of this patch.

Don't get bogged down on this, worst case we can land this code as-is and file a bug for a future cleanup.

> That's the case, but not intentionally. I'll set it to const and leave a
> comment.
> 
> |mMinValue| should be updated from the values of a
> SensorDetectedNotification (via |SensorsSensor|). But all sensors seem to
> have a minimum range of 0 and Android's API doesn't support minimum ranges.

OK, thanks for the explanation.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> This single case is probably irrelevant, but simple headers generally lead
> to short compile times. Since inlining doesn't improve runtime performance
> in this case, I'd like to keep the methods in the source file.

Yes, my concern was purely about source code length, not performance. I've been doing a few years of compiler work before landing at Mozilla ;) Generally speaking I'd prefer shorter code but it's true that our link-phase is already horribly long as it is and I doubt we'll be changing this code much (save for possibly adding more placeholders) so if you prefer it this way I'm not against it.
(In reply to Gabriele Svelto [:gsvelto] from comment #20)
> ::: hal/gonk/SensorsPollInterface.h
> > +  //
> > +  // Responses
> > +  //
> > +
> > +  typedef mozilla::ipc::DaemonResultRunnable0<
> 
> Let's use mozilla::ipc globally, it will make this code easier on the eyes
> and I don't think it's going to introduce name clashes.

I don't mind this, but the coding style says that 'using namespace' in headers is not allowed. I already had to violate this rule in SensorsHelpers.h. Maybe I can import the runnable classes.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #32)
> I don't mind this, but the coding style says that 'using namespace' in
> headers is not allowed.

I wasn't aware of this, OK then, nevermind.
Depends on: 1202704
Depends on: 1203092
(In reply to Gabriele Svelto [:gsvelto] from comment #22)
> @@ +699,5 @@
> > +    return; // Unsupported sensor type
> > +  }
> > +
> > +  SensorsInterface* interface = SensorsInterface::GetInstance();
> > +  if (NS_WARN_IF(!interface)) {
> 
> nit: From what I can tell SensorsInterface::GetInstance() never fails,
> shouldn't we be asserting here instead?

In Bluetooth, we have a situation were Gecko might not contain any backend code at all. In this case, |nullptr| is a valid result. For reasons of symmetry, I'd like to use these semantics here as well.

In the very long term, we can do something similar with sensors: the public interfaces of |SensorsInterface| and friends are not Gonk-specific. We could implement them on other systems with different driver APIs. On unsupported systems, nullptr would be returned.
FYI: I currently work on this patch set. I expect to post some updates during the next days.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #34)
> In Bluetooth, we have a situation were Gecko might not contain any backend
> code at all. In this case, |nullptr| is a valid result. For reasons of
> symmetry, I'd like to use these semantics here as well.
> 
> In the very long term, we can do something similar with sensors: the public
> interfaces of |SensorsInterface| and friends are not Gonk-specific. We could
> implement them on other systems with different driver APIs. On unsupported
> systems, nullptr would be returned.

Makes sense, thanks.
Changes since v1:

  - |Saturate| now stores its value
  - |SaturateOp| implements arithmetics operators
  - fixed overflows
  - Added tests
  - coding style cleanups

Nathan,

|Saturate| now stores the value, while |SaturateOp| performs the actual arithmetics. Users can pick which fits their needs best. All type-specifics can probably be handled in |SaturateOp|.

I also changed the code to work on the type's full range, as returned by |NumericLimits|. Special cases, such as floating-point ranges of [0,1] (which is common in graphics), can be implemented by using specialized types. I think it should also be possible to use math vectors with |SaturateOp|.
Attachment #8649831 - Attachment is obsolete: true
Attachment #8659859 - Flags: review?(nfroyd)
Changes since v1

  - shares more code with Bluetooth
  - improved error reporting
  - coding-style changes
  - improved documentation
Attachment #8649835 - Attachment is obsolete: true
Attachment #8659862 - Flags: review?(gsvelto)
Changes since v2:

  - result handlers inherit from |DaemonSocketResultHandler|
  - improved ownership management
  - improved error reporting
  - documented public interfaces
  - coding-style fixes
Attachment #8657129 - Attachment is obsolete: true
Attachment #8659864 - Flags: review?(gsvelto)
Changes since v2:

  -result handlers inherit from |DaemonSocketResultHandler|
  - improved ownership management
  - improved error reporting
  - supports protocol versions
  - documented public interfaces
  - coding-style fixes
Attachment #8657130 - Attachment is obsolete: true
Attachment #8659868 - Flags: review?(gsvelto)
Changes since v1:

  - improved ownership management
  - improved error reporting
  - documented public interfaces
  - coding-style fixes
Attachment #8649838 - Attachment is obsolete: true
Attachment #8659869 - Flags: review?(gsvelto)
Changes since v2:

  - better error handling during start
  - improved error reporting
  - coding-style fixes
Attachment #8657131 - Attachment is obsolete: true
Attachment #8659870 - Flags: review?(gsvelto)
Changes since v2:

  - removed out-commented code
Attachment #8659859 - Attachment is obsolete: true
Attachment #8659859 - Flags: review?(nfroyd)
Attachment #8659872 - Flags: review?(nfroyd)
Changes since v3:

  - fixed patch headline
Attachment #8659872 - Attachment is obsolete: true
Attachment #8659872 - Flags: review?(nfroyd)
Attachment #8659892 - Flags: review?(nfroyd)
Comment on attachment 8659892 [details] [diff] [review]
[01] Bug 1194721: Add |Saturate| template for saturation arithmetics (v4)

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

Thanks for the revisions.  A couple more things to address.

::: mfbt/Saturate.h
@@ +41,5 @@
> +    : mValue(aValue)
> +  {
> +    // We should actually check for |std::is_scalar<T>::value| to be
> +    // true, but this type trait is not available everywhere. Relax
> +    // this assertion if you what to use floating point values as well.

Nit: "...if you want to use..."

@@ +43,5 @@
> +    // We should actually check for |std::is_scalar<T>::value| to be
> +    // true, but this type trait is not available everywhere. Relax
> +    // this assertion if you what to use floating point values as well.
> +    static_assert(IsIntegral<T>::value,
> +                  "Integral type required in instanciation");

Nit: "instantiation"

@@ +129,5 @@
> +class Saturate
> +{
> +public:
> +  Saturate() = default;
> +  Saturate(const Saturate<T>&) = default;

This will need to be marked |explicit| or |MOZ_IMPLICIT|, depending on what you want the behavior to be.

@@ +250,5 @@
> +typedef detail::SaturateOp<int16_t> SaturateOpInt16;
> +typedef detail::SaturateOp<int32_t> SaturateOpInt32;
> +typedef detail::SaturateOp<uint8_t> SaturateOpUint8;
> +typedef detail::SaturateOp<uint16_t> SaturateOpUint16;
> +typedef detail::SaturateOp<uint32_t> SaturateOpUint32;

I don't think these will be useful and as suggested earlier, I think they make code more difficult to follow.  Please remove the typedefs.

I think this means Saturate<> needs a value() method or similar.  Please add one.

::: mfbt/tests/TestSaturate.cpp
@@ +20,5 @@
> +static T
> +StartValue()
> +{
> +  // start near middle of the type's range; won't hit bounds
> +  return (NumericLimits<T>::max() - NumericLimits<T>::min()) / 2;

This is going to trigger undefined behavior for signed integer types...and even if it doesn't, it means your start value is going to be near your maximum value, which is going to cause test failures.

@@ +34,5 @@
> +  T value = StartValue<T>();
> +  Saturate<T> satValue(value);
> +
> +  for (T i = 0; i < static_cast<T>(sNumOps); ++i) {
> +    A(++value == ++satValue);

...here, for example.

@@ +114,5 @@
> +  A(--satValue == (NumericLimits<T>::max() - 1));
> +  A(++satValue == (NumericLimits<T>::max()));
> +  A(++satValue == (NumericLimits<T>::max())); // don't overflow here
> +  A(++satValue == (NumericLimits<T>::max())); // don't overflow here
> +  A(--satValue == (NumericLimits<T>::max())); // back at max value

How does this work?  --satValue returns the new value, which should be one less than the max value.

@@ +128,5 @@
> +  A(++satValue == (NumericLimits<T>::min() + 1));
> +  A(--satValue == (NumericLimits<T>::min()));
> +  A(--satValue == (NumericLimits<T>::min())); // don't overflow here
> +  A(--satValue == (NumericLimits<T>::min())); // don't overflow here
> +  A(++satValue == (NumericLimits<T>::min())); // back at max value

Likewise.

::: mfbt/tests/moz.build
@@ +27,4 @@
>      'TestPair',
>      'TestRefPtr',
>      'TestRollingMean',
> +    'TestSaturate',

In addition to adding it here, I think you also need to add it to testing/cppunittest.ini.
Attachment #8659892 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8659862 [details] [diff] [review]
[05] Bug 1194721: Add helpers for Gonk sensors daemon (v2)

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

Looking good!
Attachment #8659862 - Flags: review?(gsvelto) → review+
Comment on attachment 8659864 [details] [diff] [review]
[06] Bug 1194721: Add registry interface and module for Gonk sensors (v3)

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

LGTM save for a small nit.

::: hal/gonk/SensorsRegistryInterface.h
@@ +47,5 @@
> +
> +  /**
> +   * Called if a registry command failed.
> +   *
> +   * \param aError The error code.

nit: Looking through our codebase I see we use javadoc-style comments (i.e. @param) as opposed to doxygen ones, let's change this to be consistent with the rest of the code though we don't seem to have a rule about this in the style guidelines

@@ +54,5 @@
> +
> +  /**
> +   * The callback method for |SensorsRegistryInterface::RegisterModule|.
> +   *
> +   * \param aProtocolVersion The daemon's protocol version. Make sure it's

nit: Likewise

@@ +152,5 @@
> +   * result handler's |RegisterModule| method gets called, the service
> +   * has been registered successfully and can be used.
> +   *
> +   * \param aId The id of the service that is to be registered.
> +   * \param aRes The result handler.

nit: Likewise

@@ +161,5 @@
> +   * Sends an UnregisterModule command to the Sensors daemon. The service
> +   * should not be used afterwards until it has been registered again.
> +   *
> +   * \param aId The id of the service that is to be unregistered.
> +   * \param aRes The result handler.

nit: Likewise
Comment on attachment 8659864 [details] [diff] [review]
[06] Bug 1194721: Add registry interface and module for Gonk sensors (v3)

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

LGTM save for a small nit.

::: hal/gonk/SensorsRegistryInterface.h
@@ +47,5 @@
> +
> +  /**
> +   * Called if a registry command failed.
> +   *
> +   * \param aError The error code.

nit: Looking through our codebase I see we use javadoc-style comments (i.e. @param) as opposed to doxygen ones, let's change this to be consistent with the rest of the code though we don't seem to have a rule about this in the style guidelines

@@ +54,5 @@
> +
> +  /**
> +   * The callback method for |SensorsRegistryInterface::RegisterModule|.
> +   *
> +   * \param aProtocolVersion The daemon's protocol version. Make sure it's

nit: Likewise

@@ +152,5 @@
> +   * result handler's |RegisterModule| method gets called, the service
> +   * has been registered successfully and can be used.
> +   *
> +   * \param aId The id of the service that is to be registered.
> +   * \param aRes The result handler.

nit: Likewise

@@ +161,5 @@
> +   * Sends an UnregisterModule command to the Sensors daemon. The service
> +   * should not be used afterwards until it has been registered again.
> +   *
> +   * \param aId The id of the service that is to be unregistered.
> +   * \param aRes The result handler.

nit: Likewise
Attachment #8659864 - Flags: review?(gsvelto) → review+
Comment on attachment 8659868 [details] [diff] [review]
[07] Bug 1194721: Add poll interface and module for Gonk sensors (v3)

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

Also LGTM with a couple of nits.

::: hal/gonk/SensorsPollInterface.cpp
@@ +123,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_ENABLE_SENSOR,
> +                        4)); // id

nit: Wasn't this covered by some defines too? If it wasn't it would be nice to instead of having hard-coded values.

@@ +144,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_DISABLE_SENSOR,
> +                        4)); // id

nit: Likewise

@@ +167,5 @@
> +
> +  nsAutoPtr<DaemonSocketPDU> pdu(
> +    new DaemonSocketPDU(SERVICE_ID, OPCODE_SET_PERIOD,
> +                        4 + // id
> +                        8)); // period

nit: Likewise

::: hal/gonk/SensorsPollInterface.h
@@ +48,5 @@
> +
> +  /**
> +   * Called if a poll command failed.
> +   *
> +   * \param aError The error code.

nit: As in the previous attachment I think we should use @param for consistency, likewise for all other instances of this in the patch

::: hal/gonk/SensorsRegistryInterface.h
@@ +12,5 @@
>   * All public methods and callback methods run on the main thread.
>   */
>  
> +#ifndef hal_gonk_SensorsRegistryInterface_h
> +#define hal_gonk_SensorsRegistryInterface_h

I think this change was unintended.
Attachment #8659868 - Flags: review?(gsvelto) → review+
Comment on attachment 8659869 [details] [diff] [review]
[08] Bug 1194721: Add interface for Gonk sensors daemon (v2)

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

LGTM, as with the other patches I think we should use @param instead of \param in the documentation.
Attachment #8659869 - Flags: review?(gsvelto) → review+
Comment on attachment 8659870 [details] [diff] [review]
[09] Bug 1194721: Support Gonk sensors daemon (v3)

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

LGTM save for a minor nit.

::: hal/gonk/GonkSensor.cpp
@@ +498,5 @@
> +  //
> +
> +  void ErrorNotification(SensorsError aError) override
> +  {
> +    // TODO: Try to repair some of the errors or restart cleanly.

nit: I've seen XXX used more often than TODO in our code, I'm not sure if it's a rule or not. What's certainly needed though is a reference to the bug number.
Attachment #8659870 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #50)
> Comment on attachment 8659868 [details] [diff] [review]
> [07] Bug 1194721: Add poll interface and module for Gonk sensors (v3)
> ::: hal/gonk/SensorsPollInterface.cpp
> @@ +123,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  nsAutoPtr<DaemonSocketPDU> pdu(
> > +    new DaemonSocketPDU(SERVICE_ID, OPCODE_ENABLE_SENSOR,
> > +                        4)); // id
> 
> nit: Wasn't this covered by some defines too? If it wasn't it would be nice
> to instead of having hard-coded values.

It's just a hint, so let's simply set this to zero. Gecko will resize the buffers as needed.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #53)
> It's just a hint, so let's simply set this to zero. Gecko will resize the
> buffers as needed.

OK, I didn't realize that, then let's leave as it is.
Thanks for the review!

(In reply to Nathan Froyd [:froydnj] from comment #46)
> 
> @@ +250,5 @@
> > +typedef detail::SaturateOp<int16_t> SaturateOpInt16;
> > +typedef detail::SaturateOp<int32_t> SaturateOpInt32;
> > +typedef detail::SaturateOp<uint8_t> SaturateOpUint8;
> > +typedef detail::SaturateOp<uint16_t> SaturateOpUint16;
> > +typedef detail::SaturateOp<uint32_t> SaturateOpUint32;
> 
> I don't think these will be useful and as suggested earlier, I think they
> make code more difficult to follow.  Please remove the typedefs.

I used them in patch [09]. The related data structures are simpler than with |Saturate<>|
Changes since v4:

  - fixed typos
  - fixed tests according to review
  - removed typedefs for |SaturateOp|
  - annotate copy c'tor for |Saturate| correctly
Attachment #8659892 - Attachment is obsolete: true
Changes since v2:

  - JavaDoc comments
  - renamed |SensorMode| to |SensorTriggerMode|
  - added |SensorDeliveryMode| type
  - support for delivery-mode fields in IPC protocol
Attachment #8659862 - Attachment is obsolete: true
Attachment #8664808 - Flags: review+
Changes since v3:

  - JavaDoc comments
Attachment #8659864 - Attachment is obsolete: true
Attachment #8664809 - Flags: review+
Changes since v3:

  - JavaDoc comments
  - detect length of command PDUs
  - support for delivery-mode fields in IPC protocol
Attachment #8659868 - Attachment is obsolete: true
Attachment #8664811 - Flags: review+
Changes since v2:

  - JavaDoc comments
Attachment #8659869 - Attachment is obsolete: true
Attachment #8664813 - Flags: review+
Changes since v3:

  - JavaDoc comments
  - opened a bug for handling daemon errors in Gecko
  - support for delivery-mode fields in IPC protocol

We don't do much with the new delivery-mode field in the IPC protocol, but it's parsed and validated. Future versions shall hold a wake lock for immediate delivery of events.

On L and later, there can be immediate and best-effort sensors of the same type. Gecko currently follows the semantics of older versions: proximity and significant motion uses immediate delivery, all other sensors use best effort. It's documented at [1].

[1] https://source.android.com/devices/sensors/suspend-mode.html
Attachment #8659870 - Attachment is obsolete: true
Attachment #8664818 - Flags: review+
Changes since v4:

  - don't shadow class members with local variables (see bug 1207030)
Attachment #8664811 - Attachment is obsolete: true
Attachment #8665851 - Flags: review+
Changes since v5:

  - don't shadow class members with local variables
  - fixed type-range assertions in |TestAll|
Attachment #8664806 - Attachment is obsolete: true
Changes since v6:

  - implement move constructor/assignment because 'default' is not supported on Windows
Attachment #8665852 - Attachment is obsolete: true
Comment on attachment 8666624 [details] [diff] [review]
[01] Bug 1194721: Add |Saturate| template for saturation arithmetics (v7)

Try finally looks good; the reds and oranges are unrelated AFAICT. I fixed the points of the review. Change logs are in the comments above.
Attachment #8666624 - Flags: review?(nfroyd)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #69)
> Comment on attachment 8666624 [details] [diff] [review]
> [01] Bug 1194721: Add |Saturate| template for saturation arithmetics (v7)
> 
> Try finally looks good; the reds and oranges are unrelated AFAICT. I fixed
> the points of the review. Change logs are in the comments above.

Just FYI, I don't think I'm going to get to this review until Wednesday.
Comment on attachment 8666624 [details] [diff] [review]
[01] Bug 1194721: Add |Saturate| template for saturation arithmetics (v7)

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

Thanks for the changes.  r=me with the minor problems below fixed.

::: mfbt/Saturate.h
@@ +18,5 @@
> +
> +/**
> + * |SaturateOp<T>| wraps scalar values for saturation arithmetics. A number of
> + * typedefs specialize it for common C++ types, for example |SaturateOpInt16|
> + * or |SaturateOpUint32|. Usage:

These typedefs have gone away, please remove the sentence describing them.

@@ +115,5 @@
> +  }
> +
> +private:
> +  SaturateOp(const SaturateOp<T>&) = delete;
> +  SaturateOp(const SaturateOp<T>&&) = delete;

A move constructor taking a const argument is a little weird; please drop the const here.

@@ +116,5 @@
> +
> +private:
> +  SaturateOp(const SaturateOp<T>&) = delete;
> +  SaturateOp(const SaturateOp<T>&&) = delete;
> +  const SaturateOp& operator=(SaturateOp<T>&) = delete;

I think you also want to delete the move assignment operator here.

Please drop the const qualification on the return value, and const-qualify the argument.

@@ +133,5 @@
> +  Saturate() = default;
> +  MOZ_IMPLICIT Saturate(const Saturate<T>&) = default;
> +
> +  MOZ_IMPLICIT Saturate(Saturate<T>&& aValue)
> +    : mValue(aValue.mValue)

I think this is just |= default|, right?

@@ +140,5 @@
> +  explicit Saturate(const T& aValue)
> +    : mValue(aValue)
> +  { }
> +
> +  const T& value() const

I'm not sure const T& here actually buys you anything...maybe just return T instead?

@@ +171,5 @@
> +  // Assignment operators
> +
> +  Saturate<T>& operator=(const Saturate<T>&) = default;
> +
> +  Saturate<T>& operator=(Saturate<T>&& aRhs)

This should just be |= default|, too.

::: mfbt/tests/TestSaturate.cpp
@@ +65,5 @@
> +uint32_t
> +StartValue<uint32_t>()
> +{
> +  // Picking a value near middle of uint32_t's range.
> +  return static_cast<uint8_t>(NumericLimits<int32_t>::max());

I think you could probably avoid all these specializations by:

template<typename T>
T
StartValue()
{
  // Pick a value near the middle of T's range.
  return (NumericLimits<T>::max() + NumericLimits<T>::min()) / 2;
}
Attachment #8666624 - Flags: review?(nfroyd) → review+
Hi

thanks for the review. The final issues will be fixed in the next version.

> > +private:
> > +  SaturateOp(const SaturateOp<T>&) = delete;
> > +  SaturateOp(const SaturateOp<T>&&) = delete;
> 
> A move constructor taking a const argument is a little weird; please drop
> the const here.

Right, a move from a const rval doesn't make sense. I'm surprised that the compilers let me build this.

> @@ +133,5 @@
> > +  Saturate() = default;
> > +  MOZ_IMPLICIT Saturate(const Saturate<T>&) = default;
> > +
> > +  MOZ_IMPLICIT Saturate(Saturate<T>&& aValue)
> > +    : mValue(aValue.mValue)
> 
> I think this is just |= default|, right?

I had problems with the Windows builds when using 'default' for move c'tors and assignments. So I wrote them by hand.

> @@ +140,5 @@
> > +  explicit Saturate(const T& aValue)
> > +    : mValue(aValue)
> > +  { }
> > +
> > +  const T& value() const
> 
> I'm not sure const T& here actually buys you anything...maybe just return T
> instead?

I was thinking that T could be any type, such as a math vector with internal storage allocations. Returning a copy could have significant overhead.

> @@ +171,5 @@
> > +  // Assignment operators
> > +
> > +  Saturate<T>& operator=(const Saturate<T>&) = default;
> > +
> > +  Saturate<T>& operator=(Saturate<T>&& aRhs)
> 
> This should just be |= default|, too.

Same Windows problem as with the c'tor.

> ::: mfbt/tests/TestSaturate.cpp
> @@ +65,5 @@
> > +uint32_t
> > +StartValue<uint32_t>()
> > +{
> > +  // Picking a value near middle of uint32_t's range.
> > +  return static_cast<uint8_t>(NumericLimits<int32_t>::max());
> 
> I think you could probably avoid all these specializations by:
> 
> template<typename T>
> T
> StartValue()
> {
>   // Pick a value near the middle of T's range.
>   return (NumericLimits<T>::max() + NumericLimits<T>::min()) / 2;
> }

That would work for the current tests, but I was afraid that special cases of T would fail. Let's say T could represent an unsigned char between [200,250]. In the previous reviews, you checked for potential out-of-range errors, so I tried to avoid such code.
Changes since v7:

  - cleaned up doc comment
  - fixed move c'tor of |SaturateOp|
  - non-const return type of |SaturateOp|'s assignment operator
Attachment #8666624 - Attachment is obsolete: true
Attachment #8671308 - Flags: review+
Changes since v4:

  - fix delivery mode
Attachment #8664818 - Attachment is obsolete: true
Attachment #8694806 - Flags: review+
Changes since v3:

  - prefixed filenames with 'Gonk'
Attachment #8713591 - Flags: review+
Attachment #8664808 - Attachment is obsolete: true
Changes since v4:

  - prefixed file and class names with 'Gonk'
Attachment #8664809 - Attachment is obsolete: true
Attachment #8713592 - Flags: review+
Changes since v5:

  - prefixed file and class names with 'Gonk'
Attachment #8665851 - Attachment is obsolete: true
Attachment #8713593 - Flags: review+
Changes since v3:

  - prefixed file and class names with 'Gonk'
  - use Hal::SystemService interfaces
Attachment #8664813 - Attachment is obsolete: true
Attachment #8713594 - Flags: review+
Changes since v5:

  - prefixed file and class names with 'Gonk'
  - removed |SensorsEventRunnable|
Attachment #8694806 - Attachment is obsolete: true
Attachment #8713595 - Flags: review+
backed on request from thomas
Flags: needinfo?(tzimmermann)
Changes since v6:

  - renamed |nsAutoTArray| to |AutoTArray|

The type was renamed in m-i shortly before the previous patch set got checked in.
Attachment #8713595 - Attachment is obsolete: true
Flags: needinfo?(tzimmermann)
Attachment #8715759 - Flags: review+
Depends on: 1246949
You need to log in before you can comment on or make changes to this bug.