Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3136008:
Implement device_orientation::Provider. (Closed)
|
Created:
10 years, 4 months ago by hans Modified:
9 years, 5 months ago CC:
chromium-reviews, ben+cc_chromium.org, Satish, Leandro Gracia Gil, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionImplement device_orientation::Provider.
Provider provides its registered observers with device orientation data
by finding and polling a DataFetcher on a background thread.
BUG=44654
TEST=unit_tests --gtest_filter="DeviceOrientationProviderTest.*"
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=57036
Patch Set 1 #
Total comments: 38
Patch Set 2 : Patch #Patch Set 3 : Patch #
Total comments: 34
Patch Set 4 : Patch #
Total comments: 20
Patch Set 5 : Patch #
Total comments: 6
Patch Set 6 : Patch #Messages
Total messages: 16 (0 generated)
Sorry for the huge amount of code in the unit test; suggestions for simplifications are welcome.
couple quick thoughts.... http://codereview.chromium.org/3136008/diff/1/7 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/1/7#newcode70 chrome/browser/device_orientation/provider_impl.h:70: you can move all these class defs to the cc if you like (just declare them here) However, rather than subclass Task it's far more common to use NewRunableFunction to bind a function to a templated task. Any reason you can't use that here? (send the members in as parameters to the function) http://codereview.chromium.org/3136008/diff/1/8 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/1/8#newcode75 chrome/browser/device_orientation/provider_unittest.cc:75: class TestObserver : public Provider::Observer { a TestXxxx class is normally the name of a function that runs a test. http://codereview.chromium.org/3136008/diff/1/8#newcode91 chrome/browser/device_orientation/provider_unittest.cc:91: ASSERT_TRUE(updates_.Pop(&orientation)) << "Timeout"; rather than spinning on a condvar to pull items out, it's more common to call MessageLoop::Current()->Run() at the appropriate point within the individual test functions. i.e TEST_F(my test) { ... setup the mock data fetcher ... setup expectations on my mock observer, arrange for it to call MessageLoop::Current()->Quit() when expectation is met. ... create provider (thingy under test) passing mock data fetcher and observer Call MessageLoop::Current()->Run() (or RunPending()) to do async stuffs. profit
few drive-by comments, apologies if I misunderstood the threading model.. I think it may be simple to just copy the data and pass it along rather than access things across threads? both the provider factory vector and orientation seem pretty small and safe to copy in the tasks.. http://codereview.chromium.org/3136008/diff/1/6 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/1/6#newcode67 chrome/browser/device_orientation/provider_impl.cc:67: const double threshold = 0.1; kThreshold http://codereview.chromium.org/3136008/diff/1/6#newcode69 chrome/browser/device_orientation/provider_impl.cc:69: if (o1.can_provide_alpha_ != o2.can_provide_alpha_) hmm, if the new one doesn't provide the data, it'll override the older one.. is it better to have a potentially stale but valid value or just not have any data at all? http://codereview.chromium.org/3136008/diff/1/6#newcode103 chrome/browser/device_orientation/provider_impl.cc:103: const std::vector<DataFetcherFactory>& factories = provider_->factories_; hmm, factories_ is not const, and this is on a different thread.. :( I can see this specific vector is ok, but perhaps add some documentation on the assumptions? http://codereview.chromium.org/3136008/diff/1/6#newcode124 chrome/browser/device_orientation/provider_impl.cc:124: polling_loop->PostDelayedTask(FROM_HERE, poll_task, interval); this method and poll are running on a different thread than NotificationTask, and both are accessing provider_->last_orientation_. http://codereview.chromium.org/3136008/diff/1/6#newcode132 chrome/browser/device_orientation/provider_impl.cc:132: Task* notification_task = new NotificationTask(provider_, empty); Empty() doesn't seem to be adding much, perhaps just pass Orientation() here? http://codereview.chromium.org/3136008/diff/1/7 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/1/7#newcode46 chrome/browser/device_orientation/provider_impl.h:46: StartTask(ProviderImpl* provider); explicit http://codereview.chromium.org/3136008/diff/1/7#newcode47 chrome/browser/device_orientation/provider_impl.h:47: virtual void Run(); \n http://codereview.chromium.org/3136008/diff/1/7#newcode55 chrome/browser/device_orientation/provider_impl.h:55: NotificationTask(ProviderImpl* provider, Orientation orientation); const Orientation& http://codereview.chromium.org/3136008/diff/1/7#newcode56 chrome/browser/device_orientation/provider_impl.h:56: virtual void Run(); \n http://codereview.chromium.org/3136008/diff/1/7#newcode65 chrome/browser/device_orientation/provider_impl.h:65: PollTask(ProviderImpl* provider); explicit http://codereview.chromium.org/3136008/diff/1/7#newcode66 chrome/browser/device_orientation/provider_impl.h:66: virtual void Run(); \n http://codereview.chromium.org/3136008/diff/1/7#newcode69 chrome/browser/device_orientation/provider_impl.h:69: }; on all classes above: ProviderImpl* is passed between thread boundaries, but it's not clear who owns it / keeps the reference alive until the task finishes.. I think you'll either need a ScopedRunnableMethodFactory or make this class RefCounted and then keep the provider_ members as scoped_refptr. also, IIRC, these tasks are running on different threads / message loops, right? perhaps document where they're supposed to run?
Drive-by with test comments. Please do not commit without my "LGTM". http://codereview.chromium.org/3136008/diff/1/8 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/1/8#newcode22 chrome/browser/device_orientation/provider_unittest.cc:22: static int kTimeoutMsec = 3000; This is bad. If you need bullet-proof protection against timeouts, use a browser_test. http://codereview.chromium.org/3136008/diff/1/8#newcode116 chrome/browser/device_orientation/provider_unittest.cc:116: CHECK(!instance_); Could you replace those CHECKs with some gtest macros? If you insist on using them, please convert this to a browser test. http://codereview.chromium.org/3136008/diff/1/8#newcode147 chrome/browser/device_orientation/provider_unittest.cc:147: // Wait for it to be picked up. Please get rid of this hardcoded timeout. http://codereview.chromium.org/3136008/diff/1/8#newcode316 chrome/browser/device_orientation/provider_unittest.cc:316: MessageLoop* message_loop_; Please add a comment what loop it is.
Thank you very much for the input so far. Addressing your comments and uploading new patch. http://codereview.chromium.org/3136008/diff/1/6 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/1/6#newcode67 chrome/browser/device_orientation/provider_impl.cc:67: const double threshold = 0.1; On 2010/08/12 18:04:23, bulach wrote: > kThreshold Done. http://codereview.chromium.org/3136008/diff/1/6#newcode69 chrome/browser/device_orientation/provider_impl.cc:69: if (o1.can_provide_alpha_ != o2.can_provide_alpha_) On 2010/08/12 18:04:23, bulach wrote: > hmm, if the new one doesn't provide the data, it'll override the older one.. is > it better to have a potentially stale but valid value or just not have any data > at all? > I figured if the DataFetcher stops being able to provide data for some or all angles, that's a significant change, and the observers should be notified. I think this is correct. This way a page can either use the stale data, or take some action because orientation data is no longer available. http://codereview.chromium.org/3136008/diff/1/6#newcode103 chrome/browser/device_orientation/provider_impl.cc:103: const std::vector<DataFetcherFactory>& factories = provider_->factories_; On 2010/08/12 18:04:23, bulach wrote: > hmm, factories_ is not const, and this is on a different thread.. :( > I can see this specific vector is ok, but perhaps add some documentation on the > assumptions? Passing the vector to the constructor of StartTask to make it clear there is no dangerous sharing going on. http://codereview.chromium.org/3136008/diff/1/6#newcode124 chrome/browser/device_orientation/provider_impl.cc:124: polling_loop->PostDelayedTask(FROM_HERE, poll_task, interval); On 2010/08/12 18:04:23, bulach wrote: > this method and poll are running on a different thread than NotificationTask, > and both are accessing provider_->last_orientation_. NotificationTask does not access last_orientation_; it is only accessed by StartTask and PollTask, and those are both run on the polling thread. http://codereview.chromium.org/3136008/diff/1/6#newcode132 chrome/browser/device_orientation/provider_impl.cc:132: Task* notification_task = new NotificationTask(provider_, empty); On 2010/08/12 18:04:23, bulach wrote: > Empty() doesn't seem to be adding much, perhaps just pass Orientation() here? I agree that it doesn't add much in one sense, but I added Empty() to make it more explicit to the reader that this is the special "no orientation data can be provided" orientation, rather than just some default orientation. I'm happy to remove it if you don't think it adds any clarity. http://codereview.chromium.org/3136008/diff/1/7 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/1/7#newcode46 chrome/browser/device_orientation/provider_impl.h:46: StartTask(ProviderImpl* provider); On 2010/08/12 18:04:23, bulach wrote: > explicit Done. http://codereview.chromium.org/3136008/diff/1/7#newcode47 chrome/browser/device_orientation/provider_impl.h:47: virtual void Run(); On 2010/08/12 18:04:23, bulach wrote: > \n Done. http://codereview.chromium.org/3136008/diff/1/7#newcode55 chrome/browser/device_orientation/provider_impl.h:55: NotificationTask(ProviderImpl* provider, Orientation orientation); But I want it to have a copy, as the Orientation in the calling function may go away. I guess it could be a reference, and then copied into the class member, but that would make it less clear to the caller that no reference is kept. I don't know which is preferrable here? http://codereview.chromium.org/3136008/diff/1/7#newcode56 chrome/browser/device_orientation/provider_impl.h:56: virtual void Run(); On 2010/08/12 18:04:23, bulach wrote: > \n Done. http://codereview.chromium.org/3136008/diff/1/7#newcode65 chrome/browser/device_orientation/provider_impl.h:65: PollTask(ProviderImpl* provider); On 2010/08/12 18:04:23, bulach wrote: > explicit Done. http://codereview.chromium.org/3136008/diff/1/7#newcode66 chrome/browser/device_orientation/provider_impl.h:66: virtual void Run(); On 2010/08/12 18:04:23, bulach wrote: > \n Done. http://codereview.chromium.org/3136008/diff/1/7#newcode69 chrome/browser/device_orientation/provider_impl.h:69: }; On 2010/08/12 18:04:23, bulach wrote: > on all classes above: > ProviderImpl* is passed between thread boundaries, but it's not clear who owns > it / keeps the reference alive until the task finishes.. > I think you'll either need a ScopedRunnableMethodFactory or make this class > RefCounted and then keep the provider_ members as scoped_refptr. ProviderImpl is already RefCounted (by inheritance from Provider). The StartTask and PollTask classes are owned by the poll_thread_, which in turn is owned by ProviderImpl, so they can be sure the ProviderImpl is around when they execute. For NotificationTask, this is not true. Making NotificationTask::provider_ a scoped_refptr. > also, IIRC, these tasks are running on different threads / message loops, right? > perhaps document where they're supposed to run? The DCHECKs on the tasks' Run methods are intended to document (and check) this. Adding comments to the class declarations too. http://codereview.chromium.org/3136008/diff/1/7#newcode70 chrome/browser/device_orientation/provider_impl.h:70: On 2010/08/12 17:33:04, joth wrote: > you can move all these class defs to the cc if you like (just declare them here) Done. > However, rather than subclass Task it's far more common to use > NewRunableFunction to bind a function to a templated task. Any reason you can't > use that here? (send the members in as parameters to the function) The reason I didn't do that is the comment in task.h:36 that says those factory objects can only be used on non-refcounted objects (and ProviderImpl is refcounted). I haven't investigated the underlying reasons, though. http://codereview.chromium.org/3136008/diff/1/8 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/1/8#newcode22 chrome/browser/device_orientation/provider_unittest.cc:22: static int kTimeoutMsec = 3000; On 2010/08/12 20:53:57, Paweł Hajdan Jr. wrote: > This is bad. If you need bullet-proof protection against timeouts, use a > browser_test. Removing it as it was not necessary for the test. http://codereview.chromium.org/3136008/diff/1/8#newcode75 chrome/browser/device_orientation/provider_unittest.cc:75: class TestObserver : public Provider::Observer { On 2010/08/12 17:33:04, joth wrote: > a TestXxxx class is normally the name of a function that runs a test. > Calling it UpdateChecker. Suggestions for a better name is welcome. http://codereview.chromium.org/3136008/diff/1/8#newcode91 chrome/browser/device_orientation/provider_unittest.cc:91: ASSERT_TRUE(updates_.Pop(&orientation)) << "Timeout"; On 2010/08/12 17:33:04, joth wrote: > rather than spinning on a condvar to pull items out, it's more common to call > MessageLoop::Current()->Run() at the appropriate point within the individual > test functions. > i.e > TEST_F(my test) { > ... setup the mock data fetcher > ... setup expectations on my mock observer, arrange for it to call > MessageLoop::Current()->Quit() when expectation is met. > ... create provider (thingy under test) passing mock data fetcher and observer > Call MessageLoop::Current()->Run() (or RunPending()) to do async stuffs. > profit Hmm, this does indeed sound like it would get rid of a lot of the code used for synchronization here. But I'm not sure it would let me do all the testing I want, or make the actual test code easier to read. If I understand correctly, I would need to set up all expectations before-hand, set off the message loop, and wait for the results afterwards. I'm not sure this would allow me to do all the tests I want, for example with first having one observer, waiting for an orientation, then adding or removing observers, and wait for another one. Is there some good example in the code base that you could point me to? http://codereview.chromium.org/3136008/diff/1/8#newcode116 chrome/browser/device_orientation/provider_unittest.cc:116: CHECK(!instance_); On 2010/08/12 20:53:57, Paweł Hajdan Jr. wrote: > Could you replace those CHECKs with some gtest macros? If you insist on using > them, please convert this to a browser test. Done. http://codereview.chromium.org/3136008/diff/1/8#newcode147 chrome/browser/device_orientation/provider_unittest.cc:147: // Wait for it to be picked up. On 2010/08/12 20:53:57, Paweł Hajdan Jr. wrote: > Please get rid of this hardcoded timeout. Done. http://codereview.chromium.org/3136008/diff/1/8#newcode316 chrome/browser/device_orientation/provider_unittest.cc:316: MessageLoop* message_loop_; On 2010/08/12 20:53:57, Paweł Hajdan Jr. wrote: > Please add a comment what loop it is. Done.
After discussing with joth, updated ProviderImpl to use NewRunnableMethod for creating Task objects rather than defining custom sub-classes.
cool! I have a few minor comments / suggestions, and while I'm trying to get my head around :) the unit test infrastructure, would it simplify to use message loops? http://codereview.chromium.org/3136008/diff/13001/14002 File chrome/browser/device_orientation/orientation.h (right): http://codereview.chromium.org/3136008/diff/13001/14002#newcode39 chrome/browser/device_orientation/orientation.h:39: bool isEmpty() { s/is/Is/ http://codereview.chromium.org/3136008/diff/13001/14004 File chrome/browser/device_orientation/provider.h (right): http://codereview.chromium.org/3136008/diff/13001/14004#newcode48 chrome/browser/device_orientation/provider.h:48: friend class base::RefCounted<Provider>; s/Counted/CountedThreadSafe/ and keep the dtor protected? http://codereview.chromium.org/3136008/diff/13001/14005 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/13001/14005#newcode20 chrome/browser/device_orientation/provider_impl.cc:20: do_poll_method_factory_(this) { ALLOW_THIS_IN_INITIALIZER_LIST http://codereview.chromium.org/3136008/diff/13001/14005#newcode55 chrome/browser/device_orientation/provider_impl.cc:55: ScheduleDoStart(); Start() / ScheduleDoStart() and DoStart() seem a bit confusing.. perhaps: Start() / ScheduleInitializePollingThread() / DoInitializePollingThread() ? http://codereview.chromium.org/3136008/diff/13001/14005#newcode61 chrome/browser/device_orientation/provider_impl.cc:61: data_fetcher_.reset(); hehe, the .h says that data_fetcher_ is only to be used by the polling thread.. :) http://codereview.chromium.org/3136008/diff/13001/14005#newcode94 chrome/browser/device_orientation/provider_impl.cc:94: Task *task = NewRunnableMethod(this, &ProviderImpl::DoStart, factories_); s/Task *t/Task* t/ http://codereview.chromium.org/3136008/diff/13001/14005#newcode164 chrome/browser/device_orientation/provider_impl.cc:164: return true; not sure if it'd save lines / be more readable, but perhaps may be less error prone to: bool IsElementSignificantlyDifferent(bool can_provide_element1, bool can_provide_element2, double element1, double element2); so that there's only one call per element? http://codereview.chromium.org/3136008/diff/13001/14006 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/13001/14006#newcode48 chrome/browser/device_orientation/provider_impl.h:48: void DoStart(std::vector<DataFetcherFactory> factories); const & http://codereview.chromium.org/3136008/diff/13001/14006#newcode58 chrome/browser/device_orientation/provider_impl.h:58: void ScheduleDoNotify(Orientation orientation); const& both above http://codereview.chromium.org/3136008/diff/13001/14007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/13001/14007#newcode44 chrome/browser/device_orientation/provider_unittest.cc:44: condition_.Wait(); how does condition_ interacts with AutoLock? http://codereview.chromium.org/3136008/diff/13001/14007#newcode100 chrome/browser/device_orientation/provider_unittest.cc:100: OrientationPusher() : orientation_(NULL), put this one indented on the next line, and unindent all the following. http://codereview.chromium.org/3136008/diff/13001/14007#newcode173 chrome/browser/device_orientation/provider_unittest.cc:173: }; \n http://codereview.chromium.org/3136008/diff/13001/14007#newcode457 chrome/browser/device_orientation/provider_unittest.cc:457: for (int i = 0; i < 2; i++) { ++i http://codereview.chromium.org/3136008/diff/13001/14007#newcode469 chrome/browser/device_orientation/provider_unittest.cc:469: } // namespace device_orientation I'll try to get my head around here: the tests are GREAT but the infrastructure looks complex! it seems it's injecting way too many entry points to the point it's almost a white-box test. have you considered just using message loops and blocking while expecting the callbacks? would it simplify / make it slightly more like a black box? something along the lines: MockListener { void OnOrientationUpdate(...) { ... ChromeThread::PostTask(ChromeThread::UI, FROM_HERE, new MessageLoop::QuitTask()); } } } .... SetUpTest(); // Blocks until our mock is called. ui_test_utils::RunMessageLoop(); // Ensure the mock was called with the expected values. EXPECT_TRUE(mock_listener_.has_changed); ....
Looks better now. A general comment: can we extract some of those waiting utilities to chrome/test? Like chrome/test/task_util? I'm afraid we're duplicating those simple things across our tests, so let's just start making them more re-usable. http://codereview.chromium.org/3136008/diff/13001/14007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/13001/14007#newcode154 chrome/browser/device_orientation/provider_unittest.cc:154: while (!pusher_->orientation_) pusher_->poll_condition_.Wait(); nit: Please don't cram two things into one line. http://codereview.chromium.org/3136008/diff/13001/14007#newcode202 chrome/browser/device_orientation/provider_unittest.cc:202: void ExecuteOnMessageLoop(MessageLoop* message_loop) { nit: I'd prefer ExecuteAndWait (to indicate that we are waiting). http://codereview.chromium.org/3136008/diff/13001/14007#newcode269 chrome/browser/device_orientation/provider_unittest.cc:269: void Init(ProviderImpl::DataFetcherFactory *factories) { nit: Star on the wrong side.
Refactored the test code to run the message loop manually, as suggested by joth and Marcus. This got rid of most of the synchronization code, so hopefully it is more straightforward now. Also addressed the other issues mentioned. http://codereview.chromium.org/3136008/diff/13001/14002 File chrome/browser/device_orientation/orientation.h (right): http://codereview.chromium.org/3136008/diff/13001/14002#newcode39 chrome/browser/device_orientation/orientation.h:39: bool isEmpty() { On 2010/08/16 17:01:35, bulach wrote: > s/is/Is/ Done. http://codereview.chromium.org/3136008/diff/13001/14004 File chrome/browser/device_orientation/provider.h (right): http://codereview.chromium.org/3136008/diff/13001/14004#newcode48 chrome/browser/device_orientation/provider.h:48: friend class base::RefCounted<Provider>; On 2010/08/16 17:01:35, bulach wrote: > s/Counted/CountedThreadSafe/ > and keep the dtor protected? Well spotted! Thanks. http://codereview.chromium.org/3136008/diff/13001/14005 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/13001/14005#newcode20 chrome/browser/device_orientation/provider_impl.cc:20: do_poll_method_factory_(this) { On 2010/08/16 17:01:35, bulach wrote: > ALLOW_THIS_IN_INITIALIZER_LIST Done. http://codereview.chromium.org/3136008/diff/13001/14005#newcode55 chrome/browser/device_orientation/provider_impl.cc:55: ScheduleDoStart(); On 2010/08/16 17:01:35, bulach wrote: > Start() / ScheduleDoStart() and DoStart() seem a bit confusing.. perhaps: > Start() / ScheduleInitializePollingThread() / DoInitializePollingThread() ? Done. http://codereview.chromium.org/3136008/diff/13001/14005#newcode61 chrome/browser/device_orientation/provider_impl.cc:61: data_fetcher_.reset(); On 2010/08/16 17:01:35, bulach wrote: > hehe, the .h says that data_fetcher_ is only to be used by the polling thread.. > :) Updated the comment :) http://codereview.chromium.org/3136008/diff/13001/14005#newcode94 chrome/browser/device_orientation/provider_impl.cc:94: Task *task = NewRunnableMethod(this, &ProviderImpl::DoStart, factories_); On 2010/08/16 17:01:35, bulach wrote: > s/Task *t/Task* t/ Oops. Done. http://codereview.chromium.org/3136008/diff/13001/14005#newcode164 chrome/browser/device_orientation/provider_impl.cc:164: return true; On 2010/08/16 17:01:35, bulach wrote: > not sure if it'd save lines / be more readable, but perhaps may be less error > prone to: > bool IsElementSignificantlyDifferent(bool can_provide_element1, bool > can_provide_element2, double element1, double element2); > > so that there's only one call per element? Trying that out. http://codereview.chromium.org/3136008/diff/13001/14006 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/13001/14006#newcode48 chrome/browser/device_orientation/provider_impl.h:48: void DoStart(std::vector<DataFetcherFactory> factories); On 2010/08/16 17:01:35, bulach wrote: > const & But the whole point of passing it to DoStart is to make it take a copy of the vector so it is not shared between threads... http://codereview.chromium.org/3136008/diff/13001/14006#newcode58 chrome/browser/device_orientation/provider_impl.h:58: void ScheduleDoNotify(Orientation orientation); On 2010/08/16 17:01:35, bulach wrote: > const& both above Same as above, I can't just pass in a reference, because then the Orientation object would be shared between threads. http://codereview.chromium.org/3136008/diff/13001/14007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/13001/14007#newcode44 chrome/browser/device_orientation/provider_unittest.cc:44: condition_.Wait(); On 2010/08/16 17:01:35, bulach wrote: > how does condition_ interacts with AutoLock? Obsolete. BlockingQueue doesn't exist anymore. http://codereview.chromium.org/3136008/diff/13001/14007#newcode100 chrome/browser/device_orientation/provider_unittest.cc:100: OrientationPusher() : orientation_(NULL), On 2010/08/16 17:01:35, bulach wrote: > put this one indented on the next line, and unindent all the following. Done. http://codereview.chromium.org/3136008/diff/13001/14007#newcode154 chrome/browser/device_orientation/provider_unittest.cc:154: while (!pusher_->orientation_) pusher_->poll_condition_.Wait(); On 2010/08/16 18:31:38, Paweł Hajdan Jr. wrote: > nit: Please don't cram two things into one line. Done. http://codereview.chromium.org/3136008/diff/13001/14007#newcode173 chrome/browser/device_orientation/provider_unittest.cc:173: }; On 2010/08/16 17:01:35, bulach wrote: > \n Done. http://codereview.chromium.org/3136008/diff/13001/14007#newcode202 chrome/browser/device_orientation/provider_unittest.cc:202: void ExecuteOnMessageLoop(MessageLoop* message_loop) { On 2010/08/16 18:31:38, Paweł Hajdan Jr. wrote: > nit: I'd prefer ExecuteAndWait (to indicate that we are waiting). Obsolete. SynchronousTask doesn't exist anymore. http://codereview.chromium.org/3136008/diff/13001/14007#newcode269 chrome/browser/device_orientation/provider_unittest.cc:269: void Init(ProviderImpl::DataFetcherFactory *factories) { On 2010/08/16 18:31:38, Paweł Hajdan Jr. wrote: > nit: Star on the wrong side. Oops. http://codereview.chromium.org/3136008/diff/13001/14007#newcode457 chrome/browser/device_orientation/provider_unittest.cc:457: for (int i = 0; i < 2; i++) { On 2010/08/16 17:01:35, bulach wrote: > ++i Unrolling the loop instead :) http://codereview.chromium.org/3136008/diff/13001/14007#newcode469 chrome/browser/device_orientation/provider_unittest.cc:469: } // namespace device_orientation On 2010/08/16 17:01:35, bulach wrote: > I'll try to get my head around here: the tests are GREAT but the infrastructure > looks complex! it seems it's injecting way too many entry points to the point > it's almost a white-box test. > > have you considered just using message loops and blocking while expecting the > callbacks? would it simplify / make it slightly more like a black box? Refactored to run the message loop manually. This let me get rid of almost all the classes dealing with synchronization, so hopefully the code should be easier to follow now.
nice, looks much easier to read now, thanks!! few more comments, I think we can simplify a little bit more and then we're done! :) http://codereview.chromium.org/3136008/diff/21001/22005 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/21001/22005#newcode153 chrome/browser/device_orientation/provider_impl.cc:153: double element2) { unindent previous 3 http://codereview.chromium.org/3136008/diff/21001/22005#newcode159 chrome/browser/device_orientation/provider_impl.cc:159: && std::fabs(element1 - element2) >= kThreshold) I think keeping the operator on the previous line is more common.. (same for the || below..) http://codereview.chromium.org/3136008/diff/21001/22005#newcode177 chrome/browser/device_orientation/provider_impl.cc:177: || IsElementSignificantlyDifferent(o1.can_provide_gamma_, yeah, definitely more readable! http://codereview.chromium.org/3136008/diff/21001/22006 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/21001/22006#newcode64 chrome/browser/device_orientation/provider_impl.h:64: int SamplingIntervalMs(); const? http://codereview.chromium.org/3136008/diff/21001/22007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/21001/22007#newcode54 chrome/browser/device_orientation/provider_unittest.cc:54: } \n http://codereview.chromium.org/3136008/diff/21001/22007#newcode79 chrome/browser/device_orientation/provider_unittest.cc:79: // Push orientation to Provider. Blocks until the orientation is picked up. it doesn't block anymore, right? if I understood correctly, this class is now only effectively used by "SignificantlyDifferent" test.. I think it could be further simplified so that: test context: SetInsignificatOrientation(); RunMessageLoop(); polling thread context: then, only in this case, "GetOrientation()" (called on the polling thread) would post a task to unblock the message loop... test context again: post a quit task to self. run the message loop again (silly, but add a comment saying something like: // We'll assert that no new orientation notification was in the message queue.) check that there was no orientation update. hopefully we'll be able to get rid of the cond var and perhaps the whole "pusher" thing, and just use the fetcher directly. http://codereview.chromium.org/3136008/diff/21001/22007#newcode88 chrome/browser/device_orientation/provider_unittest.cc:88: class HelperDataFetcher : public DataFetcher { MockDataFetcher would be nicer http://codereview.chromium.org/3136008/diff/21001/22007#newcode292 chrome/browser/device_orientation/provider_unittest.cc:292: AddObserver(checker_b.get()); // This breaks stuff! nice comment.. ;) http://codereview.chromium.org/3136008/diff/21001/22007#newcode340 chrome/browser/device_orientation/provider_unittest.cc:340: true, 6 + kSignificantDifference); could do with s/4|5|6/kAlpha|kBeta|kGamma/
nice, looks much easier to read now, thanks!! few more comments, I think we can simplify a little bit more and then we're done! :) http://codereview.chromium.org/3136008/diff/21001/22005 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/21001/22005#newcode153 chrome/browser/device_orientation/provider_impl.cc:153: double element2) { unindent previous 3 http://codereview.chromium.org/3136008/diff/21001/22005#newcode159 chrome/browser/device_orientation/provider_impl.cc:159: && std::fabs(element1 - element2) >= kThreshold) I think keeping the operator on the previous line is more common.. (same for the || below..) http://codereview.chromium.org/3136008/diff/21001/22005#newcode177 chrome/browser/device_orientation/provider_impl.cc:177: || IsElementSignificantlyDifferent(o1.can_provide_gamma_, yeah, definitely more readable! http://codereview.chromium.org/3136008/diff/21001/22006 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/21001/22006#newcode64 chrome/browser/device_orientation/provider_impl.h:64: int SamplingIntervalMs(); const? http://codereview.chromium.org/3136008/diff/21001/22007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/21001/22007#newcode54 chrome/browser/device_orientation/provider_unittest.cc:54: } \n http://codereview.chromium.org/3136008/diff/21001/22007#newcode79 chrome/browser/device_orientation/provider_unittest.cc:79: // Push orientation to Provider. Blocks until the orientation is picked up. it doesn't block anymore, right? if I understood correctly, this class is now only effectively used by "SignificantlyDifferent" test.. I think it could be further simplified so that: test context: SetInsignificatOrientation(); RunMessageLoop(); polling thread context: then, only in this case, "GetOrientation()" (called on the polling thread) would post a task to unblock the message loop... test context again: post a quit task to self. run the message loop again (silly, but add a comment saying something like: // We'll assert that no new orientation notification was in the message queue.) check that there was no orientation update. hopefully we'll be able to get rid of the cond var and perhaps the whole "pusher" thing, and just use the fetcher directly. http://codereview.chromium.org/3136008/diff/21001/22007#newcode88 chrome/browser/device_orientation/provider_unittest.cc:88: class HelperDataFetcher : public DataFetcher { MockDataFetcher would be nicer http://codereview.chromium.org/3136008/diff/21001/22007#newcode292 chrome/browser/device_orientation/provider_unittest.cc:292: AddObserver(checker_b.get()); // This breaks stuff! nice comment.. ;) http://codereview.chromium.org/3136008/diff/21001/22007#newcode340 chrome/browser/device_orientation/provider_unittest.cc:340: true, 6 + kSignificantDifference); could do with s/4|5|6/kAlpha|kBeta|kGamma/
Just one minor comment. http://codereview.chromium.org/3136008/diff/21001/22007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/21001/22007#newcode29 chrome/browser/device_orientation/provider_unittest.cc:29: EXPECT_FALSE(expectations_queue_.empty()); This should actually be ASSERT to avoid a crash later. In that case we might want to quite the loop immediately or something.
Uploaded new patch. Trying to accommodate bulach's suggestions for reducing the need to synchronize with the polling thread. http://codereview.chromium.org/3136008/diff/21001/22005 File chrome/browser/device_orientation/provider_impl.cc (right): http://codereview.chromium.org/3136008/diff/21001/22005#newcode153 chrome/browser/device_orientation/provider_impl.cc:153: double element2) { On 2010/08/17 18:04:49, bulach wrote: > unindent previous 3 Done. http://codereview.chromium.org/3136008/diff/21001/22005#newcode159 chrome/browser/device_orientation/provider_impl.cc:159: && std::fabs(element1 - element2) >= kThreshold) On 2010/08/17 18:04:49, bulach wrote: > I think keeping the operator on the previous line is more common.. (same for the > || below..) Done. http://codereview.chromium.org/3136008/diff/21001/22005#newcode177 chrome/browser/device_orientation/provider_impl.cc:177: || IsElementSignificantlyDifferent(o1.can_provide_gamma_, On 2010/08/17 18:04:49, bulach wrote: > yeah, definitely more readable! > Good :) http://codereview.chromium.org/3136008/diff/21001/22006 File chrome/browser/device_orientation/provider_impl.h (right): http://codereview.chromium.org/3136008/diff/21001/22006#newcode64 chrome/browser/device_orientation/provider_impl.h:64: int SamplingIntervalMs(); On 2010/08/17 18:04:49, bulach wrote: > const? Done. Also making DataFetcher::MinSamplingInterval const. http://codereview.chromium.org/3136008/diff/21001/22007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/21001/22007#newcode29 chrome/browser/device_orientation/provider_unittest.cc:29: EXPECT_FALSE(expectations_queue_.empty()); On 2010/08/17 18:27:03, Paweł Hajdan Jr. wrote: > This should actually be ASSERT to avoid a crash later. In that case we might > want to quite the loop immediately or something. Done. Making it an ASSERT. http://codereview.chromium.org/3136008/diff/21001/22007#newcode54 chrome/browser/device_orientation/provider_unittest.cc:54: } On 2010/08/17 18:04:49, bulach wrote: > \n Done. http://codereview.chromium.org/3136008/diff/21001/22007#newcode79 chrome/browser/device_orientation/provider_unittest.cc:79: // Push orientation to Provider. Blocks until the orientation is picked up. On 2010/08/17 18:04:49, bulach wrote: > it doesn't block anymore, right? Right. Sorry for the stale comment. > if I understood correctly, this class is now only effectively used by > "SignificantlyDifferent" test.. I think it could be further simplified so that: > test context: > SetInsignificatOrientation(); > RunMessageLoop(); > > polling thread context: > then, only in this case, "GetOrientation()" (called on the polling thread) would > post a task to unblock the message loop... > > test context again: > post a quit task to self. > run the message loop again (silly, but add a comment saying something like: > // We'll assert that no new orientation notification was in the message queue.) > check that there was no orientation update. > > hopefully we'll be able to get rid of the cond var and perhaps the whole > "pusher" thing, and just use the fetcher directly. Rewrote the tests to that it doesn't "push" orientation data to the provider, but rather just sets a current orientation value from the test. Hopefully this makes things simpler. http://codereview.chromium.org/3136008/diff/21001/22007#newcode88 chrome/browser/device_orientation/provider_unittest.cc:88: class HelperDataFetcher : public DataFetcher { On 2010/08/17 18:04:49, bulach wrote: > MockDataFetcher would be nicer Done. http://codereview.chromium.org/3136008/diff/21001/22007#newcode292 chrome/browser/device_orientation/provider_unittest.cc:292: AddObserver(checker_b.get()); // This breaks stuff! On 2010/08/17 18:04:49, bulach wrote: > nice comment.. ;) That was not intentional. Nothing should break stuff in this test :) http://codereview.chromium.org/3136008/diff/21001/22007#newcode340 chrome/browser/device_orientation/provider_unittest.cc:340: true, 6 + kSignificantDifference); On 2010/08/17 18:04:49, bulach wrote: > could do with s/4|5|6/kAlpha|kBeta|kGamma/ Done.
LGTM very nice!! many thanks for taking care of my comments, really appreciate.. ;) just a few last nits, feel free to go ahead! http://codereview.chromium.org/3136008/diff/24002/28007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/24002/28007#newcode56 chrome/browser/device_orientation/provider_unittest.cc:56: int* expectations_count_ptr_; perhaps add a comment explaining that this is safe, i.e., it's setup by the fixture on ui thread, then it blocks, then OnOrientationUpdate changes it and when it's done it'll unblock the ui thread for checking.. may be less verbose.. :) http://codereview.chromium.org/3136008/diff/24002/28007#newcode61 chrome/browser/device_orientation/provider_unittest.cc:61: class OrientationPusher : public base::RefCounted<OrientationPusher> { perhaps MockOrientationFactory? http://codereview.chromium.org/3136008/diff/24002/28007#newcode357 chrome/browser/device_orientation/provider_unittest.cc:357: } // namespace \n
Cool. Will land this once it's spent some time on the try bots, and Paweł gives his LGTM. http://codereview.chromium.org/3136008/diff/24002/28007 File chrome/browser/device_orientation/provider_unittest.cc (right): http://codereview.chromium.org/3136008/diff/24002/28007#newcode56 chrome/browser/device_orientation/provider_unittest.cc:56: int* expectations_count_ptr_; On 2010/08/18 14:09:49, bulach wrote: > perhaps add a comment explaining that this is safe, i.e., it's setup by the > fixture on ui thread, then it blocks, then OnOrientationUpdate changes it and > when it's done it'll unblock the ui thread for checking.. may be less verbose.. > :) Done. http://codereview.chromium.org/3136008/diff/24002/28007#newcode61 chrome/browser/device_orientation/provider_unittest.cc:61: class OrientationPusher : public base::RefCounted<OrientationPusher> { On 2010/08/18 14:09:49, bulach wrote: > perhaps MockOrientationFactory? Done. http://codereview.chromium.org/3136008/diff/24002/28007#newcode357 chrome/browser/device_orientation/provider_unittest.cc:357: } // namespace On 2010/08/18 14:09:49, bulach wrote: > \n Done.
LGTM |
