Refactor NavigationHandleImpl::RegisterNavigationThrottles.

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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 <iterator>
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
@@ skipping 1073 matching lines @@
   }
 
   if (!callback.is_null())
     callback.Run(result);
 
   // No code after running the callback, as it might have resulted in our
   // destruction.
 }
 
 void NavigationHandleImpl::RegisterNavigationThrottles() {
-  // Register the navigation throttles. The vector returned by
-  // CreateThrottlesForNavigation is not assigned to throttles_ directly because
-  // it would overwrite any throttles previously added with
-  // RegisterThrottleForTesting.
-  // TODO(carlosk, arthursonzogni): should simplify this to either use
-  // |throttles_| directly (except for the case described above) or
-  // |throttles_to_register| for registering all throttles.
-  std::vector<std::unique_ptr<NavigationThrottle>> throttles_to_register =
-      GetDelegate()->CreateThrottlesForNavigation(this);
+  // Note: |throttle_| might not be empty. Some NavigationThrottle might have

[clamy, 2017/07/11 12:32:37]

s/NavigationThrottle/NavigationThrottles

[arthursonzogni, 2017/07/11 14:37:23]

Done.

+  // been registered with RegisterThrottleForTesting. These must reside at the

[clamy, 2017/07/11 12:32:37]

s/These must reside ... elies on this./
TestNavigationManagerThrottle expects that the NavigationThrottles added for
test are the last NavigationThrottles to execute. Take them out while appending
the rest of the NavigationThrottles.

[arthursonzogni, 2017/07/11 14:37:23]

Done.

+  // end of |throttles_|. TestNavigationManagerThrottle relies on this.
+  std::vector<std::unique_ptr<NavigationThrottle>> testing_throttles =
+      std::move(throttles_);
+
+  throttles_ = GetDelegate()->CreateThrottlesForNavigation(this);
 
   // Check for renderer-inititated main frame navigations to data URLs. This is
   // done first as it may block the main frame navigation altogether.
-  std::unique_ptr<NavigationThrottle> data_url_navigation_throttle =
-      DataUrlNavigationThrottle::CreateThrottleForNavigation(this);
-  if (data_url_navigation_throttle)
-    throttles_to_register.push_back(std::move(data_url_navigation_throttle));
+  AddThrottle(DataUrlNavigationThrottle::CreateThrottleForNavigation(this));
 
-  std::unique_ptr<content::NavigationThrottle> ancestor_throttle =
-      content::AncestorThrottle::MaybeCreateThrottleFor(this);
-  if (ancestor_throttle)
-    throttles_.push_back(std::move(ancestor_throttle));
-
-  std::unique_ptr<content::NavigationThrottle> form_submission_throttle =
-      content::FormSubmissionThrottle::MaybeCreateThrottleFor(this);
-  if (form_submission_throttle)
-    throttles_.push_back(std::move(form_submission_throttle));
+  AddThrottle(AncestorThrottle::MaybeCreateThrottleFor(this));
+  AddThrottle(FormSubmissionThrottle::MaybeCreateThrottleFor(this));
 
   // Check for mixed content. This is done after the AncestorThrottle and the
   // FormSubmissionThrottle so that when folks block mixed content with a CSP
   // policy, they don't get a warning. They'll still get a warning in the
   // console about CSP blocking the load.
-  std::unique_ptr<NavigationThrottle> mixed_content_throttle =
-      MixedContentNavigationThrottle::CreateThrottleForNavigation(this);
-  if (mixed_content_throttle)
-    throttles_to_register.push_back(std::move(mixed_content_throttle));
+  AddThrottle(
+      MixedContentNavigationThrottle::CreateThrottleForNavigation(this));
 
-  std::unique_ptr<NavigationThrottle> devtools_throttle =
-      RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this);
-  if (devtools_throttle)
-    throttles_to_register.push_back(std::move(devtools_throttle));
+  AddThrottle(RenderFrameDevToolsAgentHost::CreateThrottleForNavigation(this));
 
-  throttles_.insert(throttles_.begin(),
-                    std::make_move_iterator(throttles_to_register.begin()),
-                    std::make_move_iterator(throttles_to_register.end()));
+  throttles_.insert(throttles_.end(),

[clamy, 2017/07/11 12:32:37]

nit: maybe add a comment
// Insert all testing NavigationThrottles last.

[arthursonzogni, 2017/07/11 14:37:23]

Done.

+                    std::make_move_iterator(testing_throttles.begin()),
+                    std::make_move_iterator(testing_throttles.end()));
+}
+
+void NavigationHandleImpl::AddThrottle(
+    std::unique_ptr<NavigationThrottle> throttle) {
+  if (throttle)
+    throttles_.push_back(std::move(throttle));
 }
 
 bool NavigationHandleImpl::IsSelfReferentialURL() {
   // about: URLs should be exempted since they are reserved for other purposes
   // and cannot be the source of infinite recursion. See
   // https://crbug.com/341858 .
   if (url_.SchemeIs("about"))
     return false;
 
   // Browser-triggered navigations should be exempted.
@@ skipping 36 matching lines @@
   // Stop expecting a navigation to the current site URL in the current expected
   // process.
   SetExpectedProcess(nullptr);
 
   // Update the site URL and the expected process.
   site_url_ = new_site_url;
   SetExpectedProcess(post_redirect_process);
 }
 
 }  // namespace content