Remove WebContentsImpl::OnDidRedirectProvisionalLoad.

content/browser/web_contents/navigation_controller_impl_unittest.cc

Index: content/browser/web_contents/navigation_controller_impl_unittest.cc
diff --git a/content/browser/web_contents/navigation_controller_impl_unittest.cc b/content/browser/web_contents/navigation_controller_impl_unittest.cc
index b0b09a544e0d0def241b14566f98b45cf20755cb..c3d32614a508f36ec6ca6e03981a5fae8cc3667f 100644
--- a/content/browser/web_contents/navigation_controller_impl_unittest.cc
+++ b/content/browser/web_contents/navigation_controller_impl_unittest.cc
@@ -518,16 +518,10 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortCancelsPending) {
 
   // Now the navigation redirects.
   const GURL kRedirectURL("http://bee");
-  test_rvh()->OnMessageReceived(
-      ViewHostMsg_DidRedirectProvisionalLoad(0,  // routing_id
-                                             -1,  // pending page_id
-                                             GURL(),  // opener
-                                             kNewURL,  // old url
-                                             kRedirectURL));  // new url

[Charlie Reis, 2012/05/30 22:43:31]

I don't understand.  Doesn't this just remove the redirect (which is the
relevant part of the test)?

More specifically, does this modified version of the test fail if you undo the
security fix from the patch below?
http://codereview.chromium.org/6993037/diff/8001/content/browser/tab_contents...

[mmenke, 2012/05/30 22:50:43]

After this CL, the NavigationController will no longer receive any notification
on redirects, it looks to me, and thus can't update its title in response to the
redirect.

[Charlie Reis, 2012/05/31 16:56:27]

Ok, I experimentally verified that this updated test still catches regressions
for 83031.  That is, it fails if I re-introduce the bug into
WebContentsImpl::OnDidFailProvisionalLoadWithError.

Please update the comment on line 519 to also say that the RVH gets no
notification of the redirect, since the code is confusing as it stands.

 
   // We don't want to change the NavigationEntry's url, in case it cancels.
   // Prevents regression of http://crbug.com/77786.
-  EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());
+  //EXPECT_EQ(kNewURL, controller.GetPendingEntry()->GetURL());

[Charlie Reis, 2012/05/30 22:43:31]

Shouldn't have commented out code.  But again, this is a security bug regression
test you're removing.  Maybe I don't understand what you're trying to achieve. 
Is there a reason it's safe not to have these checks?

[Charlie Reis, 2012/05/31 16:56:27]

This is unfortunate.  I'd like to keep this line in case the bug comes back
elsewhere, but it's kind of meaningless without actually simulating a redirect. 
The only way I know to do that is with
ResourceDispatcherHost::NotifyReceivedRedirect, which isn't available in this
unit test.

I suppose it makes the most sense to remove this line.  You've removed the whole
method that contained the bug, so it's unlikely to regress.

[Deprecated (see juliatuttle), 2012/05/31 18:16:24]

Done.

 
   // It may abort before committing, if it's a download or due to a stop or
   // a new navigation from the user.