TestingPlatformSupport: register Platform instance correctly

TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests, webkit_unit_tests

Review-Url: https://codereview.chromium.org/2588403002
Cr-Commit-Position: refs/heads/master@{#443219}
Committed: https://chromium.googlesource.com/chromium/src/+/f651acf25aaea54d6aed2488c8237214f1a2b38f

[Takashi Toyoshima, 2016-12-20 13:51:33]

toyoshim@chromium.org changed reviewers:
+ jbroman@chromium.org

[Takashi Toyoshima, 2016-12-20 13:51:34]

Can you take a look?
I need this fix for the following core/fetch immigration.
https://codereview.chromium.org/2584423002/

[jbroman, 2016-12-20 19:04:09]

It feels a bit hacky to call the register method repeatedly.

WDYT of making mainThread() virtual (but I assume there's a reason it isn't --
maybe performance?), or passing it through to the Platform constructor? That
way, we can avoid virtual calls in the constructor altogether.

[Takashi Toyoshima, 2016-12-21 05:46:11]

> WDYT of making mainThread() virtual (but I assume there's a reason it isn't --
> maybe performance?), or passing it through to the Platform constructor? That

Yeah, I think so. I heard that calling mainThread() is a little faster than
currentThread() probably because of this, and if we change to call mainThread()
instead of currentThread() at the place we can assume it's always the main
thread, it improve visible performance.

Also, passing it through to the constructor is also difficult because
TestingPlatformSupportWithMockScheduler create the main thread inside its
constructor that happens after the parent class construction.

Another idea is to have static create() method and to make the constructor
private and call Platform::setCurrentPlatformForTesting(this) after construction
inside create().

[Takashi Toyoshima, 2016-12-21 09:38:35]

> Another idea is to have static create() method and to make the constructor
> private and call Platform::setCurrentPlatformForTesting(this) after
construction
> inside create().

This makes creating sub-class very inconvenient and we have many sub-classes in
core and platform.
Another idea is to prepare a ScopedTestingPlatformSupport<T> class.
Now my change is working in progress, but this could make sure
setCurrentPlatformForTesting() is called in a right way, and user side changes
can be minimized. It isn't finished yet, but Patch Set 2 is the one I'm doing
this. I will continue this tomorrow if you agreed.

[jbroman, 2016-12-21 17:41:26]

On 2016/12/21 at 05:46:11, toyoshim wrote:
> > WDYT of making mainThread() virtual (but I assume there's a reason it isn't
--
> > maybe performance?), or passing it through to the Platform constructor? That
> 
> Yeah, I think so. I heard that calling mainThread() is a little faster than
currentThread() probably because of this, and if we change to call mainThread()
instead of currentThread() at the place we can assume it's always the main
thread, it improve visible performance.
> 
> Also, passing it through to the constructor is also difficult because
TestingPlatformSupportWithMockScheduler create the main thread inside its
constructor that happens after the parent class construction.

Urgh, the fact that it does depend on things that aren't supplied to the
TestingPlatformSupportWithMockScheduler constructor but are needed in multiple
initializer arguments makes it super awkward.
Something like this will work, but it's rather ugly.

// protected
Platform(WebThread* mainThread) : m_mainThread(mainThread) {}

// protected
PlatformTestingSupport(const Config& config, WebThread* mainThread) :
Platform(mainThread), /* other stuff */ {}

// public
PlatformTestingSupport(const Config& config) : PlatformTestingSupport(config) {
m_mainThread = PlatformTestingSupport::currentThread(); }

// private
// (the massive parameter list is used because these objects must exist before
the constructor is called)
using SchedulerParts = std::tuple<std::unique_ptr<base::SimpleTestTickClock>,
scoped_refptr<cc::OrderedSimpleTaskRunner>,
std::unique_ptr<scheduler::RendererSchedulerImpl>, std::unique_ptr<WebThread>>;
TestingPlatformSupportWithMockScheduler(const Config& config, SchedulerParts
schedulerParts)
    : PlatformTestingSupport(config, std::get<3>(schedulerParts).get()) {
  // or the members could be initialized using
std::move(base::get<n>(schedulerParts)) instead
  std::tie(m_clock, m_mockTaskRunner, m_scheduler, m_thread) =
std::move(schedulerParts);
}

