Move the render process host tests in content to chrome. These test chrome specific behavior with t…

content/browser/child_process_security_policy_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/file_path.h"
 #include "base/platform_file.h"
@@ skipping 158 matching lines @@
   EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("abOuT:cAChe")));
 
   // Requests for about: pages should be denied.
   p->GrantRequestURL(kRendererID, GURL("about:crash"));
   EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("about:crash")));
 
   // These requests for chrome:// pages should be granted.
   p->GrantRequestURL(kRendererID, GURL(content::kTestNewTabURL));
   EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL(content::kTestNewTabURL)));
 
-  p->GrantRequestURL(kRendererID, GURL(content::kTestHistoryURL));
-  EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL(content::kTestHistoryURL)));
-
-  p->GrantRequestURL(kRendererID, GURL(content::kTestBookmarksURL));
-  EXPECT_TRUE(p->CanRequestURL(kRendererID, GURL(content::kTestBookmarksURL)));

[Charlie Reis, 2012/07/27 19:51:09]

Shouldn't we check these in a chrome test instead of just removing them?

[jam, 2012/07/27 19:54:45]

I guess I didn't understand how this is different from the above check. is it
because in chrome, that's different?

are you saying I should create a new test in chrome, since that uses its own
Content*Client interfaces, and just move those 4 lines to it?

[Charlie Reis, 2012/07/27 20:03:19]

To be honest, I'm not sure why there are separate tests for NTP, history, and
bookmarks, but it might be to ensure that the NTP isn't special cased (i.e.,
that the whole chrome:// origin is granted).

@msw: You appear to have added these checks in
http://codereview.chromium.org/7068007.  Can you say anything about why all
three are needed?
Yes, that's what I was suggesting.  That way we get the chrome:// URLs out of a
content test, but still keep the test coverage (assuming this bit of coverage is
worthwhile).

[msw, 2012/07/27 20:59:05]

I don't remember exactly why I added these checks. "chrome:" isn't registered as
a WebSafeScheme afaict, but "about:" is a PseudoScheme. I suspect the purpose is
to ensure that PseudoScheme "about:" URL grants are denied, while (unknown?)
"chrome:" URL grants are accepted. If that's the case, it seems fine to remove
redundant "chrome:" URL checks. John, you removed some redundant "about:" URL
checks in crrev.com/120891 but I would gladly defer this decision to someone
with a clearer understanding of this security policy code (perhaps "chrome:"
should actually be a psuedo scheme too?).

[Charlie Reis, 2012/07/27 21:35:43]

Ok.  LGTM, then, but it might be good to get Tom's eyes on it to be sure we're
not losing any important coverage for CanRequestURL.

-
   p->Remove(kRendererID);
 }
 
 TEST_F(ChildProcessSecurityPolicyTest, JavaScriptTest) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
 
   p->Add(kRendererID);
 
   EXPECT_FALSE(p->CanRequestURL(kRendererID, GURL("javascript:alert('xss')")));
@@ skipping 294 matching lines @@
 
   // Renderers are added and removed on the UI thread, but the policy can be
   // queried on the IO thread.  The ChildProcessSecurityPolicy needs to be
   // prepared to answer policy questions about renderers who no longer exist.
 
   // In this case, we default to secure behavior.
   EXPECT_FALSE(p->CanRequestURL(kRendererID, url));
   EXPECT_FALSE(p->CanReadFile(kRendererID, file));
   EXPECT_FALSE(p->HasWebUIBindings(kRendererID));
 }