Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2447973003:
[sensors] [win] Implement ambient light sensor for Windows platform (Closed)
|
Created:
4 years, 1 month ago by shalamov Modified:
4 years, 1 month ago Reviewers:
Reilly Grant (use Gerrit), Lei Zhang, darktears, timvolodine, miu, maksims (do not use this acc), Mikhail CC:
chromium-reviews, miu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sensors] [win] Implement ambient light sensor for Windows platform
This CL adds implementation for ambient light sensor on Windows platform
and introduces adaptation layer between Generic Sensor and Windows Sensor
APIs.
Three new classes are introduced:
* PlatformSensorProviderWin: creates PlatformSensorWin instances and manages
sensor thread where all Windows Sensor API COM objects are running.
* PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications
about sensor reading and state change updates. Lives on IPC thread.
* PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading
and state change updates. Lives on sensor thread.
Unit tests are added to test new functionality.
BUG=606766
Committed: https://crrev.com/d08cd09c1949eccd6bb6d93ef4ed7e91be272eea
Cr-Commit-Position: refs/heads/master@{#429536}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Fixes for review comments from Mikhail. #
Total comments: 32
Patch Set 3 : Fixes for comments from Reilly and Mikhail #
Total comments: 2
Patch Set 4 : Modify timestamp calculation as proposed by miu@ #
Total comments: 46
Patch Set 5 : Fixes for comments from Lei Zhang #
Total comments: 8
Patch Set 6 : Fixes for review comments #Patch Set 7 : Rebased to master #Messages
Total messages: 49 (25 generated)
Description was changed from ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 ========== to ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 ==========
alexander.shalamov@intel.com changed reviewers: + alexis.menard@intel.com, maksim.sisov@intel.com, mikhail.pozdnyakov@intel.com
PTAL
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2447973003/diff/1/device/BUILD.gn File device/BUILD.gn (right): https://codereview.chromium.org/2447973003/diff/1/device/BUILD.gn#newcode226 device/BUILD.gn:226: libs = [ "sensorsapi.lib" ] += ? https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:23: return nullptr; maybe logging here? https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:61: if (FAILED(hr) || !m_sensor_manager_) return (!FAILED(hr) && m_sensor_manager_); https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:85: sensor_thread_->Stop(); sensor_thread_.reset() ? https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:58: double lux = 0; nit: = 0.0 https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:24: class PlatformSensorReaderWin final : public base::NonThreadSafe { it probably should not be base::NonThreadSafe, whereas {Start, Stop}Sensor methods are thread-safe. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:38: SensorReading& reading)>; does this semantics come from platform? https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:46: unsigned long GetMinimalReportingIntervalMs() const; pls document which methods are thread-safe and which are not
https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:50: bool GetReaderInitParamsForSensor(mojom::SensorType type, would be better to have it like: std::unique_ptr<ReaderInitParams> ReaderInitParams::Create(mojom::SensorType type); https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:224: read_func_(std::move(params.reader_func)), can we keep params as a member?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2447973003/diff/1/device/BUILD.gn File device/BUILD.gn (right): https://codereview.chromium.org/2447973003/diff/1/device/BUILD.gn#newcode226 device/BUILD.gn:226: libs = [ "sensorsapi.lib" ] On 2016/10/26 06:40:15, Mikhail wrote: > += ? It is a first libs definition for win platform, so I can't do += https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:23: return nullptr; On 2016/10/26 06:40:15, Mikhail wrote: > maybe logging here? Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:61: if (FAILED(hr) || !m_sensor_manager_) On 2016/10/26 06:40:15, Mikhail wrote: > return (!FAILED(hr) && m_sensor_manager_); Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_provider_win.cc:85: sensor_thread_->Stop(); On 2016/10/26 06:40:15, Mikhail wrote: > sensor_thread_.reset() ? Thread can be reused after Stop is called. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:50: bool GetReaderInitParamsForSensor(mojom::SensorType type, On 2016/10/26 06:52:59, Mikhail wrote: > would be better to have it like: > > std::unique_ptr<ReaderInitParams> ReaderInitParams::Create(mojom::SensorType > type); Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:58: double lux = 0; On 2016/10/26 06:40:15, Mikhail wrote: > nit: = 0.0 Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.cc:224: read_func_(std::move(params.reader_func)), On 2016/10/26 06:52:59, Mikhail wrote: > can we keep params as a member? Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:24: class PlatformSensorReaderWin final : public base::NonThreadSafe { On 2016/10/26 06:40:15, Mikhail wrote: > it probably should not be base::NonThreadSafe, whereas {Start, Stop}Sensor > methods are thread-safe. Done. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:38: SensorReading& reading)>; On 2016/10/26 06:40:16, Mikhail wrote: > does this semantics come from platform? No, it is a signature of a function that should be implemented for every sensor type. Converts data from ISensorDataReport to SensorReading. I moved it to cpp file. https://codereview.chromium.org/2447973003/diff/1/device/generic_sensor/platf... device/generic_sensor/platform_sensor_reader_win.h:46: unsigned long GetMinimalReportingIntervalMs() const; On 2016/10/26 06:40:15, Mikhail wrote: > pls document which methods are thread-safe and which are not Done.
alexander.shalamov@intel.com changed reviewers: + reillyg@chromium.org, timvolodine@chromium.org
Reilly, Tim, could you please take a look?
lgtm https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:21: // Instances of this class must be created and destucted on sensor thread. nit: destucted on same thread. ?
On 2016/10/26 13:35:31, Mikhail wrote: > > https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... > File device/generic_sensor/platform_sensor_reader_win.h (right): > > https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... > device/generic_sensor/platform_sensor_reader_win.h:21: // Instances of this > class must be created and destucted on sensor thread. > nit: destucted on same thread. ? Also pls. update chrome/browser/about_flags.cc to enable sensors content feature on Win.
https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:390: .WillByDefault(Invoke([](REFSENSOR_TYPE_ID, ISensorCollection** result) { Here and also elsewhere such as above in SetSupportedSensor() you can combine EXPECT_CALL and ON_CALL statements like these into a single EXPECT_CALL because WillOnce() implies Times(1) if nothing else is specified: EXPECT_CALL(*m_sensor_manager_, GetSensorsByType(SENSOR_TYPE_AMBIENT_LIGHT, _)) .WillOnce(Invoke([](REFSENSOR_TYPE_ID, ISensorCollection** result) { *result = nullptr; return E_FAIL; })); https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:436: EXPECT_THAT(SUCCEEDED(hr), true); Comparisons to true can be fishy. Use EXPECT_TRUE(SUCCEEDED(hr)). https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.cc:64: return (!FAILED(hr) && m_sensor_manager_); nit: parens around the return value are unnecessary https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.cc:97: if (!HasSensors()) I don't see this function defined anywhere. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_win.h (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.h:63: base::win::ScopedComPtr<ISensorManager> m_sensor_manager_; s/m_// https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:33: // Gets value from the report for provided key. nit: space between "namespace {" and the first function in it. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:44: return true; else return false? https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:126: if (state != SensorState::SENSOR_STATE_READY || &&? https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:141: // milliseconds is substracted from current high resolution timestamp. I don't see what additional accuracy this gives us. I'm starting to wonder if we're using base::Time and base::TimeTicks correctly so I'm going to add a //base reviewer to check. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:173: PlatformSensorReaderWin* m_platform_sensor_; s/m_// https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:181: // Get reader function and reporting mode. The comments in this function are unnecessary. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:21: // Instances of this class must be created and destucted on sensor thread. s/destucted/destructed/ https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:63: friend class EventListener; nit: spaces between friends, fields and the DISALLOW_COPY_AND_ASSIGN macro. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:64: base::Lock lock_; Document why these fields (and which ones) must be protected by a lock. I'm assuming it's because of MTA? https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_win.cc:11: } // anonymous namespace Just, "namespace".
reillyg@chromium.org changed reviewers: + thestig@chromium.org
thestig, can you take a look at the use of base::Time and base::TimeTicks in platform_sensor_reader_win.cc:141. I think the goal is to convert between the system clock and the monotonic clock because that's what the base for a DOMHighResTimestamp is.
I would recommend miu@, as he is the Time lord. ;)
Patchset #3 (id:50001) has been deleted
Patchset #3 (id:70001) has been deleted
https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:390: .WillByDefault(Invoke([](REFSENSOR_TYPE_ID, ISensorCollection** result) { On 2016/10/27 00:59:19, Reilly Grant wrote: > Here and also elsewhere such as above in SetSupportedSensor() you can combine > EXPECT_CALL and ON_CALL statements like these into a single EXPECT_CALL because > WillOnce() implies Times(1) if nothing else is specified: > > EXPECT_CALL(*m_sensor_manager_, > GetSensorsByType(SENSOR_TYPE_AMBIENT_LIGHT, _)) > .WillOnce(Invoke([](REFSENSOR_TYPE_ID, ISensorCollection** result) { > *result = nullptr; > return E_FAIL; > })); Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:436: EXPECT_THAT(SUCCEEDED(hr), true); On 2016/10/27 00:59:19, Reilly Grant wrote: > Comparisons to true can be fishy. Use EXPECT_TRUE(SUCCEEDED(hr)). Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.cc:64: return (!FAILED(hr) && m_sensor_manager_); On 2016/10/27 00:59:19, Reilly Grant wrote: > nit: parens around the return value are unnecessary Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.cc:97: if (!HasSensors()) On 2016/10/27 00:59:19, Reilly Grant wrote: > I don't see this function defined anywhere. It is in the base class https://cs.chromium.org/chromium/src/device/generic_sensor/platform_sensor_pr... https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_provider_win.h (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_provider_win.h:63: base::win::ScopedComPtr<ISensorManager> m_sensor_manager_; On 2016/10/27 00:59:19, Reilly Grant wrote: > s/m_// Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:33: // Gets value from the report for provided key. On 2016/10/27 00:59:19, Reilly Grant wrote: > nit: space between "namespace {" and the first function in it. Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:44: return true; On 2016/10/27 00:59:19, Reilly Grant wrote: > else return false? Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:126: if (state != SensorState::SENSOR_STATE_READY || On 2016/10/27 00:59:19, Reilly Grant wrote: > &&? Thanks! Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:141: // milliseconds is substracted from current high resolution timestamp. On 2016/10/27 00:59:19, Reilly Grant wrote: > I don't see what additional accuracy this gives us. I'm starting to wonder if > we're using base::Time and base::TimeTicks correctly so I'm going to add a > //base reviewer to check. I'll wait for guidance from miu@, all this hassle is because, for some reason, Windows API returns sensor timestamp in system time, must be some monotonic high precision timestamp, not wallclock. Then, on blink side it is easy to convert it to DOMHighResTimeStamp. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:173: PlatformSensorReaderWin* m_platform_sensor_; On 2016/10/27 00:59:19, Reilly Grant wrote: > s/m_// Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:181: // Get reader function and reporting mode. On 2016/10/27 00:59:19, Reilly Grant wrote: > The comments in this function are unnecessary. Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:21: // Instances of this class must be created and destucted on sensor thread. On 2016/10/27 00:59:20, Reilly Grant wrote: > s/destucted/destructed/ Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:21: // Instances of this class must be created and destucted on sensor thread. On 2016/10/26 13:35:31, Mikhail wrote: > nit: destucted on same thread. ? Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:63: friend class EventListener; On 2016/10/27 00:59:20, Reilly Grant wrote: > nit: spaces between friends, fields and the DISALLOW_COPY_AND_ASSIGN macro. Done. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.h:64: base::Lock lock_; On 2016/10/27 00:59:19, Reilly Grant wrote: > Document why these fields (and which ones) must be protected by a lock. I'm > assuming it's because of MTA? Yes. MTA allows to use COM interfaces from different threads, yet, synchronization must be provided for concurrent access. https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_win.cc (right): https://codereview.chromium.org/2447973003/diff/40001/device/generic_sensor/p... device/generic_sensor/platform_sensor_win.cc:11: } // anonymous namespace On 2016/10/27 00:59:20, Reilly Grant wrote: > Just, "namespace". Done.
miu@chromium.org changed reviewers: + miu@chromium.org
Time lord jumping-in here. :) Comments on the time sampling/conversions (mostly things to consider for your use case): https://codereview.chromium.org/2447973003/diff/90001/device/generic_sensor/p... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/90001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:150: base::TimeTicks ticks_now = base::TimeTicks::Now(); 1. How precise a measurement do you need? When sampling the two clocks, it's possible for things like thread-context switching to cause 50+ ms to elapse between each sampling (we hit this issue in the unit tests for base::Time, and had to do special things to mitigate). Is this amount of error acceptable to your use cases? 2. How often is OnDataUpdated() called? FWICT, this would probably be no more than a few times in a second, so what you're doing here should be okay. If it's more frequent, you may need to consider a "fancier" solution here. ;-) https://codereview.chromium.org/2447973003/diff/90001/device/generic_sensor/p... device/generic_sensor/platform_sensor_reader_win.cc:170: reading.timestamp = (ticks_now - (delta + base::TimeTicks())).InSecondsF(); 1. Semantically, it seems the parentheses are in the wrong place here; although, the math itself does the right thing. I think this statement should be: reading.timestamp = ((ticks_now - delta) - base::TimeTicks()).InSecondsF(); 2. IMHO, the SensorReading::timestamp field should be of type base::TimeTicks (this is allowed in Blink code), and the conversion to Blink's performance time data type should occur at the last possible moment in blink::SensorReading::timestamp(). This has the advantage that *all* time data type/value translation is in one code module (right now it is split between here and the other file). In general, in Chromium code, try to keep time values in the base::TimeXXX types; and only map to other data types when necessary. 3. base::Time is not a monotonically increasing clock, and it may have jumped around in some unpredictable way (between when the report's timestamp was sampled and when time_now was sampled). Therefore, it's possible that the TimeTicks values you're computing (based on them) will not be monotonically increasing. You should consider adding some code to check that. Something like: if (last_sensor_reading_.timestamp > reading.timestamp) { reading.timestamp = last_sensor_reading_.timestamp; // Or, maybe it would be better to not report this new reading? Depends on your intended use cases... }
Thanks for comments miu@!
> 1. How precise a measurement do you need? When sampling the two clocks, it's
> possible for things like thread-context switching to cause 50+ ms to elapse
> between each sampling (we hit this issue in the unit tests for base::Time, and
> had to do special things to mitigate). Is this amount of error acceptable to
> your use cases?
I think it is acceptable, especially for slow sensors like ambient light or
proximity.
> 2. How often is OnDataUpdated() called? FWICT, this would probably be no more
> than a few times in a second, so what you're doing here should be okay. If
it's
> more frequent, you may need to consider a "fancier" solution here. ;-)
For ambient light sensor it is not that important. Accelerometer, gyro and
magnetometer are capped to 60Hz and might require some improvements in the
future.
> 1. Semantically, it seems the parentheses are in the wrong place here;
although,
> the math itself does the right thing. I think this statement should be:
> reading.timestamp = ((ticks_now - delta) - base::TimeTicks()).InSecondsF();
Thanks, modified code as you proposed.
> 2. IMHO, the SensorReading::timestamp field should be of type base::TimeTicks
device::SensorReading structure is written directly to shared memory, that is
why we use double for timestamp.
> 3. base::Time is not a monotonically increasing clock, and it may have jumped
> around in some unpredictable way (between when the report's timestamp was
> sampled and when time_now was sampled). Therefore, it's possible that the
> TimeTicks values you're computing (based on them) will not be monotonically
> increasing. You should consider adding some code to check that. Something
like:
>
> if (last_sensor_reading_.timestamp > reading.timestamp) {
> reading.timestamp = last_sensor_reading_.timestamp;
> // Or, maybe it would be better to not report this new reading? Depends on
> your intended use cases...
> }
We discussed about that with Mikhail and decided that it would be better to drop
event instead of having different readings with same timestamp.
Time logic in device/generic_sensor/platform_sensor_reader_win.cc lgtm.
https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/BUILD.gn:72: "portabledeviceguids.lib", nit: alphabetical order https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:12: nit: no blank lines between all the non-system includes. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:216: run_loop_ = std::make_unique<base::RunLoop>(); Use nullptr like line 223? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); auto* https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); And who frees |mock_report| ? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:334: std::unique_ptr<base::MessageLoop> message_loop_; Can this just be a base::MessageLoopForIO? (not an unique_ptr) https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:406: auto client = std::make_unique<NiceMock<MockPlatformSensorClient>>(sensor); We're using base::MakeUnique for now. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:22: if (base::win::GetVersion() < base::win::VERSION_WIN7) { Modern Chromium only supports Win7 and newer. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:64: return !FAILED(hr) && sensor_manager_; Why not SUCCEEDED(hr)? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:73: sensor_thread_.reset(new base::Thread("Windows platform sensor thread")); base::MakeUnique https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.h:36: void SetSensorManagerForTesing( type: ForTesting https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.h:49: PlatformSensorProviderWin(); separate the ctor for the rest. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:85: } // namespace nit: blank line before https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:183: PlatformSensorReaderWin* platform_sensor_reader_; You can do: PlatformSensorReaderWin* const platform_sensor_reader_; Which combined with the DCHECK() in the ctor, means readers of this code will know this pointer is never a nullptr. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:215: return base::WrapUnique( Can this be base::MakeUnique()? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:232: if (FAILED(hr) || count == 0) Since you return |sensor| either way here: if (SUCCEEDED(hr) && count > 0) sensor_collection->GetAt(0, sensor.Receive()); return sensor; https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.h:65: base::Lock lock_; Can you group this with the member variables it protects. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:10: const double kDefaultSensorReportingFrequency = 5.0; constexpr https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:32: DCHECK(sensor_reader_); Already checked in the ctor. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:60: DCHECK(sensor_reader_); Already checked in the ctor, plus we usually make sure we are on the right thread first before doing other checks. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:67: 1 / (minimal_reporting_interval_ms / base::Time::kMillisecondsPerSecond); Why can't this be just: kMillisecondsPerSecond / interval_ms ? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.h:43: ~PlatformSensorWin() override; nit: blank line after
Patchset #5 (id:130001) has been deleted
Thanks for looking Lei Zhang, fixed most of the comments, except: device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:216: run_loop_ = std::make_unique<base::RunLoop>(); On 2016/10/29 05:10:41, Lei Zhang wrote: > Use nullptr like line 223? https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/BUILD.gn (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/BUILD.gn:72: "portabledeviceguids.lib", On 2016/10/29 05:10:41, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:12: On 2016/10/29 05:10:41, Lei Zhang wrote: > nit: no blank lines between all the non-system includes. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:216: run_loop_ = std::make_unique<base::RunLoop>(); On 2016/10/29 05:10:41, Lei Zhang wrote: > Use nullptr like line 223? It is inner loop that is used at line 220. I cannot make it nullptr here. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); On 2016/10/29 05:10:41, Lei Zhang wrote: > auto* Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); On 2016/10/29 05:10:41, Lei Zhang wrote: > And who frees |mock_report| ? MockISensorDataReport is derived from IUnknownImpl that provides ref counting. The |mock_report| is wrapped by base::win::ScopedComPtr<ISensorDataReport> data_report and released when function exits. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:334: std::unique_ptr<base::MessageLoop> message_loop_; On 2016/10/29 05:10:41, Lei Zhang wrote: > Can this just be a base::MessageLoopForIO? (not an unique_ptr) Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:406: auto client = std::make_unique<NiceMock<MockPlatformSensorClient>>(sensor); On 2016/10/29 05:10:41, Lei Zhang wrote: > We're using base::MakeUnique for now. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:22: if (base::win::GetVersion() < base::win::VERSION_WIN7) { On 2016/10/29 05:10:41, Lei Zhang wrote: > Modern Chromium only supports Win7 and newer. I removed check. Thanks. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:64: return !FAILED(hr) && sensor_manager_; On 2016/10/29 05:10:42, Lei Zhang wrote: > Why not SUCCEEDED(hr)? Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:73: sensor_thread_.reset(new base::Thread("Windows platform sensor thread")); On 2016/10/29 05:10:41, Lei Zhang wrote: > base::MakeUnique Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.h:36: void SetSensorManagerForTesing( On 2016/10/29 05:10:42, Lei Zhang wrote: > type: ForTesting Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.h:49: PlatformSensorProviderWin(); On 2016/10/29 05:10:42, Lei Zhang wrote: > separate the ctor for the rest. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:85: } // namespace On 2016/10/29 05:10:42, Lei Zhang wrote: > nit: blank line before Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:183: PlatformSensorReaderWin* platform_sensor_reader_; On 2016/10/29 05:10:42, Lei Zhang wrote: > You can do: PlatformSensorReaderWin* const platform_sensor_reader_; > > Which combined with the DCHECK() in the ctor, means readers of this code will > know this pointer is never a nullptr. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:215: return base::WrapUnique( On 2016/10/29 05:10:42, Lei Zhang wrote: > Can this be base::MakeUnique()? Constructor is private, can't use base::MakeUnique here. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:232: if (FAILED(hr) || count == 0) On 2016/10/29 05:10:42, Lei Zhang wrote: > Since you return |sensor| either way here: > > if (SUCCEEDED(hr) && count > 0) > sensor_collection->GetAt(0, sensor.Receive()); > return sensor; Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.h:65: base::Lock lock_; On 2016/10/29 05:10:42, Lei Zhang wrote: > Can you group this with the member variables it protects. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:10: const double kDefaultSensorReportingFrequency = 5.0; On 2016/10/29 05:10:42, Lei Zhang wrote: > constexpr Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:32: DCHECK(sensor_reader_); On 2016/10/29 05:10:42, Lei Zhang wrote: > Already checked in the ctor. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:60: DCHECK(sensor_reader_); On 2016/10/29 05:10:42, Lei Zhang wrote: > Already checked in the ctor, plus we usually make sure we are on the right > thread first before doing other checks. Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.cc:67: 1 / (minimal_reporting_interval_ms / base::Time::kMillisecondsPerSecond); On 2016/10/29 05:10:42, Lei Zhang wrote: > Why can't this be just: kMillisecondsPerSecond / interval_ms ? Done. https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.h (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.h:43: ~PlatformSensorWin() override; On 2016/10/29 05:10:42, Lei Zhang wrote: > nit: blank line after Done.
https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); On 2016/10/31 13:12:57, shalamov wrote: > On 2016/10/29 05:10:41, Lei Zhang wrote: > > And who frees |mock_report| ? > > MockISensorDataReport is derived from IUnknownImpl that provides ref counting. > The |mock_report| is wrapped by base::win::ScopedComPtr<ISensorDataReport> > data_report and released when function exits. So it's self releasing? Please add a comment, because it is not obvious from just reading this function by itself.
lgtm with nits https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:68: base::MakeUnique<base::Thread>("Windows platform sensor thread"); As with the Linux patch, you might as well call this just the sensor thread since it's obvious that it's the Windows version. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:206: sensor->GetProperty(SENSOR_PROPERTY_MIN_REPORT_INTERVAL, &variant))) { nit: since this had to get split across two lines you might as well move the method call to its own line for better readability: HRESULT hr = sensor->GetProperty(SENSOR_PROPERTY_MIN_REPORT_INTERVAL, &variant); if (SUCCEEDED(hr) && variant.vt == VT_UI4) params->min_reporting_interval_ms = variant.ulVal; https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:301: if (SUCCEEDED(props->SetUnsignedIntegerValue( Same here. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.h (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.h:53: std::unique_ptr<PlatformSensorReaderWin> sensor_reader_; Since sensor_reader_ is always destroyed by posting a task to sensor_thread_runner_ I like to document this fact by making the field a raw pointer, since the unique_ptr destructor will never actually do anything.
Fixed review comments. Added one test for the issue that Reilly found in PlatformSensorReaderWin.cpp:130 https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... File device/generic_sensor/platform_sensor_and_provider_unittest_win.cc (right): https://codereview.chromium.org/2447973003/diff/110001/device/generic_sensor/... device/generic_sensor/platform_sensor_and_provider_unittest_win.cc:308: auto mock_report = new NiceMock<MockISensorDataReport>(); On 2016/10/31 18:22:16, Lei Zhang wrote: > On 2016/10/31 13:12:57, shalamov wrote: > > On 2016/10/29 05:10:41, Lei Zhang wrote: > > > And who frees |mock_report| ? > > > > MockISensorDataReport is derived from IUnknownImpl that provides ref counting. > > The |mock_report| is wrapped by base::win::ScopedComPtr<ISensorDataReport> > > data_report and released when function exits. > > So it's self releasing? Please add a comment, because it is not obvious from > just reading this function by itself. Added comments. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_provider_win.cc (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_provider_win.cc:68: base::MakeUnique<base::Thread>("Windows platform sensor thread"); On 2016/10/31 21:49:43, Reilly Grant wrote: > As with the Linux patch, you might as well call this just the sensor thread > since it's obvious that it's the Windows version. Done. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_reader_win.cc (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:206: sensor->GetProperty(SENSOR_PROPERTY_MIN_REPORT_INTERVAL, &variant))) { On 2016/10/31 21:49:43, Reilly Grant wrote: > nit: since this had to get split across two lines you might as well move the > method call to its own line for better readability: > > HRESULT hr = sensor->GetProperty(SENSOR_PROPERTY_MIN_REPORT_INTERVAL, &variant); > if (SUCCEEDED(hr) && variant.vt == VT_UI4) > params->min_reporting_interval_ms = variant.ulVal; Done. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_reader_win.cc:301: if (SUCCEEDED(props->SetUnsignedIntegerValue( On 2016/10/31 21:49:43, Reilly Grant wrote: > Same here. Done. https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... File device/generic_sensor/platform_sensor_win.h (right): https://codereview.chromium.org/2447973003/diff/150001/device/generic_sensor/... device/generic_sensor/platform_sensor_win.h:53: std::unique_ptr<PlatformSensorReaderWin> sensor_reader_; On 2016/10/31 21:49:43, Reilly Grant wrote: > Since sensor_reader_ is always destroyed by posting a task to > sensor_thread_runner_ I like to document this fact by making the field a raw > pointer, since the unique_ptr destructor will never actually do anything. Done.
The CQ bit was checked by alexander.shalamov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2016/10/31 18:22:16, Lei Zhang wrote: > So it's self releasing? Please add a comment, because it is not obvious from > just reading this function by itself. thestig@ I added comments, is that enough?
On 2016/11/02 07:04:03, shalamov wrote: > On 2016/10/31 18:22:16, Lei Zhang wrote: > > So it's self releasing? Please add a comment, because it is not obvious from > > just reading this function by itself. > > thestig@ I added comments, is that enough? Yes, I don't have any more comments. chrome/browser/about_flags.cc lgtm as well.
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexander.shalamov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from mikhail.pozdnyakov@intel.com, miu@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2447973003/#ps190001 (title: "Rebased to master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 ========== to ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 ========== to ========== [sensors] [win] Implement ambient light sensor for Windows platform This CL adds implementation for ambient light sensor on Windows platform and introduces adaptation layer between Generic Sensor and Windows Sensor APIs. Three new classes are introduced: * PlatformSensorProviderWin: creates PlatformSensorWin instances and manages sensor thread where all Windows Sensor API COM objects are running. * PlatformSensorWin: Uses PlatformSensorReaderWin to receive notifications about sensor reading and state change updates. Lives on IPC thread. * PlatformSensorReaderWin - Uses ISensor COM interface to get sensor reading and state change updates. Lives on sensor thread. Unit tests are added to test new functionality. BUG=606766 Committed: https://crrev.com/d08cd09c1949eccd6bb6d93ef4ed7e91be272eea Cr-Commit-Position: refs/heads/master@{#429536} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d08cd09c1949eccd6bb6d93ef4ed7e91be272eea Cr-Commit-Position: refs/heads/master@{#429536} |
