Support the Clear-Site-Data header on resource requests

content/browser/browsing_data/clear_site_data_throttle_unittest.cc

Index: content/browser/browsing_data/clear_site_data_throttle_unittest.cc
diff --git a/content/browser/browsing_data/clear_site_data_throttle_unittest.cc b/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
index bdfa8fad3d519996800691f6526b3e3ad320f0f1..1911a6ab03d19880c8076bcf2f8354f4e98d987f 100644
--- a/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
+++ b/content/browser/browsing_data/clear_site_data_throttle_unittest.cc
@@ -7,7 +7,12 @@
 #include <memory>
 
 #include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/scoped_command_line.h"
+#include "content/common/net/url_request_service_worker_data.h"
 #include "content/public/common/content_switches.h"
+#include "net/base/load_flags.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -15,20 +20,45 @@ namespace content {
 
 class ClearSiteDataThrottleTest : public testing::Test {
  public:
-  void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableExperimentalWebPlatformFeatures);
-    throttle_ = ClearSiteDataThrottle::CreateThrottleForNavigation(nullptr);
-  }
+  void SetUp() override { throttle_.reset(new ClearSiteDataThrottle(nullptr)); }
 
   ClearSiteDataThrottle* GetThrottle() {
     return static_cast<ClearSiteDataThrottle*>(throttle_.get());
   }
 
  private:
-  std::unique_ptr<NavigationThrottle> throttle_;
+  base::MessageLoop message_loop_;
+  std::unique_ptr<ResourceThrottle> throttle_;
 };
 
+TEST_F(ClearSiteDataThrottleTest, CreateThrottleForRequest) {
+  // Enable experimental features.
+  std::unique_ptr<base::test::ScopedCommandLine> command_line(
+      new base::test::ScopedCommandLine());
+  command_line->GetProcessCommandLine()->AppendSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+
+  // Create a URL request.
+  GURL url("https://www.example.com");
+  net::TestURLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(
+      context.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr));
+
+  // We will not create the throttle for an empty ResourceRequestInfo.
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // We can create the throttle for a valid ResourceRequestInfo.
+  ResourceRequestInfo::AllocateForTesting(request.get(), RESOURCE_TYPE_IMAGE,
+                                          nullptr, 0, 0, 0, false, true, true,
+                                          true, false);
+  EXPECT_TRUE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // But not if experimental web features are disabled again.
+  request->SetLoadFlags(net::LOAD_NORMAL);
+  command_line.reset();
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+}
+
 TEST_F(ClearSiteDataThrottleTest, ParseHeader) {

[mmenke, 2016/10/21 15:16:21]

We don't have any tests of the deferring behavior, which I think we should have.
 We could have a browser test that makes sure we clear cookies before following
a redirect, but that could flakily pass, while a well-written unit test would
not.

If we go with a browser test, I'd suggest a redirect test (Something we don't
have but need somewhere).  Set a cookie manually, request a resource which has
the clear header and redirects us to another site, which sets another cookie. 
Then make sure the first cookie was cleared, wasn't set with the request, and
the second one is set at the end of the test.

[mmenke, 2016/10/21 15:16:21]

I suggest making all these tests more integration-y.  De-friend the individual
tests, create a mock ResourceController that checks if a particular cookie /
whatever has been / has not been cleared on Resume, and then call the throttle's
WillStartRequest / WillRedirectRequest methods, as in production code, check if
we're deferred or not (as expected), and if deferred, wait for the resume call. 
That more fully checks all expected behaviors.

If the BrowsingDataRemover can't easily be used in unit tests, then just make a
virtual method that posts the task to call ClearSiteDataOnUIThread, then
subclass it, and not go through ClearSiteDataThrottle::CreateThrottleForRequest
for most of these tests.

The browser tests do a reasonable job, but think we could have better coverage
here, WDYT?  Not sure about mocking out stuff like console messages, either. 
Could be it's just too much work.

I'm not going to push too much on this, given the good coverage we're getting
from your browser tests.

[msramek, 2016/10/31 19:23:36]

I disassembled ClearSiteDataThrottle a bit to make it better mockable, added a
few tests for error messages and ClearSiteDataThrottleTest.DeferAndResume to
test deferring.

Regarding the the set-cookie-and-delete-cookie test, neither the unittest or
browsertest can reasonably do that, as the call goes through
ContentBrowserClient out to the browser. We could implement a mock cookie
storage / MockContentBrowserClient, but at that point we're just testing the
mock implementation, which is as good as testing EXPECT_CALL.

There is one problem where I got stuck, maybe you can help me there. I wanted to
write the test about deferring in the following way:

Create a request, create a throttle, then keep modifying the request (e.g.
redirect it somewhere) and EXPECT_CALLs on the throttle. The problem is that I
don't know how to reasonably modify the request - all interesting methods are
non-public. URLRequestJob is a friend that has access to them, but subclasses of
URLRequestJob are not friends.

We could for example run the request against an EmbeddedTestServer, but I
already have this kind of stuff in the browsertest, and don't want to copy over
the whole infrastructure.

So instead, I just always create a new request and then just manually run
WillRedirectRequest() or WillProcessResponse() and see what happens. This mostly
achieves what I want, but it's not elegant and the test could still pass if we
replaced URLRequest::url() with URLRequest::original_url(). Fortunately, we test
that in many other places.

[mmenke, 2016/11/08 18:23:59]

Hrm...That seems a little weird.  From a layering standpoint, this is part of a
web API, so doesn't seem like it should be dependent on Chrome to implement it,
just from a correctness standpoint.  Or, alternatively, content-shell should.

Beyond that, blimp, headless, views, cast, and Android WebView all fail to
implement the requisite method.  Do you have any plans here?  Is the plan to
support this feature on none of these?
Why do you want to modify the request?  You can register a request handler that
repeatedly returns redirects and varying headers.

   struct TestCase {
     const char* header;