Adds RequestContextType information to the NavigationHandle.

content/browser/frame_host/navigation_handle_impl_browsertest.cc

 // Copyright 2016 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 "content/browser/frame_host/navigation_handle_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/request_context_type.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test_utils_internal.h"
 #include "net/dns/mock_host_resolver.h"
 #include "ui/base/page_transition_types.h"
 #include "url/url_constants.h"
@@ skipping 120 matching lines @@
         will_start_result_(will_start_result),
         will_redirect_result_(will_redirect_result),
         will_process_result_(will_process_result),
         did_call_will_start_(did_call_will_start),
         did_call_will_redirect_(did_call_will_redirect),
         did_call_will_process_(did_call_will_process) {}
   ~TestNavigationThrottle() override {}
 
   void Resume() { navigation_handle()->Resume(); }
 
+  RequestContextType fetch_request_context_type() {
+    return fetch_request_context_type_;
+  }
+
  private:
   // NavigationThrottle implementation.
   NavigationThrottle::ThrottleCheckResult WillStartRequest() override {
+    NavigationHandleImpl* navigation_handle_impl =
+        static_cast<NavigationHandleImpl*>(navigation_handle());
+    CHECK_NE(REQUEST_CONTEXT_TYPE_UNSPECIFIED,

[nasko, 2016/07/21 22:04:35]

Why would it be unspecified? At the start of the request, shouldn't we know what
it is?

[carlosk, 2016/07/22 12:51:26]

The only thing we know here is that it should have been set and not to
UNSPECIFIED. The next CHECKs below confirm the value won't change as the
navigation progresses.

Given these guarantees the actual test can simply check a single point in time
in the navigation for the expected value to have to whole flow tested.

+             navigation_handle_impl->fetch_request_context_type());
+    fetch_request_context_type_ =
+        navigation_handle_impl->fetch_request_context_type();
+
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, did_call_will_start_);
     return will_start_result_;
   }
 
   NavigationThrottle::ThrottleCheckResult WillRedirectRequest() override {
+    NavigationHandleImpl* navigation_handle_impl =
+        static_cast<NavigationHandleImpl*>(navigation_handle());
+    CHECK_EQ(fetch_request_context_type_,
+             navigation_handle_impl->fetch_request_context_type());
+
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             did_call_will_redirect_);
     return will_redirect_result_;
   }
 
   NavigationThrottle::ThrottleCheckResult WillProcessResponse() override {
+    NavigationHandleImpl* navigation_handle_impl =
+        static_cast<NavigationHandleImpl*>(navigation_handle());
+    CHECK_EQ(fetch_request_context_type_,
+             navigation_handle_impl->fetch_request_context_type());
+
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                             did_call_will_process_);
     return will_process_result_;
   }
 
   NavigationThrottle::ThrottleCheckResult will_start_result_;
   NavigationThrottle::ThrottleCheckResult will_redirect_result_;
   NavigationThrottle::ThrottleCheckResult will_process_result_;
   base::Closure did_call_will_start_;
   base::Closure did_call_will_redirect_;
   base::Closure did_call_will_process_;
+  RequestContextType fetch_request_context_type_;
 };
 
 // Install a TestNavigationThrottle on all following requests and allows waiting
 // for various NavigationThrottle related events. Waiting works only for the
 // immediately next navigation. New instances are needed to wait for further
 // navigations.
 class TestNavigationThrottleInstaller : public WebContentsObserver {
  public:
   TestNavigationThrottleInstaller(
       WebContents* web_contents,
@@ skipping 463 matching lines @@
   // Wait for the end of the navigation.
   navigation_observer.Wait();
 
   EXPECT_TRUE(observer.has_committed());
   EXPECT_TRUE(observer.was_redirected());
   EXPECT_FALSE(observer.is_error());
   EXPECT_EQ(shell()->web_contents()->GetLastCommittedURL(),
             GURL(embedded_test_server()->GetURL("bar.com", "/title2.html")));
 }
 
+// Checks that the RequestContextType value is properly set.
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
+                       VerifyRequestContextType) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b(c))"));

[clamy, 2016/07/21 11:30:21]

I'm not sure I see the point of testing frame c here. Also, I would find it more
interesting to test various kind of navigations, to get different values for
REQUEST_CONTEXT_TYPE.

[nasko, 2016/07/21 22:04:35]

Actually, testing a frame that is not direct child of the main frame is a good
way to catch bugs that assume frame->parent is the top and we've seem some of
those :).

Ditto on the different context types though. The context is hardcoded a few
places and this test won't test much if it only checks for that one value.

[carlosk, 2016/07/22 12:51:26]

Oh well... I had removed the testing of the extra node and now reverted back to
having it. I hope you both agree? :)