// public
static TestingPlatformSupportWithMockScheduler::SchedulerParts
makeSchedulerParts() {
  std::unique_ptr<base::SimpleTestTickClock> clock(new
base::SimpleTestTickClock());
  scoped_refptr<cc::OrderedSimpleTaskRunner> mockTaskRunner(new
cc::OrderedSimpleTaskRunner(clock.get(), true));
  std::unique_ptr<scheduler::RendererSchedulerImpl> scheduler(new
scheduler::RendererSchedulerImpl(
          scheduler::SchedulerTqmDelegateForTest::Create(
              m_mockTaskRunner,
              base::WrapUnique(new scheduler::TestTimeSource(m_clock.get())))));
  return std::make_tuple(std::move(clock), std::move(mockTaskRunner),
std::move(scheduler), scheduler->CreateMainThread());
}
TestingPlatformSupportWithMockScheduler(const Config& config)
    : TestingPlatformSupportWithMockScheduler(config, makeSchedulerParts()) {}


> Another idea is to have static create() method and to make the constructor
private and call Platform::setCurrentPlatformForTesting(this) after construction
inside create().

The awkward thing here is that they're presently used in a stack-allocated way.

I think the root of the problem here is that PlatformTestingSupport is trying to
be both a platform and a manager of platforms. WDYT about splitting it into a
PlatformTestingSupport class (that implements blink::Platform) and a
ScopedOverridePlatform class  (that calls
Platform::setCurrentPlatformForTesting).

It could be a simple class, which is probably clearest (though it turns one line
into two at each call site), or possibly a template (that constructs a given
platform class T and sets it as current) -- though I think I prefer the former.

[jbroman, 2016-12-21 17:42:38]

Ah, I see you had a similar idea between when I started writing this response
and when I finished it. :)

[Takashi Toyoshima, 2016-12-22 06:22:38]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 06:22:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Takashi Toyoshima, 2016-12-22 06:29:07]

