TestingPlatformSupport: register Platform instance correctly

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

Index: third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
diff --git a/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h b/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
index 159c0febae33681c850788625010da4a68eee369..035091aea01ad1fd890fe06d15b8d8a42690f9a8 100644
--- a/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
+++ b/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h
@@ -37,6 +37,7 @@
 #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>
 
@@ -188,6 +189,34 @@ class TestingPlatformSupportWithMockScheduler : public TestingPlatformSupport {
   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);
 
@@ -202,7 +231,7 @@ class ScopedUnittestsEnvironmentSetup {
   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