I'm a bit puzzled about when do different values apply. For instance, I don't
understand why we don't get *_FRAME or *_IFRAME in these cases, but only
*_LOCATION. I did some codesearch-ing and found no C++ reference of those frame
related values (nor of their Blink counterparts in
WebURLRequest::RequestContext) that are used for *setting* any values (except
for tests).

I added tests for HYPERLINK and FORM. I tried and failed adding tests for IMAGE
and DOWNLOAD. For the former I ran a browser initiated navigation to an image
file but the type was still LOCATION. For the latter I was unable to get the
download part to work; I think I'd need to mix the harnesses from both this test
and download_browsertest.cc and that is too much trouble for this IMO.

Most other values should be for sub resources and not used in navigations (as
seems to be the case for IMAGE).

[clamy, 2016/07/22 13:51:31]

I think it's normal that a navigation to an image is marked as _LOCATION (most
important art is that it's a navigation). I agree that _DOWNLOAD is a bit hard
to get there.

[carlosk, 2016/07/22 16:00:29]

Acknowledged.

+  GURL b_url(embedded_test_server()->GetURL(
+      "b.com", "/cross_site_iframe_factory.html?b(c())"));
+  GURL c_url(embedded_test_server()->GetURL(
+      "c.com", "/cross_site_iframe_factory.html?c()"));
+
+  TestNavigationThrottleInstaller installer(
+      shell()->web_contents(), NavigationThrottle::PROCEED,
+      NavigationThrottle::PROCEED, NavigationThrottle::PROCEED);
+  TestNavigationManager main_manager(shell()->web_contents(), main_url);
+  TestNavigationManager b_manager(shell()->web_contents(), b_url);
+  TestNavigationManager c_manager(shell()->web_contents(), c_url);
+  NavigationStartUrlRecorder url_recorder(shell()->web_contents());
+  TestNavigationThrottle* last_throttle = nullptr;
+
+  // Waits until the end of the main frame navigation.
+  shell()->LoadURL(main_url);
+  main_manager.WaitForWillStartRequest();
+  // Checks the navigation is the one expected for the main frame.
+  EXPECT_NE(last_throttle, installer.navigation_throttle());
+  EXPECT_EQ(main_url, url_recorder.urls().back());
+  EXPECT_EQ(1ul, url_recorder.urls().size());
+  // Checks the main frame RequestContextType.
+  EXPECT_EQ(REQUEST_CONTEXT_TYPE_LOCATION,
+            installer.navigation_throttle()->fetch_request_context_type());
+  last_throttle = installer.navigation_throttle();
+
+  // Ditto for frame b.
+  main_manager.WaitForNavigationFinished();
+  b_manager.WaitForWillStartRequest();
+  EXPECT_NE(last_throttle, installer.navigation_throttle());
+  EXPECT_EQ(b_url, url_recorder.urls().back());
+  EXPECT_EQ(2ul, url_recorder.urls().size());
+  EXPECT_EQ(REQUEST_CONTEXT_TYPE_LOCATION,
+            installer.navigation_throttle()->fetch_request_context_type());
+  last_throttle = installer.navigation_throttle();
+
+  // Ditto for frame c.
+  b_manager.WaitForNavigationFinished();
+  c_manager.WaitForWillStartRequest();
+  EXPECT_NE(last_throttle, installer.navigation_throttle());
+  EXPECT_EQ(c_url, url_recorder.urls().back());
+  EXPECT_EQ(3ul, url_recorder.urls().size());
+  EXPECT_EQ(REQUEST_CONTEXT_TYPE_LOCATION,
+            installer.navigation_throttle()->fetch_request_context_type());
+  last_throttle = installer.navigation_throttle();
+
+  // Lets the final navigation finish so that we conclude running the checks
+  // that happen in TestNavigationThrottle.
+  c_manager.WaitForNavigationFinished();
+}
+
 // Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
 // URL from the very beginning of the navigation.
 class NavigationHandleImplHttpsUpgradeBrowserTest
     : public NavigationHandleImplBrowserTest {
  public:
   void CheckHttpsUpgradedIframeNavigation(const GURL& start_url,
                                           const GURL& iframe_secure_url) {
     ASSERT_TRUE(start_url.SchemeIs(url::kHttpScheme));
     ASSERT_TRUE(iframe_secure_url.SchemeIs(url::kHttpsScheme));
 
@@ skipping 44 matching lines @@
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplHttpsUpgradeBrowserTest,
                        StartUrlIsHttpsUpgradedCrossSite) {
   GURL start_url(
       embedded_test_server()->GetURL("/https_upgrade_cross_site.html"));
   GURL cross_site_iframe_secure_url("https://other.com/title1.html");
 
   CheckHttpsUpgradedIframeNavigation(start_url, cross_site_iframe_secure_url);
 }
 
 }  // namespace content