TestingPlatformSupport: register Platform instance correctly

third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 19 matching lines @@
 
 #ifndef TestingPlatformSupport_h
 #define TestingPlatformSupport_h
 
 #include "platform/PlatformExport.h"
 #include "platform/WebTaskRunner.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebCompositorSupport.h"
 #include "public/platform/WebScheduler.h"
 #include "public/platform/WebThread.h"
+#include "wtf/Assertions.h"
 #include "wtf/Vector.h"
 #include <memory>
 
 namespace base {
 class SimpleTestTickClock;
 class TestDiscardableMemoryAllocator;
 }
 
 namespace cc {
 class OrderedSimpleTaskRunner;
@@ skipping 131 matching lines @@
 
  protected:
   static double getTestTime();
 
   std::unique_ptr<base::SimpleTestTickClock> m_clock;
   scoped_refptr<cc::OrderedSimpleTaskRunner> m_mockTaskRunner;
   std::unique_ptr<scheduler::RendererSchedulerImpl> m_scheduler;
   std::unique_ptr<WebThread> m_thread;
 };
 
+template <class T>
+class ScopedTestingPlatformSupport {

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

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

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

Add STACK_ALLOCATED.

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

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.

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

comments including usage were added.

+ public:
+  ScopedTestingPlatformSupport() : m_originalPlatform(Platform::current()) {}

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

Do we need this constructor?

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

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.

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

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...

+  explicit ScopedTestingPlatformSupport(
+      std::unique_ptr<T> testingPlatformSupport)
+      : m_originalPlatform(Platform::current()) {
+    reset(std::move(testingPlatformSupport));
+  }
+  void reset(std::unique_ptr<T> testingPlatformSupport) {
+    DCHECK_NE(m_testingPlatformSupport.get(), testingPlatformSupport.get());
+    if (testingPlatformSupport.get())

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

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

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

Good point. To restore a valid platform instance always safely, I modified this
method a little.

+      Platform::setCurrentPlatformForTesting(testingPlatformSupport.get());
+    m_testingPlatformSupport = std::move(testingPlatformSupport);
+  }
+  ~ScopedTestingPlatformSupport() {
+    m_testingPlatformSupport.reset();
+    if (m_originalPlatform)
+      Platform::setCurrentPlatformForTesting(m_originalPlatform);

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

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

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

Done.

+  }
+  const T* operator->() const { return m_testingPlatformSupport.get(); }
+  T* operator->() { return m_testingPlatformSupport.get(); }
+
+ private:
+  Platform* m_originalPlatform;
+  std::unique_ptr<T> m_testingPlatformSupport;
+};
+
 class ScopedUnittestsEnvironmentSetup {
   WTF_MAKE_NONCOPYABLE(ScopedUnittestsEnvironmentSetup);
 
  public:
   ScopedUnittestsEnvironmentSetup(int argc, char** argv);
   ~ScopedUnittestsEnvironmentSetup();
 
  private:
   class DummyPlatform;
   std::unique_ptr<base::TestDiscardableMemoryAllocator>
       m_discardableMemoryAllocator;
   std::unique_ptr<DummyPlatform> m_platform;
   std::unique_ptr<cc_blink::WebCompositorSupportImpl> m_compositorSupport;
   TestingPlatformSupport::Config m_testingPlatformConfig;
-  std::unique_ptr<TestingPlatformSupport> m_testingPlatformSupport;
+  ScopedTestingPlatformSupport<TestingPlatformSupport> m_testingPlatformSupport;
 };
 
 }  // namespace blink
 
 #endif  // TestingPlatformSupport_h