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)
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.
Assignee | ||
Comment 1•9 years ago
|
||
I work on this and will post patches soon.
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8649832 -
Flags: review?(shuang)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8649833 -
Flags: review?(shuang)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8649834 -
Flags: review?(shuang)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8649835 -
Flags: review?(gsvelto)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8649836 -
Flags: review?(gsvelto)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8649837 -
Flags: review?(gsvelto)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8649838 -
Flags: review?(gsvelto)
Assignee | ||
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Attachment #8649834 -
Flags: review?(shuang) → review+
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 14•9 years ago
|
||
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 15•9 years ago
|
||
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-
Assignee | ||
Comment 16•9 years ago
|
||
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.
Assignee | ||
Comment 17•9 years ago
|
||
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.
Comment 18•9 years ago
|
||
(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.
Comment 19•9 years ago
|
||
(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 20•9 years ago
|
||
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 21•9 years ago
|
||
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 22•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
Changes since v1: - incorporate protocol changes from bug 1196221
Attachment #8649836 -
Attachment is obsolete: true
Attachment #8657129 -
Flags: review-
Assignee | ||
Comment 24•9 years ago
|
||
Changes since v1: - incorporate protocol changes from bug 1196221
Attachment #8657130 -
Flags: review-
Assignee | ||
Comment 25•9 years ago
|
||
Changes since v1: - incorporate protocol changes from bug 1196221
Attachment #8649841 -
Attachment is obsolete: true
Attachment #8657131 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8649837 -
Attachment is obsolete: true
Assignee | ||
Comment 26•9 years ago
|
||
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.
Assignee | ||
Comment 27•9 years ago
|
||
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.
Assignee | ||
Comment 28•9 years ago
|
||
> ::: 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.
Assignee | ||
Comment 29•9 years ago
|
||
(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.
Assignee | ||
Comment 30•9 years ago
|
||
(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.
Comment 31•9 years ago
|
||
(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.
Assignee | ||
Comment 32•9 years ago
|
||
(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.
Comment 33•9 years ago
|
||
(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.
Assignee | ||
Comment 34•9 years ago
|
||
(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.
Assignee | ||
Comment 35•9 years ago
|
||
FYI: I currently work on this patch set. I expect to post some updates during the next days.
Comment 36•9 years ago
|
||
(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.
Assignee | ||
Comment 37•9 years ago
|
||
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)
Assignee | ||
Comment 38•9 years ago
|
||
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)
Assignee | ||
Comment 39•9 years ago
|
||
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)
Assignee | ||
Comment 40•9 years ago
|
||
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)
Assignee | ||
Comment 41•9 years ago
|
||
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)
Assignee | ||
Comment 42•9 years ago
|
||
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)
Assignee | ||
Comment 43•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dbd89d62101
Assignee | ||
Comment 44•9 years ago
|
||
Changes since v2: - removed out-commented code
Attachment #8659859 -
Attachment is obsolete: true
Attachment #8659859 -
Flags: review?(nfroyd)
Attachment #8659872 -
Flags: review?(nfroyd)
Assignee | ||
Comment 45•9 years ago
|
||
Changes since v3: - fixed patch headline
Attachment #8659872 -
Attachment is obsolete: true
Attachment #8659872 -
Flags: review?(nfroyd)
Attachment #8659892 -
Flags: review?(nfroyd)
Comment 46•9 years ago
|
||
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 47•9 years ago
|
||
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 48•9 years ago
|
||
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 49•9 years ago
|
||
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 50•9 years ago
|
||
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 51•9 years ago
|
||
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 52•9 years ago
|
||
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+
Assignee | ||
Comment 53•9 years ago
|
||
(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.
Comment 54•9 years ago
|
||
(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.
Assignee | ||
Comment 55•9 years ago
|
||
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<>|
Assignee | ||
Comment 56•9 years ago
|
||
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
Assignee | ||
Comment 57•9 years ago
|
||
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+
Assignee | ||
Comment 58•9 years ago
|
||
Changes since v3: - JavaDoc comments
Attachment #8659864 -
Attachment is obsolete: true
Attachment #8664809 -
Flags: review+
Assignee | ||
Comment 59•9 years ago
|
||
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+
Assignee | ||
Comment 60•9 years ago
|
||
Changes since v2: - JavaDoc comments
Attachment #8659869 -
Attachment is obsolete: true
Attachment #8664813 -
Flags: review+
Assignee | ||
Comment 61•9 years ago
|
||
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+
Assignee | ||
Comment 62•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a40d03262d1b
Assignee | ||
Comment 63•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ccd052f77d2
Assignee | ||
Comment 64•9 years ago
|
||
Changes since v4: - don't shadow class members with local variables (see bug 1207030)
Attachment #8664811 -
Attachment is obsolete: true
Attachment #8665851 -
Flags: review+
Assignee | ||
Comment 65•9 years ago
|
||
Changes since v5: - don't shadow class members with local variables - fixed type-range assertions in |TestAll|
Attachment #8664806 -
Attachment is obsolete: true
Assignee | ||
Comment 66•9 years ago
|
||
Another push to try; the previous one interfered with bug 1207030. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4a8078bdaf7
Assignee | ||
Comment 67•9 years ago
|
||
Changes since v6: - implement move constructor/assignment because 'default' is not supported on Windows
Attachment #8665852 -
Attachment is obsolete: true
Assignee | ||
Comment 68•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=24cc154ba364
Assignee | ||
Comment 69•9 years ago
|
||
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)
Comment 70•9 years ago
|
||
(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 71•9 years ago
|
||
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+
Assignee | ||
Comment 72•9 years ago
|
||
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.
Assignee | ||
Comment 73•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad10061bd3e1
Assignee | ||
Comment 74•9 years ago
|
||
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+
Assignee | ||
Comment 75•8 years ago
|
||
Changes since v4: - fix delivery mode
Attachment #8664818 -
Attachment is obsolete: true
Attachment #8694806 -
Flags: review+
Assignee | ||
Comment 76•8 years ago
|
||
Changes since v3: - prefixed filenames with 'Gonk'
Attachment #8713591 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8664808 -
Attachment is obsolete: true
Assignee | ||
Comment 77•8 years ago
|
||
Changes since v4: - prefixed file and class names with 'Gonk'
Attachment #8664809 -
Attachment is obsolete: true
Attachment #8713592 -
Flags: review+
Assignee | ||
Comment 78•8 years ago
|
||
Changes since v5: - prefixed file and class names with 'Gonk'
Attachment #8665851 -
Attachment is obsolete: true
Attachment #8713593 -
Flags: review+
Assignee | ||
Comment 79•8 years ago
|
||
Changes since v3: - prefixed file and class names with 'Gonk' - use Hal::SystemService interfaces
Attachment #8664813 -
Attachment is obsolete: true
Attachment #8713594 -
Flags: review+
Assignee | ||
Comment 80•8 years ago
|
||
Changes since v5: - prefixed file and class names with 'Gonk' - removed |SensorsEventRunnable|
Attachment #8694806 -
Attachment is obsolete: true
Attachment #8713595 -
Flags: review+
Comment 81•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6c228ef008 https://hg.mozilla.org/integration/mozilla-inbound/rev/bbadd0a1367d https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a5a6174748 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5cf57e8b67 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7dbf51f1e8 https://hg.mozilla.org/integration/mozilla-inbound/rev/f817794c422b https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5db0961a0a https://hg.mozilla.org/integration/mozilla-inbound/rev/15a9b3c68084 https://hg.mozilla.org/integration/mozilla-inbound/rev/8adcb4e83cf4
Assignee | ||
Comment 82•8 years ago
|
||
https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=8adcb4e83cf4
Comment 83•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/567dc1cdc9e0 https://hg.mozilla.org/integration/mozilla-inbound/rev/40380997f78f https://hg.mozilla.org/integration/mozilla-inbound/rev/13c88df44978 https://hg.mozilla.org/integration/mozilla-inbound/rev/21ed650f0f8a https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b46679b8f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/da2258abf6ea https://hg.mozilla.org/integration/mozilla-inbound/rev/2582d2b8f75f https://hg.mozilla.org/integration/mozilla-inbound/rev/12acc8ed602e https://hg.mozilla.org/integration/mozilla-inbound/rev/454ca590e200
Assignee | ||
Comment 85•8 years ago
|
||
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+
Comment 86•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/56072bca8401 https://hg.mozilla.org/integration/mozilla-inbound/rev/e25fe2724b9e https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1561a18ce9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb592d6e508 https://hg.mozilla.org/integration/mozilla-inbound/rev/67a40851409d https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd47c4c4cd6 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd570dc405ce https://hg.mozilla.org/integration/mozilla-inbound/rev/303d6630e698 https://hg.mozilla.org/integration/mozilla-inbound/rev/df9079dc3f01
Assignee | ||
Comment 87•8 years ago
|
||
https://treeherder.allizom.org/#/jobs?repo=mozilla-inbound&revision=df9079dc3f01
Comment 88•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/56072bca8401 https://hg.mozilla.org/mozilla-central/rev/e25fe2724b9e https://hg.mozilla.org/mozilla-central/rev/2a1561a18ce9 https://hg.mozilla.org/mozilla-central/rev/fdb592d6e508 https://hg.mozilla.org/mozilla-central/rev/67a40851409d https://hg.mozilla.org/mozilla-central/rev/8dd47c4c4cd6 https://hg.mozilla.org/mozilla-central/rev/dd570dc405ce https://hg.mozilla.org/mozilla-central/rev/303d6630e698 https://hg.mozilla.org/mozilla-central/rev/df9079dc3f01
You need to log in
before you can comment on or make changes to this bug.
Description
•