Simplify EffectiveNavigationPolicy and friends

We always pass kNavigationPolicyCurrentTab from FrameLoader,
which is turned into kNavigationPolicyNewForegroundTab.

This observation allows us to remove dead branches in
EffectiveNavigationPolicy and simplify some calls there.

Bug: 849055
Change-Id: I9d1c08fba784111f3a544685da3f69c69380bedc
Reviewed-on: https://chromium-review.googlesource.com/1100243Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567618}

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -705,37 +705,6 @@ bool FrameLoader::PrepareRequestForThisFrame(FrameLoadRequest& request) {
   return true;
 }
 
-static bool ShouldNavigateTargetFrame(NavigationPolicy policy) {
-  switch (policy) {
-    case kNavigationPolicyCurrentTab:
-      return true;
-
-    // Navigation will target a *new* frame (e.g. because of a ctrl-click),
-    // so the target frame can be ignored.
-    case kNavigationPolicyNewBackgroundTab:
-    case kNavigationPolicyNewForegroundTab:
-    case kNavigationPolicyNewWindow:
-    case kNavigationPolicyNewPopup:
-      return false;
-
-    // Navigation won't really target any specific frame,
-    // so the target frame can be ignored.
-    case kNavigationPolicyIgnore:
-    case kNavigationPolicyDownload:
-      return false;
-
-    case kNavigationPolicyHandledByClient:
-      // Impossible, because at this point we shouldn't yet have called
-      // client()->decidePolicyForNavigation(...).
-      NOTREACHED();
-      return true;
-
-    default:
-      NOTREACHED() << policy;
-      return true;
-  }
-}
-
 static WebNavigationType DetermineNavigationType(
     WebFrameLoadType frame_load_type,
     bool is_form_submission,
@@ -783,6 +752,8 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
   CHECK(!IsBackForwardLoadType(frame_load_type));
   DCHECK(passed_request.TriggeringEventInfo() !=
          WebTriggeringEventInfo::kUnknown);
+  DCHECK(policy != kNavigationPolicyHandledByClient &&
+         policy != kNavigationPolicyHandledByClientForInitialHistory);
 
   DCHECK(frame_->GetDocument());
   if (HTMLFrameOwnerElement* element = frame_->DeprecatedLocalOwner())
@@ -806,8 +777,11 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
                                   AtomicString(request.FrameName()), *frame_,
                                   request.GetResourceRequest().Url());
 
