Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
844763008:
Refactor and cleanup scheduler_unittest.cc (Closed)
DescriptionRefactor and cleanup scheduler_unittest.cc
The following lines are repeated in almost every test cc/scheduler/scheduler_unittest.cc file
FakeSchedulerClient client;
SchedulerSettings scheduler_settings;
scheduler_settings.use_external_begin_frame_source = true;
CREATE_SCHEDULER_AND_INIT_SURFACE(scheduler_settings);
created test fixture class and clean up to remove duplicate codes.
BUG=449028
Committed: https://crrev.com/6e48d842f2a93b65f6354d1e47b1964d078e2658
Cr-Commit-Position: refs/heads/master@{#313619}
Patch Set 1 #Patch Set 2 : changed assign derived class simply #
Total comments: 10
Patch Set 3 : move InitializeOutputSurfaceAndFirstCommit #Patch Set 4 : move member of FakeSchedulerClient to SchedulerTest #
Total comments: 4
Patch Set 5 : move some inline fake classes #
Total comments: 9
Patch Set 6 : move some member and restore macro by brian`s guide #Patch Set 7 : change some argument to scoped_ptr #
Total comments: 2
Patch Set 8 : arrange SetUpScheduler caller #Messages
Total messages: 21 (2 generated)
jincheol.jo@navercorp.com changed reviewers: + brianderson@chromium.org, mithro@mithis.com, simonhong@chromium.org
Thanks jincheol for cleanup this. I think using class fixture looks much better. LGTM after some nits are addressed. We also need owner(@brianderson)'s confirm. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:24: using testing::Test; Using using-directive is not preferred in chromium. (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces) https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:40: #define CREATE_SCHEDULER_AND_INIT_SURFACE(settings) \ This can be removed. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:47: class FakeSchedulerClient; This also can be removed. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:171: ExpectAction("", -1, 0, scheduler); Using EXPECT_NO_ACTION is more self-descriptive. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:179: ExpectAction("ScheduledActionBeginOutputSurfaceCreation", 0, 1, scheduler); In this case, EXPECT_SINGLE_ACTION() is more better. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:394: scheduler_ = client_->CreateScheduler(scheduler_settings_); How about moving CreateScheduler() from FakeSchedulerClient to SchedulerTest? We also can move other things that are un-related with client like FakeExternalBeginFrameSource or PowerMonitor in here.
https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:24: using testing::Test; On 2015/01/15 14:10:00, simonhong wrote: > Using using-directive is not preferred in chromium. > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces) Done. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:171: ExpectAction("", -1, 0, scheduler); On 2015/01/15 14:10:00, simonhong wrote: > Using EXPECT_NO_ACTION is more self-descriptive. Done. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:179: ExpectAction("ScheduledActionBeginOutputSurfaceCreation", 0, 1, scheduler); On 2015/01/15 14:10:00, simonhong wrote: > In this case, EXPECT_SINGLE_ACTION() is more better. Done. https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:394: scheduler_ = client_->CreateScheduler(scheduler_settings_); On 2015/01/15 14:10:00, simonhong wrote: > How about moving CreateScheduler() from FakeSchedulerClient to SchedulerTest? > We also can move other things that are un-related with client like > FakeExternalBeginFrameSource or PowerMonitor in here. Acknowledged.
On 2015/01/16 06:39:50, Jincheol Jo wrote: > https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... > File cc/scheduler/scheduler_unittest.cc (right): > > https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:24: using testing::Test; > On 2015/01/15 14:10:00, simonhong wrote: > > Using using-directive is not preferred in chromium. > > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces) > > Done. > > https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:171: ExpectAction("", -1, 0, scheduler); > On 2015/01/15 14:10:00, simonhong wrote: > > Using EXPECT_NO_ACTION is more self-descriptive. > > Done. > > https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:179: > ExpectAction("ScheduledActionBeginOutputSurfaceCreation", 0, 1, scheduler); > On 2015/01/15 14:10:00, simonhong wrote: > > In this case, EXPECT_SINGLE_ACTION() is more better. > > Done. > > https://codereview.chromium.org/844763008/diff/20001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:394: scheduler_ = > client_->CreateScheduler(scheduler_settings_); > On 2015/01/15 14:10:00, simonhong wrote: > > How about moving CreateScheduler() from FakeSchedulerClient to SchedulerTest? > > We also can move other things that are un-related with client like > > FakeExternalBeginFrameSource or PowerMonitor in here. > > Acknowledged. I attached new codes by simonhong`s guide. Please take a look. :-)
Thanks @jincheol.jo! Let's wait owner's review :) https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:67: void SetScheduler(TestScheduler* scheduler) { scheduler_ = scheduler; } set_scheduler(...) https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:222: class FakePowerMonitorSource : public base::PowerMonitorSource { nit: How about moving these two class(FakePowerMonitorSource and FakeExternalBeginFrameSource) definition outside of SchedulerTest like FakeSchedulerClient? I think SchedulerTest don't need to have these as an inline.
I added some modification by simonhong`s guide https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:67: void SetScheduler(TestScheduler* scheduler) { scheduler_ = scheduler; } On 2015/01/21 22:04:09, simonhong wrote: > set_scheduler(...) Done. https://codereview.chromium.org/844763008/diff/60001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:222: class FakePowerMonitorSource : public base::PowerMonitorSource { On 2015/01/21 22:04:09, simonhong wrote: > nit: How about moving these two class(FakePowerMonitorSource and > FakeExternalBeginFrameSource) definition outside of SchedulerTest like > FakeSchedulerClient? > I think SchedulerTest don't need to have these as an inline. Done.
https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (left): https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:99: now_src_(TestNowSource::Create()), Should now_src_ and task_runner_ also move to the SchedulerTest class like the power monitor and external frame source did? (automatic_swap_ack and begin_frame_is_sent_to_children are client concepts and should be kep in FakeSchedulerClient.) https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:25: client->ExpectAction(action, action_index, expected_num_actions, \ Please keep the implementation in the macro. I know it's ugly, but it allows the error messages to include the line number where the error occurred. https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:189: void PushAction(const char* description) { Does PushAction need to be public now? https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:287: void SetUpScheduler(FakeSchedulerClient* client, bool initSurface) { Please make the client argument a scoped_ptr so it's clearer that we don't have memory leaks in the tests.
On 2015/01/27 01:08:25, brianderson wrote: > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > File cc/scheduler/scheduler_unittest.cc (left): > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:99: now_src_(TestNowSource::Create()), > Should now_src_ and task_runner_ also move to the SchedulerTest class like the > power monitor and external frame source did? > > (automatic_swap_ack and begin_frame_is_sent_to_children are client concepts and > should be kep in FakeSchedulerClient.) > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > File cc/scheduler/scheduler_unittest.cc (right): > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:25: client->ExpectAction(action, > action_index, expected_num_actions, \ > Please keep the implementation in the macro. I know it's ugly, but it allows the > error messages to include the line number where the error occurred. > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:189: void PushAction(const char* description) > { > Does PushAction need to be public now? > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:287: void SetUpScheduler(FakeSchedulerClient* > client, bool initSurface) { > Please make the client argument a scoped_ptr so it's clearer that we don't have > memory leaks in the tests. I got it.
https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (left): https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:99: now_src_(TestNowSource::Create()), On 2015/01/27 01:08:25, brianderson wrote: > Should now_src_ and task_runner_ also move to the SchedulerTest class like the > power monitor and external frame source did? > > (automatic_swap_ack and begin_frame_is_sent_to_children are client concepts and > should be kep in FakeSchedulerClient.) Done. https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:25: client->ExpectAction(action, action_index, expected_num_actions, \ On 2015/01/27 01:08:25, brianderson wrote: > Please keep the implementation in the macro. I know it's ugly, but it allows the > error messages to include the line number where the error occurred. Done. https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:189: void PushAction(const char* description) { On 2015/01/27 01:08:25, brianderson wrote: > Does PushAction need to be public now? This is placed in FakeSchedulerClient Class but this is used by FakeExternalBeginFrameSource::OnNeedsBeginFramesChange. So it need to be public. https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:636: SetUpScheduler(client, true); when I make SetUpScheduler 1st arugment a scoped_ptr, I`m facing a compile error as follows. no known conversion from 'scoped_ptr<cc::(anonymous namespace)::SchedulerClientThatsetNeedsDrawInsideDraw>' to 'scoped_ptr<cc::(anonymous namespace)::FakeSchedulerClient> FakeSchedulerClient`s child class have a problem.
https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... cc/scheduler/scheduler_unittest.cc:636: SetUpScheduler(client, true); On 2015/01/28 00:53:47, Jincheol Jo wrote: > when I make SetUpScheduler 1st arugment a scoped_ptr, I`m facing a compile error > as follows. > > no known conversion from 'scoped_ptr<cc::(anonymous > namespace)::SchedulerClientThatsetNeedsDrawInsideDraw>' to > 'scoped_ptr<cc::(anonymous namespace)::FakeSchedulerClient> > > FakeSchedulerClient`s child class have a problem. Does casting the argument inline work? If not, you should be able to assign the client to a scoped_ptr<FakeSchedulerClient> first and pass that in as the argument instead. See: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped...
On 2015/01/28 01:09:14, brianderson wrote: > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > File cc/scheduler/scheduler_unittest.cc (right): > > https://codereview.chromium.org/844763008/diff/80001/cc/scheduler/scheduler_u... > cc/scheduler/scheduler_unittest.cc:636: SetUpScheduler(client, true); > On 2015/01/28 00:53:47, Jincheol Jo wrote: > > when I make SetUpScheduler 1st arugment a scoped_ptr, I`m facing a compile > error > > as follows. > > > > no known conversion from 'scoped_ptr<cc::(anonymous > > namespace)::SchedulerClientThatsetNeedsDrawInsideDraw>' to > > 'scoped_ptr<cc::(anonymous namespace)::FakeSchedulerClient> > > > > FakeSchedulerClient`s child class have a problem. > > Does casting the argument inline work? > If not, you should be able to assign the client to a > scoped_ptr<FakeSchedulerClient> first and pass that in as the argument instead. > See: > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... Thank brian for detailed information. I modified some codes by your guide.
https://codereview.chromium.org/844763008/diff/120001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/120001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:661: scoped_ptr<SchedulerClientThatsetNeedsDrawInsideDraw> client = Hmm, this is a bit verbose. Does the following work? SchedulerClientThatsetNeedsDrawInsideDraw* client = new SchedulerClientThatsetNeedsDrawInsideDraw; ... SetUpScheduler(make_scoped_ptr(client).Pass(), true);
https://codereview.chromium.org/844763008/diff/120001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/844763008/diff/120001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:661: scoped_ptr<SchedulerClientThatsetNeedsDrawInsideDraw> client = On 2015/01/28 03:32:42, brianderson wrote: > Hmm, this is a bit verbose. Does the following work? > > SchedulerClientThatsetNeedsDrawInsideDraw* client = > new SchedulerClientThatsetNeedsDrawInsideDraw; > ... > SetUpScheduler(make_scoped_ptr(client).Pass(), true); Done.
Thanks for the cleanup! lgtm
Thanks for the cleanup! lgtm
The CQ bit was checked by jincheol.jo@navercorp.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/844763008/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/6e48d842f2a93b65f6354d1e47b1964d078e2658 Cr-Commit-Position: refs/heads/master@{#313619}
Message was sent while issue was closed.
On 2015/01/28 23:44:09, I haz the power (commit-bot) wrote: > Patchset 8 (id:??) landed as > https://crrev.com/6e48d842f2a93b65f6354d1e47b1964d078e2658 > Cr-Commit-Position: refs/heads/master@{#313619} Thank simon, brian for review. :-) |
