Support manual (white|black)list, previewing and allowing after interstitial

chrome/browser/managed_mode/managed_mode_navigation_observer.h

Index: chrome/browser/managed_mode/managed_mode_navigation_observer.h
diff --git a/chrome/browser/managed_mode/managed_mode_navigation_observer.h b/chrome/browser/managed_mode/managed_mode_navigation_observer.h
index 8ac23a817f41bd3865582df722ccf7c04e8f6203..1ce35827edd2e69597fbd8ff17032e53c659ecb1 100644
--- a/chrome/browser/managed_mode/managed_mode_navigation_observer.h
+++ b/chrome/browser/managed_mode/managed_mode_navigation_observer.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_MANAGED_MODE_NAVIGATION_OBSERVER_H_
-#define CHROME_BROWSER_MANAGED_MODE_NAVIGATION_OBSERVER_H_
+#ifndef CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_NAVIGATION_OBSERVER_H_
+#define CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_NAVIGATION_OBSERVER_H_
 
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -17,20 +17,32 @@ class ManagedModeNavigationObserver
  public:
   virtual ~ManagedModeNavigationObserver();
 
+  // The resource throttle contains an observer:hostname map to check when
+  // deciding whether to show an interstitial or not. These functions
+  // add/remove the current observer to/from that map.
+  void AddObserverOnIOThread();
+  void RemoveObserverOnIOThread();
+
+  // Sets the specific infobar as dismissed.
   void WarnInfobarDismissed();
   void PreviewInfobarDismissed();
 
+  // Dismisses an infobar without user action on the infobar itself.

[Bernhard Bauer, 2012/11/26 16:06:30]

"Dismiss" means a user action (clicking on the little (x) button on the
infobar). I think you mean remove?

[Sergiu, 2012/11/27 15:37:34]

You were right, it's pretty unsafe to have it set arbitrary pointers to NULL so
I just removed the function.

+  void DismissInfobar(InfoBarDelegate** dismissed_infobar_delegate);
+
   // Adds the url to the list of navigated URLs if it was not already there.
-  void AddNavigatedURL(const GURL& url);
+  void SaveNavigatedURL(const GURL& url);

[Bernhard Bauer, 2012/11/26 16:06:30]

This method could be private?

[Sergiu, 2012/11/27 15:37:34]

Made private.

+
+  // Whitelist exact URLs for redirects and host patterns for the final URL.
+  // If the URL uses HTTPS whitelist only the host on HTTPS.
+  void AddSavedURLsToWhitelist();
+
+  // Marks/clears the current interstitial state.

[Bernhard Bauer, 2012/11/26 16:06:30]

I would like for this class not to know about interstitials. Can you describe
these methods purely in terms of how they affect the behavior/state of this
class? 

[Sergiu, 2012/11/27 15:37:34]

Refactored the two variables after_interstitial and redirects_completed into a
state variable, tell me what you think about that.

+  void MarkShownInterstitial();
+  void ClearShownInterstitial();
 
-  // For the list of URLs that were navigated since the last Observer
-  // instantiation:
-  //  * add the first N-1 to the whitelist such that only those URLs are
-  //  allowed and not more general versions of them.
-  //  * add only the host part of the last URL so that it allows all
-  //  pages on that domain. If it's HTTPS do the same but only on the HTTPS
-  //  protocol.
-  void AddURLList();
+  // Clears the state recorded in relation with a shown interstitial.
+  void ClearInterstitialState();
 
  private:
   friend class WebContentsUserData<ManagedModeNavigationObserver>;
@@ -38,6 +50,17 @@ class ManagedModeNavigationObserver
   explicit ManagedModeNavigationObserver(content::WebContents* web_contents);
 
   // content::WebContentsObserver implementation.
+  // An example regarding the order in which these event take place for

[Bernhard Bauer, 2012/11/26 16:06:30]

Nit: "events"

[Sergiu, 2012/11/27 15:37:34]

Done.

+  // google.com in our case is as follows:
+  // 1. User types in google.com and clicks enter.
+  //  -> NavigateToPendingEntry: http://google.com
+  //  -> DidStartProvisionalLoadForFrame http://google.com
+  //  -> ProvisionalChangeToMainFrameUrl http://google.com
+  // 2. Interstitial is shown to the user. User clicks "Preview".
+  //  -> ProvisionalChangeToMainFrameUrl http://www.google.com (redirect)
+  //  -> DidCommitProvisionalLoadForFrame http://www.google.com (redirect)
+  //  -> DidNavigateMainFrame http://www.google.com
+  // 3. Page is shown.
   virtual void NavigateToPendingEntry(
       const GURL& url,
       content::NavigationController::ReloadType reload_type) OVERRIDE;
@@ -69,10 +92,11 @@ class ManagedModeNavigationObserver
   InfoBarDelegate* preview_infobar_delegate_;
   std::vector<GURL> navigated_urls_;
   bool after_interstitial_;
+  bool redirects_completed_;

[Bernhard Bauer, 2012/11/26 16:06:30]

I think we should have an explicit state enum here. 

[Sergiu, 2012/11/27 15:37:34]

Done.

 
   int last_allowed_page_;
 
   DISALLOW_COPY_AND_ASSIGN(ManagedModeNavigationObserver);
 };
 
-#endif  // CHROME_BROWSER_MANAGED_MODE_NAVIGATION_OBSERVER_H_
+#endif  // CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_NAVIGATION_OBSERVER_H_