Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529343002:
[sensors] Fix flakiness in generic sensors tests (Closed)
Description[sensors] Fix flakiness in generic sensors tests
This patch fixes flakiness for generic sensors layout tests by resetting
SensorProvider mock stub bindings at the end of the each layout test and
re-initializing it when needed in SensorProviderProxy.
Before this CL, if multiple sensor tests were executed by the same
content shell instance, Frame's mojo interface overrides were reset after
each test, yet SensorProviderProxy was not aware of it and kept using old
SensorProvider mojom proxy.
BUG=606766
Committed: https://crrev.com/8825a7140545d858daa36defc294668a79ecf1bf
Cr-Commit-Position: refs/heads/master@{#435187}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixes for review comments from Mikhail #
Total comments: 2
Patch Set 3 : result => providerProxy #
Total comments: 6
Patch Set 4 : Fixes for review comments from Reilly #
Messages
Total messages: 32 (19 generated)
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...
alexander.shalamov@intel.com changed reviewers: + haraken@chromium.org, mikhail.pozdnyakov@intel.com, reillyg@chromium.org
Please take a look.
LGTM on my side
https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:42: result->initializeSensorProviderIfNeeded(frame); maybe split methods, like "isInitialized() const" + "initialize()", like we have for SensorProxy. Then, if (!provider->isInitialized()) provider->initialize() and call 'initialize()' from ctor. https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:78: device::mojom::blink::SensorProvider* SensorProviderProxy::getSensorProvider() why move it here (and rename)? IMO it's fine to keep it inline.
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 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...
https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:42: result->initializeSensorProviderIfNeeded(frame); On 2016/11/28 10:42:51, Mikhail wrote: > maybe split methods, like "isInitialized() const" + "initialize()", like we have > for SensorProxy. Then, > > if (!provider->isInitialized()) > provider->initialize() > > and call 'initialize()' from ctor. Done. https://codereview.chromium.org/2529343002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:78: device::mojom::blink::SensorProvider* SensorProviderProxy::getSensorProvider() On 2016/11/28 10:42:51, Mikhail wrote: > why move it here (and rename)? IMO it's fine to keep it inline. Wanted to make lazy getter, but then noticed that frame is required. Forgot to move it back. Done.
lgtm https://codereview.chromium.org/2529343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:35: SensorProviderProxy* result = static_cast<SensorProviderProxy*>( nit: pls rename 'result' to 'providerProxy'
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 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...
https://codereview.chromium.org/2529343002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:35: SensorProviderProxy* result = static_cast<SensorProviderProxy*>( On 2016/11/28 13:16:08, Mikhail wrote: > nit: pls rename 'result' to 'providerProxy' Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits and a question that doesn't have to be resolved in this change. https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js:294: } nit: unnecessary braces https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:33: SensorProviderProxy* SensorProviderProxy::from(LocalFrame* frame) { nit: This function should have a "// static" marker before it. https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:42: friend class SensorProxy; // To call getSensorProvider(). If this class expects someone to call getSensorProvider() on it why isn't it a public function?
https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/sensor/resources/sensor-helpers.js:294: } On 2016/11/28 19:40:33, Reilly Grant wrote: > nit: unnecessary braces Done. https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.cpp:33: SensorProviderProxy* SensorProviderProxy::from(LocalFrame* frame) { On 2016/11/28 19:40:33, Reilly Grant wrote: > nit: This function should have a "// static" marker before it. Done. https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h (right): https://codereview.chromium.org/2529343002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/sensor/SensorProviderProxy.h:42: friend class SensorProxy; // To call getSensorProvider(). On 2016/11/28 19:40:33, Reilly Grant wrote: > If this class expects someone to call getSensorProvider() on it why isn't it a > public function? It is private just to make interaction between mojom::SensorProvider interface and rest of the blink code little stricter / manageable. The SensorProxy is the only class that can request sensor instance from the platform and all communication with mojom::Sensor is done thru SensorProxy.
The CQ bit was checked by alexander.shalamov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, mikhail.pozdnyakov@intel.com, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2529343002/#ps60001 (title: "Fixes for review comments from Reilly")
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
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_...)
The CQ bit was checked by alexander.shalamov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480491048035970,
"parent_rev": "f2bec37dc9d81d85f8112f2bdf7c6313888e4cd0", "commit_rev":
"85008947569a213fc063ff530427a716265ac4b7"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [sensors] Fix flakiness in generic sensors tests This patch fixes flakiness for generic sensors layout tests by resetting SensorProvider mock stub bindings at the end of the each layout test and re-initializing it when needed in SensorProviderProxy. Before this CL, if multiple sensor tests were executed by the same content shell instance, Frame's mojo interface overrides were reset after each test, yet SensorProviderProxy was not aware of it and kept using old SensorProvider mojom proxy. BUG=606766 ========== to ========== [sensors] Fix flakiness in generic sensors tests This patch fixes flakiness for generic sensors layout tests by resetting SensorProvider mock stub bindings at the end of the each layout test and re-initializing it when needed in SensorProviderProxy. Before this CL, if multiple sensor tests were executed by the same content shell instance, Frame's mojo interface overrides were reset after each test, yet SensorProviderProxy was not aware of it and kept using old SensorProvider mojom proxy. BUG=606766 Committed: https://crrev.com/8825a7140545d858daa36defc294668a79ecf1bf Cr-Commit-Position: refs/heads/master@{#435187} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8825a7140545d858daa36defc294668a79ecf1bf Cr-Commit-Position: refs/heads/master@{#435187} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