thanks, I finished yesterday's new plan.
Let me see if trybots being happy with this change.

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp (left):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontCacheTest.cpp:15: class
EmptyPlatform : public TestingPlatformSupport {
this wasn't needed at all :)

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/DEPS (right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/DEPS:7: "+base/logging.h",
for DCHECK

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
(right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp:150:
DCHECK_NE(m_oldPlatform, Platform::current());
I confirm that this check actually works to catch wrong usages, using
TestingPlatformSupport or its sub-classes without ScopedTestingPlatformSupport.

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/Platform.h:142: // calls virtual methods
internally, you should not call with self instance
probably worth noting

[Takashi Toyoshima, 2016-12-22 07:44:06]

Description was changed from

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a constructor.

This patch registers the Platform instance again after virtual methods
get ready.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests
==========

to

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSuppoer class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests
==========

[Takashi Toyoshima, 2016-12-22 07:45:14]

Description is also updated. Since trybots look happy, PTAL.

[commit-bot: I haz the power, 2016-12-22 08:21:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 08:21:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbroman, 2016-12-22 15:27:40]

Description was changed from

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSuppoer class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests
==========

to

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests
==========

[jbroman, 2016-12-22 15:27:54]

lgtm with two comments

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/DEPS (right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/DEPS:7: "+base/logging.h",
On 2016/12/22 at 06:29:06, toyoshim wrote:
> for DCHECK

DCHECK is available via wtf/Assertions.h; that's how other places get it
currently.

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:196:
explicit ScopedTestingPlatformSupport(T* testingPlatformSupport)
Taking ownership by raw pointer, here and below, is something we're trying to
get rid of. Could this take std::unique_ptr instead (and have the callers use
MakeUnique)?

[Takashi Toyoshima, 2017-01-11 08:35:19]

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/DEPS (right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/DEPS:7: "+base/logging.h",
On 2016/12/22 15:27:54, jbroman wrote:
> On 2016/12/22 at 06:29:06, toyoshim wrote:
> > for DCHECK
> 
> DCHECK is available via wtf/Assertions.h; that's how other places get it
> currently.

Done.

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:34: #include
"base/logging.h"
removed

https://codereview.chromium.org/2588403002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:196:
explicit ScopedTestingPlatformSupport(T* testingPlatformSupport)
On 2016/12/22 15:27:54, jbroman wrote:
> Taking ownership by raw pointer, here and below, is something we're trying to
> get rid of. Could this take std::unique_ptr instead (and have the callers use
> MakeUnique)?

Done.

[Takashi Toyoshima, 2017-01-11 08:43:54]

toyoshim@chromium.org changed reviewers:
+ haraken@chromium.org

[Takashi Toyoshima, 2017-01-11 08:43:55]

+haraken

Could you put a stamp for non-functional changes in Platform.h and modules? Main
modification for the TestingPlatformSupport was already reviewed by jbroman.

[Takashi Toyoshima, 2017-01-11 08:44:06]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 08:44:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-11 08:58:08]

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:193: class
ScopedTestingPlatformSupport {

Add STACK_ALLOCATED.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:193: class
ScopedTestingPlatformSupport {

Add a comment and explain how this class should be used.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:195:
ScopedTestingPlatformSupport() : m_originalPlatform(Platform::current()) {}

Do we need this constructor?

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:203: if
(testingPlatformSupport.get())

If testingPlatformSupport is null, don't we want to set the current platform to
m_originalPlatform?

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:210:
Platform::setCurrentPlatformForTesting(m_originalPlatform);

If you apply the change I proposed above, you can just call reset(nullptr) in
~ScopedTestingPlatformSupport().

[commit-bot: I haz the power, 2017-01-11 10:02:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 10:02:36]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Takashi Toyoshima, 2017-01-11 12:46:54]

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:193: class
ScopedTestingPlatformSupport {
On 2017/01/11 08:58:08, haraken wrote:
> 
> Add a comment and explain how this class should be used.

comments including usage were added.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:193: class
ScopedTestingPlatformSupport {
Actually, this can not be a STACK_ALLOCATED class because testing classes want
to have this instance as a member so that each test item does not need to
construct it respectively.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:195:
ScopedTestingPlatformSupport() : m_originalPlatform(Platform::current()) {}
Yes.

Here is one example that requires this constructor, having it as a testing class
member.
https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:195:
ScopedTestingPlatformSupport() : m_originalPlatform(Platform::current()) {}
I noticed there was a potential problem here.
Platform::current() may return nullptr if this instance is stack-allocated.

I will obtain it inside reset() on the first meaningful call.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:203: if
(testingPlatformSupport.get())
Good point. To restore a valid platform instance always safely, I modified this
method a little.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:210:
Platform::setCurrentPlatformForTesting(m_originalPlatform);
On 2017/01/11 08:58:08, haraken wrote:
> 
> If you apply the change I proposed above, you can just call reset(nullptr) in
> ~ScopedTestingPlatformSupport().

Done.

[Takashi Toyoshima, 2017-01-11 12:47:01]

The CQ bit was checked by toyoshim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 12:47:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-01-11 13:03:38]

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:61:
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>

Hmm. It looks a bit nasty to allocate a "Scoped" object on a on-heap object.
It's not scoped and actually you're manually resetting the object at every
SetUp().

Would it be nasty to allocate ScopedTestingPlatformSupport in each test, just
like OriginAccessEntryTest.cpp is doing?

https://codereview.chromium.org/2588403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:238: void
reset(std::unique_ptr<T> testingPlatformSupport) {

This method is getting a bit too complicated.

Maybe would it make things simpler if we introduce Vector<Platform*> and provide
a push/pop-style API? Every time a Platform* object is pushed or popped, you can
call Platform::setCurrentPlatformForTesting.

[commit-bot: I haz the power, 2017-01-11 13:47:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 13:47:55]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Takashi Toyoshima, 2017-01-12 08:56:14]

Can you review a new idea?
There seems to be room for discussion if the template class should be
STACK_ALLOCATED or not because in some tests having it as a member looks very
useful.

Thanks.

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:61:
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
I do not feel it's nasty to use ScopedFoo in members because it has been a
chrome style outside blink though majority was already replaced with
std::unique_ptr.

But, as you suggested, restricting this class as STACK_ALLOCATED would help
reducing complexity, and it looks reasonable to use this class via AutoLock like
interface.

I will change the template class a little so that we can place it in each test
more easily.

https://codereview.chromium.org/2588403002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:238: void
reset(std::unique_ptr<T> testingPlatformSupport) {
Original intention of this complicated logic was to detect wrong usages that
users own TestingPlatformSupport instance directly without this class. But,
probably it's too complex only for a small benefit.

Also, changing this class to STACK_ALLOCATED would make it possible to remove
almost all logic here without introducing a Vector.

Please review the new patch set.

https://codereview.chromium.org/2588403002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:286:
ScopedTestingPlatformSupport<TestingPlatformSupport> m_testingPlatformSupport;
Unfortunately, we can not use ScopedTestingPlatformSupport here any more. So we
manually manage TestingPlatformSupport only in this class exceptionally.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (left):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:46: void SetUp()
override {
This change is unfortunate, but I have no better idea.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/TimerTest.cpp:30: void setupTimes(
Changes in this class are also unfortunate, but...

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h:85: class
ScrollbarTestSuite : public testing::Test {
This class is used in some tests in platform/scroll, but after this change, this
class get to be a little useless.

[haraken, 2017-01-12 11:21:01]

LGTM

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:60: m_scriptRunner =
ScriptRunner::create(m_document.get());

It might be slightly cleaner to move this logic into
ScriptRunnerTest::initialize().

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/TimerTest.cpp:30: void setupTimes(
On 2017/01/12 08:56:13, toyoshim wrote:
> Changes in this class are also unfortunate, but...

setupTimes => initialize ?

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h:85: class
ScrollbarTestSuite : public testing::Test {
On 2017/01/12 08:56:13, toyoshim wrote:
> This class is used in some tests in platform/scroll, but after this change,
this
> class get to be a little useless.

Yeah, maybe we want to remove it in a follow-up CL.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:123: //
correctly.  See also comments on ScopedTestingPlatformSupport.

Slightly simpler:

// A base class to override Platform methods for testing. You can override the
behavior by subclassing TestingPlatformSupport or using
ScopedTestingPlatformSupport (see below).

(I'm wondering why compositorSupport is handled specially.)

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:210: // on
its destruction.  Instances could be nested.

Slightly simpler:

// ScopedTestingPlatformSupport<MyTestingPlatformSupport> can be used to
override Platform::current() with MyTestingPlatformSupport, like this:

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:231: //   //
Here the origianl MyTestingPlatformSupport should be restored.

original

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:234: class
ScopedTestingPlatformSupport final {

This looks much simpler!

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:144: // inside a Platform
subclass's constructor.

// you should not pass in a Platform object that is not fully instantiated

[Takashi Toyoshima, 2017-01-12 12:13:02]

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/ScriptRunnerTest.cpp:60: m_scriptRunner =
ScriptRunner::create(m_document.get());
On 2017/01/12 11:21:00, haraken wrote:
> 
> It might be slightly cleaner to move this logic into
> ScriptRunnerTest::initialize().

Done.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/TimerTest.cpp (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/TimerTest.cpp:30: void setupTimes(
On 2017/01/12 11:21:00, haraken wrote:
> On 2017/01/12 08:56:13, toyoshim wrote:
> > Changes in this class are also unfortunate, but...
> 
> setupTimes => initialize ?

Done.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/scroll/ScrollbarTestSuite.h:85: class
ScrollbarTestSuite : public testing::Test {
On 2017/01/12 11:21:00, haraken wrote:
> On 2017/01/12 08:56:13, toyoshim wrote:
> > This class is used in some tests in platform/scroll, but after this change,
> this
> > class get to be a little useless.
> 
> Yeah, maybe we want to remove it in a follow-up CL.

Acknowledged.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
(right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:123: //
correctly.  See also comments on ScopedTestingPlatformSupport.
Actually, existing TestingPlatformSupport and
TestingPlatformSupportWithMockScheduler can take a Config parameter in
constructors so that users can override compositorSupport behavior.

I think the original author intended that Config is used for overriding
stateless behavior like just returning preallocated object like
compositorSupport, and otherwise we can subclass this for introducing stateful
logic.

But anyway, I'd adopt your simple explanation here.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:210: // on
its destruction.  Instances could be nested.
On 2017/01/12 11:21:01, haraken wrote:
> 
> Slightly simpler:
> 
> // ScopedTestingPlatformSupport<MyTestingPlatformSupport> can be used to
> override Platform::current() with MyTestingPlatformSupport, like this:

Done.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:231: //   //
Here the origianl MyTestingPlatformSupport should be restored.
On 2017/01/12 11:21:01, haraken wrote:
> 
> original

Done.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h:234: class
ScopedTestingPlatformSupport final {
On 2017/01/12 11:21:01, haraken wrote:
> 
> This looks much simpler!

Acknowledged.

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/Platform.h (right):

https://codereview.chromium.org/2588403002/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/Platform.h:144: // inside a Platform
subclass's constructor.
On 2017/01/12 11:21:01, haraken wrote:
> 
> // you should not pass in a Platform object that is not fully instantiated

Done.

[Takashi Toyoshima, 2017-01-12 12:14:46]

Description was changed from

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests
==========

to

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests, webkit_unit_tests
==========

[Takashi Toyoshima, 2017-01-12 12:15:00]

The CQ bit was checked by toyoshim@chromium.org

[Takashi Toyoshima, 2017-01-12 12:15:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbroman@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2588403002/#ps140001
(title: "review #32")

[commit-bot: I haz the power, 2017-01-12 12:15:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 14:00:26]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484223300952210,
"parent_rev": "552a72f3e4266b85153e64a98a10ae614c10a176", "commit_rev":
"f651acf25aaea54d6aed2488c8237214f1a2b38f"}

[commit-bot: I haz the power, 2017-01-12 14:01:00]

Description was changed from

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests, webkit_unit_tests
==========

to

==========
TestingPlatformSupport: register Platform instance correctly

Now, TestingPlatformSupport registers itself as the current Platform
instance, but it is inside the constructor.

On the other hand, a child class TestingPlatformSupportWithMockScheduler
expects the parent class reigsters it for children.

But, to register a Platform instance, virtual methods should be ready
to call. Rephrasing it, we can not register a Platform instance
correctly inside a super-class's constructor.

This patch provides ScopedTestingPlatformSupport class template that
manages TestingPlatformSupport instance and Platform instance
installation.

Without this patch, Platform::current()->mainThread() returns a
wrong WebThread, and it makes new tests that I will add fail.

BUG=n/a
TEST=platform_blink_unittests, webkit_unit_tests

Review-Url: https://codereview.chromium.org/2588403002
Cr-Commit-Position: refs/heads/master@{#443219}
Committed:
https://chromium.googlesource.com/chromium/src/+/f651acf25aaea54d6aed2488c823...
==========

[commit-bot: I haz the power, 2017-01-12 14:01:02]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f651acf25aaea54d6aed2488c823...