Resource: helper class to prohibit {add,remove}Client() calls in scope

third_party/WebKit/Source/core/fetch/ResourceTest.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceTest.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceTest.cpp b/third_party/WebKit/Source/core/fetch/ResourceTest.cpp
index f0a77a9c9d097b8b299f2a5cafc8b616b953c9bc..b9a547e55a4f369f743640bc7ac5688cd7f6f919 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceTest.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceTest.cpp
@@ -51,6 +51,25 @@ void createTestResourceAndSetCachedMetadata(const ResourceResponse& response) {
   return;
 }
 
+class TestProhibitAddRemoveClientResource : public Resource {

[hiroshige, 2016/10/05 08:37:25]

optional: I'd like to avoid creating new test-only subclass of Resource, but
this is still OK.
(Alternatively, we might skip this test and add a new test when we add actual
use of ProhibitAddRemoveClientInScope).

[hiroshige, 2016/10/05 08:37:25]

Add "final".

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Done.

+ public:
+  static TestProhibitAddRemoveClientResource* create() {
+    return new TestProhibitAddRemoveClientResource(
+        ResourceRequest(), Resource::Raw, ResourceLoaderOptions());

[kouhei (in TOK), 2016/10/05 08:29:42]

We can just pass these to Resource ctor in line 70. and remove ctor args from
line 67.

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Done.

+  }
+  void test() {

[kouhei (in TOK), 2016/10/05 08:29:42]

We should have this logic in line 112 func.

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Moving EXPECT_DEATH()s to test function.

+    ProhibitAddRemoveClientInScope prohibitAddRemoveClient(this);
+    EXPECT_DEATH(addClient(nullptr), "!m_isAddRemoveClientProhibited");

[hiroshige, 2016/10/05 08:37:25]

addClient(nullptr) is invalid even outside ProhibitAddRemoveClientInScope, so
please use MockResourceClient or something non-null.

[Shao-Chuan Lee, 2016/10/05 08:51:20]

Since MockResourceClient calls addClient in ctor, I tried

EXPECT_DEATH(new MockResourceClient(this), ...)

but the problem is EXPECT_DEATH seems to fork another process for the check,
then it crashes in Oilpan code (operator new -> enterGCForbiddenScopeIfNeeded()
-> checkThread()).

To workaround this I'm now checking if stderr matches
"!m_isAddRemoveClientProhibited", or we should modify MockResourceClient or have
another minimalist ResourceClient subclass.

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Done.

+    EXPECT_DEATH(removeClient(nullptr), "!m_isAddRemoveClientProhibited");

[hiroshige, 2016/10/05 08:37:26]

ditto.

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Done.

+  }
+
+ private:
+  TestProhibitAddRemoveClientResource(const ResourceRequest& request,

[hiroshige, 2016/10/05 08:37:25]

We can remove these parameters as they receive only fixed values.

[Shao-Chuan Lee, 2016/10/05 09:04:53]

Done.

+                                      Type type,
+                                      const ResourceLoaderOptions& options)
+      : Resource(request, type, options) {}
+};
+
 }  // anonymous namespace
 
 TEST(ResourceTest, SetCachedMetadata_SendsMetadataToPlatform) {
@@ -89,4 +108,8 @@ TEST(ResourceTest, RevalidateWithFragment) {
   resource->responseReceived(revalidatingResponse, nullptr);
 }
 
+TEST(ResourceTest, ProhibitAddRemoveClientInScope) {
+  TestProhibitAddRemoveClientResource::create()->test();
+}
+
 }  // namespace blink