-  if (target_frame && target_frame != frame_ &&
-      ShouldNavigateTargetFrame(policy)) {
+  // Downloads and navigations which specifically target a *new* frame
+  // (e.g. because of a ctrl-click) should ignore the target.
+  bool should_navigate_target_frame = policy == kNavigationPolicyCurrentTab;
+
+  if (target_frame && target_frame != frame_ && should_navigate_target_frame) {
     if (target_frame->IsLocalFrame() &&
         !ToLocalFrame(target_frame)->IsNavigationAllowed()) {
       return;
@@ -829,10 +803,10 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
     if (policy == kNavigationPolicyDownload) {
       Client()->DownloadURL(request.GetResourceRequest());
       return;  // Navigation/download will be handled by the client.
-    } else if (ShouldNavigateTargetFrame(policy)) {
+    } else if (should_navigate_target_frame) {
       request.GetResourceRequest().SetFrameType(
           network::mojom::RequestContextFrameType::kAuxiliary);
-      CreateWindowForRequest(request, *frame_, policy);
+      CreateWindowForRequest(request, *frame_);
       return;  // Navigation will be handled by the new frame/window.
     }
   }

third_party/blink/renderer/core/page/create_window.cc

@@ -54,8 +54,7 @@ namespace blink {
 // Check that the desired NavigationPolicy |policy| is compatible with the
 // observed input event |current_event|.
 // TODO(dgozman): move this to navigation_policy.cc.
-NavigationPolicy EffectiveNavigationPolicy(NavigationPolicy policy,
-                                           const WebInputEvent* current_event,
+NavigationPolicy EffectiveNavigationPolicy(const WebInputEvent* current_event,
                                            const WebWindowFeatures& features) {
   // If our default configuration was modified by a script or wasn't
   // created by a user gesture, then show as a popup. Else, let this
@@ -71,38 +70,21 @@ NavigationPolicy EffectiveNavigationPolicy(NavigationPolicy policy,
       app_policy == kNavigationPolicyNewPopup) {
     // User and app agree that we want a new window; let the app override the
     // decorations.
-    user_policy = app_policy;
-  } else if (user_policy == kNavigationPolicyCurrentTab) {
+    return app_policy;
+  }
+
+  if (user_policy == kNavigationPolicyCurrentTab) {
     // User doesn't want a specific policy, use app policy instead.
-    user_policy = app_policy;
+    return app_policy;
   }
 
-  if (policy == kNavigationPolicyIgnore) {
+  if (user_policy == kNavigationPolicyDownload) {
     // When the input event suggests a download, but the navigation was
     // initiated by script, we should not override it.
-    if (user_policy == kNavigationPolicyDownload) {
-      return as_popup ? kNavigationPolicyNewPopup
-                      : kNavigationPolicyNewForegroundTab;
-    }
-    return user_policy;
-  }
-
-  if (policy == kNavigationPolicyNewBackgroundTab &&
-      user_policy != kNavigationPolicyNewBackgroundTab &&
-      !UIEventWithKeyState::NewTabModifierSetFromIsolatedWorld()) {
-    // Don't allow background tabs to be opened via script setting the
-    // event modifiers.
-    return kNavigationPolicyNewForegroundTab;
-  }
-
-  if (policy == kNavigationPolicyDownload &&
-      user_policy != kNavigationPolicyDownload) {
-    // Don't allow downloads to be triggered via script setting the event
-    // modifiers.
-    return kNavigationPolicyNewForegroundTab;
+    return app_policy;
   }
 
-  return policy;
+  return user_policy;
 }
 
 // Though isspace() considers \t and \v to be whitespace, Win IE doesn't when
@@ -233,10 +215,8 @@ WebWindowFeatures GetWindowFeaturesFromString(const String& feature_string) {
 static Frame* ReuseExistingWindow(LocalFrame& active_frame,
                                   LocalFrame& lookup_frame,
                                   const AtomicString& frame_name,
-                                  NavigationPolicy policy,
                                   const KURL& destination_url) {
-  if (!frame_name.IsEmpty() && !EqualIgnoringASCIICase(frame_name, "_blank") &&
-      policy == kNavigationPolicyIgnore) {
+  if (!frame_name.IsEmpty() && !EqualIgnoringASCIICase(frame_name, "_blank")) {
     if (Frame* frame = lookup_frame.FindFrameForNavigation(
             frame_name, active_frame, destination_url)) {
       if (!EqualIgnoringASCIICase(frame_name, "_self")) {
@@ -256,14 +236,16 @@ static Frame* ReuseExistingWindow(LocalFrame& active_frame,
 static Frame* CreateNewWindow(LocalFrame& opener_frame,
                               const FrameLoadRequest& request,
                               const WebWindowFeatures& features,
-                              NavigationPolicy policy,
+                              bool force_new_foreground_tab,
                               bool& created) {
   Page* old_page = opener_frame.GetPage();
   if (!old_page)
     return nullptr;
 
-  policy =
-      EffectiveNavigationPolicy(policy, CurrentInputEvent::Get(), features);
+  NavigationPolicy policy =
+      force_new_foreground_tab
+          ? kNavigationPolicyNewForegroundTab
+          : EffectiveNavigationPolicy(CurrentInputEvent::Get(), features);
 
   const SandboxFlags sandbox_flags =
       opener_frame.GetDocument()->IsSandboxed(
@@ -320,7 +302,7 @@ static Frame* CreateWindowHelper(LocalFrame& opener_frame,
                                  LocalFrame& lookup_frame,
                                  const FrameLoadRequest& request,
                                  const WebWindowFeatures& features,
-                                 NavigationPolicy policy,
+                                 bool force_new_foreground_tab,
                                  bool& created) {
   DCHECK(request.GetResourceRequest().RequestorOrigin() ||
          opener_frame.GetDocument()->Url().IsEmpty());
@@ -332,10 +314,10 @@ static Frame* CreateWindowHelper(LocalFrame& opener_frame,
   created = false;
 
   Frame* window =
-      features.noopener
+      features.noopener || force_new_foreground_tab
           ? nullptr
           : ReuseExistingWindow(active_frame, lookup_frame, request.FrameName(),
-                                policy, request.GetResourceRequest().Url());
+                                request.GetResourceRequest().Url());
 
   if (!window) {
     // Sandboxed frames cannot open new auxiliary browsing contexts.
@@ -353,7 +335,7 @@ static Frame* CreateWindowHelper(LocalFrame& opener_frame,
   }
 
   if (window) {
-    // JS can run inside reuseExistingWindow (via onblur), which can detach
+    // JS can run inside ReuseExistingWindow (via onblur), which can detach
     // the target window.
     if (!window->Client())
       return nullptr;
@@ -362,7 +344,8 @@ static Frame* CreateWindowHelper(LocalFrame& opener_frame,
     return window;
   }
 
-  return CreateNewWindow(opener_frame, request, features, policy, created);
+  return CreateNewWindow(opener_frame, request, features,
+                         force_new_foreground_tab, created);
 }
 
 DOMWindow* CreateWindow(const String& url_string,
@@ -436,7 +419,7 @@ DOMWindow* CreateWindow(const String& url_string,
   bool created;
   Frame* new_frame = CreateWindowHelper(
       opener_frame, *active_frame, opener_frame, frame_request, window_features,
-      kNavigationPolicyIgnore, created);
+      false /* force_new_foreground_tab */, created);
   if (!new_frame)
     return nullptr;
   if (new_frame->DomWindow()->IsInsecureScriptAccess(calling_window,
@@ -467,8 +450,7 @@ DOMWindow* CreateWindow(const String& url_string,
 }
 
 void CreateWindowForRequest(const FrameLoadRequest& request,
-                            LocalFrame& opener_frame,
-                            NavigationPolicy policy) {
+                            LocalFrame& opener_frame) {
   DCHECK(request.GetResourceRequest().RequestorOrigin() ||
          (opener_frame.GetDocument() &&
           opener_frame.GetDocument()->Url().IsEmpty()));
@@ -481,15 +463,12 @@ void CreateWindowForRequest(const FrameLoadRequest& request,
       opener_frame.GetDocument()->IsSandboxed(kSandboxPopups))
     return;
 
-  if (policy == kNavigationPolicyCurrentTab)
-    policy = kNavigationPolicyNewForegroundTab;
-
   WebWindowFeatures features;
   features.noopener = request.GetShouldSetOpener() == kNeverSetOpener;
   bool created;
-  Frame* new_frame =
-      CreateWindowHelper(opener_frame, opener_frame, opener_frame, request,
-                         features, policy, created);
+  Frame* new_frame = CreateWindowHelper(
+      opener_frame, opener_frame, opener_frame, request, features,
+      true /* force_new_foreground_tab */, created);
   if (!new_frame)
     return;
   if (request.GetShouldSendReferrer() == kMaybeSendReferrer) {

third_party/blink/renderer/core/page/create_window.h

@@ -49,14 +49,11 @@ DOMWindow* CreateWindow(const String& url_string,
                         LocalFrame& opener_frame,
                         ExceptionState&);
 
-void CreateWindowForRequest(const FrameLoadRequest&,
-                            LocalFrame& opener_frame,
-                            NavigationPolicy);
+void CreateWindowForRequest(const FrameLoadRequest&, LocalFrame& opener_frame);
 
 // Exposed for testing
 CORE_EXPORT NavigationPolicy
-EffectiveNavigationPolicy(NavigationPolicy,
-                          const WebInputEvent* current_event,
+EffectiveNavigationPolicy(const WebInputEvent* current_event,
                           const WebWindowFeatures&);
 
 // Exposed for testing

third_party/blink/renderer/core/page/effective_navigation_policy_test.cc

@@ -48,7 +48,7 @@ class EffectiveNavigationPolicyTest : public testing::Test {
     event.button = button;
     if (as_popup)
       features.tool_bar_visible = false;
-    return EffectiveNavigationPolicy(kNavigationPolicyIgnore, &event, features);
+    return EffectiveNavigationPolicy(&event, features);
   }
 
   WebWindowFeatures features;
@@ -154,46 +154,38 @@ TEST_F(EffectiveNavigationPolicyTest, MiddleClickPopup) {
 
 TEST_F(EffectiveNavigationPolicyTest, NoToolbarsForcesPopup) {
   features.tool_bar_visible = false;
-  EXPECT_EQ(
-      kNavigationPolicyNewPopup,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewPopup,
+            EffectiveNavigationPolicy(nullptr, features));
   features.tool_bar_visible = true;
-  EXPECT_EQ(
-      kNavigationPolicyNewForegroundTab,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewForegroundTab,
+            EffectiveNavigationPolicy(nullptr, features));
 }
 
 TEST_F(EffectiveNavigationPolicyTest, NoStatusBarForcesPopup) {
   features.status_bar_visible = false;
-  EXPECT_EQ(
-      kNavigationPolicyNewPopup,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewPopup,
+            EffectiveNavigationPolicy(nullptr, features));
   features.status_bar_visible = true;
-  EXPECT_EQ(
-      kNavigationPolicyNewForegroundTab,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewForegroundTab,
+            EffectiveNavigationPolicy(nullptr, features));
 }
 
 TEST_F(EffectiveNavigationPolicyTest, NoMenuBarForcesPopup) {
   features.menu_bar_visible = false;
-  EXPECT_EQ(
-      kNavigationPolicyNewPopup,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewPopup,
+            EffectiveNavigationPolicy(nullptr, features));
   features.menu_bar_visible = true;
-  EXPECT_EQ(
-      kNavigationPolicyNewForegroundTab,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewForegroundTab,
+            EffectiveNavigationPolicy(nullptr, features));
 }
 
 TEST_F(EffectiveNavigationPolicyTest, NotResizableForcesPopup) {
   features.resizable = false;
-  EXPECT_EQ(
-      kNavigationPolicyNewPopup,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewPopup,
+            EffectiveNavigationPolicy(nullptr, features));
   features.resizable = true;
-  EXPECT_EQ(
-      kNavigationPolicyNewForegroundTab,
-      EffectiveNavigationPolicy(kNavigationPolicyIgnore, nullptr, features));
+  EXPECT_EQ(kNavigationPolicyNewForegroundTab,
+            EffectiveNavigationPolicy(nullptr, features));
 }
 
 }  // namespace blink