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

chrome/browser/managed_mode/managed_mode_resource_throttle.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_RESOURCE_THROTTLE_H_
 #define CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_RESOURCE_THROTTLE_H_
 
 #include "base/compiler_specific.h"
 #include "base/memory/weak_ptr.h"
 #include "content/public/browser/resource_throttle.h"
 
 class ManagedModeURLFilter;
 
 namespace net {
 class URLRequest;
 }
 
-namespace policy {
-class URLBlacklist;
-}
-
 class ManagedModeResourceThrottle : public content::ResourceThrottle {
  public:
   ManagedModeResourceThrottle(const net::URLRequest* request,
                               int render_process_host_id,
                               int render_view_id,
                               bool is_main_frame);
   virtual ~ManagedModeResourceThrottle();
 
+  // Adds/removes the observer characterized by the render_process_host_id and

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

AddObserver makes it sound like the method adds an observer to this object,
which is not really what happens. What we do is (temporarily) whitelist all
requests for the given RenderView and host?

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

Renamed.

+  // render_view_id to the preview map. Adding saves the last approved
+  // hostname as well.
+  static void AddObserver(int render_process_host_id,
+                          int render_view_id,
+                          const std::string& host);
+  static void RemoveObserver(int render_process_host_id, int render_view_id);
+
+  // Checks whether the |url| needs to trigger an interstitial. If it
+  // |is_redirect| and the user already saw an interstitial which lead to this

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

Nit: the simple past of "lead" is "led".

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

Done.

+  // url then do not show another one.

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

Nit: URL in upper case if you use it as a noun, |url| in pipes if you're
referring to the variable.

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

Done.

+  void CheckNeedToShowInterstitial(bool is_redirect,

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

This actually *does* show the interstitial if necessary, right? You should name
the method to indicate that.

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

Done.

+                                   const GURL& url,
+                                   bool* defer);
+
   // content::ResourceThrottle implementation:
   virtual void WillStartRequest(bool* defer) OVERRIDE;
 
+  virtual void WillRedirectRequest(const GURL& new_url, bool* defer) OVERRIDE;
+
  private:
   void OnInterstitialResult(bool continue_request);
 
   base::WeakPtrFactory<ManagedModeResourceThrottle> weak_ptr_factory_;
   const net::URLRequest* request_;
   int render_process_host_id_;
   int render_view_id_;
   bool is_main_frame_;
+  // A resource throttle is in preview mode if the user clicked "Preview" after
+  // an interstitial.

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

I think this class also shouldn't know about preview mode. Can we just talk
about this in terms of behavior (i.e. in some cases we show an interstitial, in
some cases we don't)?

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

Renamed it to after_interstitial_ which I think fits and relates to the
ResourceThrottle.

+  bool is_in_preview_mode_;
   const ManagedModeURLFilter* url_filter_;
 
   DISALLOW_COPY_AND_ASSIGN(ManagedModeResourceThrottle);
 };
 
 #endif  // CHROME_BROWSER_MANAGED_MODE_MANAGED_MODE_RESOURCE_THROTTLE_H_