Fix how captive portals track which page is loading.

chrome/browser/captive_portal/captive_portal_tab_helper.h

Index: chrome/browser/captive_portal/captive_portal_tab_helper.h
===================================================================
--- chrome/browser/captive_portal/captive_portal_tab_helper.h	(revision 151564)
+++ chrome/browser/captive_portal/captive_portal_tab_helper.h	(working copy)
@@ -13,6 +13,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "webkit/glue/resource_type.h"
 
 class GURL;
 class Profile;
@@ -35,9 +36,9 @@
 // to inform the tab's CaptivePortalLoginDetector when the tab is at a captive
 // portal's login page.
 //
-// TODO(mmenke): Support redirects.  Needed for HSTS, which simulates redirects
-//               at the network layer.  Also may reduce the number of
-//               unnecessary captive portal checks on high latency connections.
+// The TabHelper assumes that a WebContents can only have one RenderViewHost
+// with a provisional load at a time, and tracks only that navigation.  While
+// this may not be perfectly accurate in all cases, it should be good enough.

[cbentzel, 2012/08/15 17:00:38]

Can you be clearer in this comment about a case where there would be more than
one RVH with a provisional load?

[mmenke, 2012/08/16 14:30:03]

Done.

 //
 // For the design doc, see:
 // https://docs.google.com/document/d/1k-gP2sswzYNvryu9NcgN7q5XrsMlUdlUdoW9WRaEmfM/edit
@@ -85,16 +86,12 @@
   // page.  This is set to false when a captive portal is no longer detected.
   bool IsLoginTab() const;
 
- private:
-  friend class CaptivePortalBrowserTest;
-  friend class CaptivePortalTabHelperTest;
-
+ protected:

[cbentzel, 2012/08/15 17:00:38]

Why are these protected instead of private?

[mmenke, 2012/08/16 14:30:03]

Since I had to override GetProvisionalChildID for unit tests anyways, I had the
subclass friend the test fixture, rather than the CaptivePortalTabHelper itself.
 I was thinking this would make it slightly clearer what functions were used by
the tests.

Since this apparently wasn't true, and the tests no longer need to use a
subclass, I've gone ahead and removed the protected section.

   // Called by Observe in response to the corresponding event.
-  void OnRedirect(int64 frame_id, const GURL& new_url);
+  void OnRedirect(int child_id,
+                  ResourceType::Type resource_type,
+                  const GURL& new_url);
 
-  // Called by Observe in response to the corresponding event.
-  void OnCaptivePortalResults(Result previous_result, Result result);
-
   // Called to indicate a tab is at, or is navigating to, the captive portal
   // login page.
   void SetIsLoginTab();
@@ -102,11 +99,25 @@
   // |this| takes ownership of |tab_reloader|.
   void SetTabReloaderForTest(CaptivePortalTabReloader* tab_reloader);
 
+  const content::RenderViewHost* provisional_render_view_host() const {
+    return provisional_render_view_host_;
+  }
+
+ private:
+  friend class CaptivePortalBrowserTest;
+
+  // Called by Observe in response to the corresponding event.
+  void OnCaptivePortalResults(Result previous_result, Result result);
+
   CaptivePortalTabReloader* GetTabReloaderForTest();
 
   // Opens a login tab if the profile's active window doesn't have one already.
   void OpenLoginTab();
 
+  // Returns the child ID of the provisional RenderViewHost's Renderer process.
+  // Returns -1 if there's no such RenderViewHost.
+  virtual int GetProvisionalChildID() const;
+
   // Neither of these will ever be NULL.
   scoped_ptr<CaptivePortalTabReloader> tab_reloader_;
   scoped_ptr<CaptivePortalLoginDetector> login_detector_;
@@ -118,10 +129,11 @@
   // net::OK, otherwise.
   int pending_error_code_;
 
-  // The ID of the main frame that's currently provisionally loaded, if there is
-  // one.  -1 (unknown/invalid) when there is no such frame, or when an id of
-  // -1 is passed to DidStartProvisionalLoadForFrame.
-  int64 provisional_main_frame_id_;
+  // The RenderViewHost with a provisional load, if any.  Can either be
+  // the currently displayed RenderViewHost or a pending RenderViewHost for
+  // cross-process navitations.  NULL when there's currently no provisional
+  // load.
+  content::RenderViewHost* provisional_render_view_host_;
 
   content::NotificationRegistrar registrar_;