Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1312123003:
Add browsertests for //device/vibration. (Closed)
|
Created:
5 years, 3 months ago by leonhsl(Using Gerrit) Modified:
5 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, haltonhuo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd browsertests for //device/vibration.
Created integration test to verify that VibrationManager mojo service
implementation is correctly exposed to the renderer.
Todo: Create browsertests for Java impl VibrationManager service
on android.
BUG=
Committed: https://crrev.com/64c1206dcc7df116ab14fabe86580c354f67fb26
Cr-Commit-Position: refs/heads/master@{#352838}
Patch Set 1 #Patch Set 2 : Fixed tryserver failure #
Total comments: 13
Patch Set 3 : Rebase only #Patch Set 4 : Address comments #Patch Set 5 : Move test file #
Messages
Total messages: 30 (8 generated)
leon.han@intel.com changed reviewers: + timvolodine@chromium.org
Created integration test to verify VibrationManager service exposure. Would you please take a look on this for now, while I'm investigating how to create a javatests specific for VibrationManager java impl on android. Thanks~
On 2015/08/28 03:03:41, leon.han wrote: > Created integration test to verify VibrationManager service exposure. > Would you please take a look on this for now, while I'm investigating how to > create a javatests specific for VibrationManager java impl on android. Thanks~ Have confirmed the tests PASS.
On 2015/08/28 03:06:22, leon.han wrote: > On 2015/08/28 03:03:41, leon.han wrote: > > Created integration test to verify VibrationManager service exposure. > > Would you please take a look on this for now, while I'm investigating how to > > create a javatests specific for VibrationManager java impl on android. Thanks~ > > Have confirmed the tests PASS. looks like some issues with the bots ;)
Uploaded patch #2 to fix tryserver failure. PTAL, Thanks~ Main update: Let test code use its own MessageLoopRunner to wait until VibrationManager API got called.
ping~
https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... File content/browser/vibration/vibration_manager_integration_browsertest.cc (right): https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:117: g_wait_vibrate_runner->Run(); would base::WaitableEvent be a cleaner solution here? message loops can be tricky.. that way I think you could also get rid of globals.
Thanks a lot for kindly comments. https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... File content/browser/vibration/vibration_manager_integration_browsertest.cc (right): https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:117: g_wait_vibrate_runner->Run(); On 2015/09/11 17:44:04, timvolodine wrote: > would base::WaitableEvent be a cleaner solution here? message loops can be > tricky.. that way I think you could also get rid of globals. Because both browser test code and the VibrationManager service run on the same thread(main thread), so I used message loops which can do async waiting, this is a mimic of TestNavigationObserver implementation.. ;) And, as FakeVibrationManager instance is created dynamically on upon of mojo connection, test code can't get its reference, so I put some variables globally instead of putting them into FakeVibrationManager.
ping~ WDYT?
lgtm modulo comments https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... File content/browser/vibration/vibration_manager_integration_browsertest.cc (right): https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:6: #include "content/public/browser/web_contents.h" is this needed? https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:7: #include "content/public/common/content_client.h" and this? https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:19: // That is, they verify that the service implementation is correctly exposed to nit: maybe put on previous line for better readability https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:109: ResetGlobalValues(); should this be in SetUp somewhere? https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:117: g_wait_vibrate_runner->Run(); On 2015/09/14 10:35:34, leon.han wrote: > On 2015/09/11 17:44:04, timvolodine wrote: > > would base::WaitableEvent be a cleaner solution here? message loops can be > > tricky.. that way I think you could also get rid of globals. > > Because both browser test code and the VibrationManager service run on the same > thread(main thread), so I used message loops which can do async waiting, this is > a mimic of TestNavigationObserver implementation.. ;) > And, as FakeVibrationManager instance is created dynamically on upon of mojo > connection, test code can't get its reference, so I put some variables globally > instead of putting them into FakeVibrationManager. ok https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:125: ResetGlobalValues(); same question
Thanks a lot for kindly review :) Addressed all comments. https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... File content/browser/vibration/vibration_manager_integration_browsertest.cc (right): https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:6: #include "content/public/browser/web_contents.h" On 2015/09/28 15:05:53, timvolodine wrote: > is this needed? Acknowledged. Removed. https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:7: #include "content/public/common/content_client.h" On 2015/09/28 15:05:53, timvolodine wrote: > and this? Acknowledged. Removed. https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:19: // That is, they verify that the service implementation is correctly exposed to On 2015/09/28 15:05:53, timvolodine wrote: > nit: maybe put on previous line for better readability Done. https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:109: ResetGlobalValues(); On 2015/09/28 15:05:53, timvolodine wrote: > should this be in SetUp somewhere? Acknowledged. Moved into SetUpOnMainThread(). https://codereview.chromium.org/1312123003/diff/20001/content/browser/vibrati... content/browser/vibration/vibration_manager_integration_browsertest.cc:125: ResetGlobalValues(); On 2015/09/28 15:05:53, timvolodine wrote: > same question Acknowledged. Moved into SetUpOnMainThread().
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312123003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312123003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
leon.han@intel.com changed reviewers: + jam@chromium.org
Hi, jam@, PTAL as OWNER review. Thanks~
On 2015/09/29 09:28:35, leon.han wrote: > Hi, jam@, PTAL as OWNER review. Thanks~ no need to create a subdirectory for just one test file. just put it in content/browser.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312123003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312123003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/09/29 19:42:08, jam wrote: > On 2015/09/29 09:28:35, leon.han wrote: > > Hi, jam@, PTAL as OWNER review. Thanks~ > > no need to create a subdirectory for just one test file. just put it in > content/browser. Done. Thanks~
ping~ jam@
On 2015/10/06 10:54:01, leon.han wrote: > ping~ jam@ sorry, I completely missed this. lgtm
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from timvolodine@chromium.org Link to the patchset: https://codereview.chromium.org/1312123003/#ps80001 (title: "Move test file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1312123003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1312123003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/64c1206dcc7df116ab14fabe86580c354f67fb26 Cr-Commit-Position: refs/heads/master@{#352838} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
