Refactor ChromeGeolocationPermissionContext

chrome/browser/geolocation/chrome_geolocation_permission_context.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_GEOLOCATION_CHROME_GEOLOCATION_PERMISSION_CONTEXT_H_
 #define CHROME_BROWSER_GEOLOCATION_CHROME_GEOLOCATION_PERMISSION_CONTEXT_H_
 
+#include <string>
+
 #include "base/memory/scoped_ptr.h"
 #include "content/public/browser/geolocation_permission_context.h"
 
 class GeolocationInfoBarQueueController;
 class PrefService;
 class Profile;
 
 // Chrome specific implementation of GeolocationPermissionContext; manages
 // Geolocation permissions flow, and delegates UI handling via
 // GeolocationInfoBarQueueController.
 class ChromeGeolocationPermissionContext
     : public content::GeolocationPermissionContext {
  public:
-  explicit ChromeGeolocationPermissionContext(Profile* profile);
+  static ChromeGeolocationPermissionContext* Create(Profile *profile);

[bulach, 2012/10/17 12:59:33]

nit: s/Profile *p/Profile* p/

however, I just chatted with jknotten, and it feels strange to have this factory
method here, since this class can be derived. our suggestion would be:

- add another file, chrome_geolocation_permission_context_factory.h / .cc
- move this method there
- in the implementation, #include both the android and the regular files
- add #ifdefs in the implementation to create the right one..

[Ramya, 2012/10/17 17:38:43]

Agreed. Will make the change in the http://codereview.chromium.org/11188020/
which introduces android implementations.

 
   static void RegisterUserPrefs(PrefService *user_prefs);
 
   // GeolocationPermissionContext implementation:
   virtual void RequestGeolocationPermission(
       int render_process_id,
       int render_view_id,
       int bridge_id,
       const GURL& requesting_frame,
       base::Callback<void(bool)> callback) OVERRIDE;
   virtual void CancelGeolocationPermissionRequest(
       int render_process_id,
       int render_view_id,
       int bridge_id,
       const GURL& requesting_frame) OVERRIDE;
 
- private:
+ protected:
+  explicit ChromeGeolocationPermissionContext(Profile* profile);
   virtual ~ChromeGeolocationPermissionContext();
 
-  // Removes any pending InfoBar request.
-  void CancelPendingInfoBarRequest(int render_process_id,
-                                   int render_view_id,
-                                   int bridge_id);
+  // Decide whether the geolocation permission should be granted.
+  // Calls PermissionDecided if permission can be decided non-interactively,
+  // or NotifyPermissionSet if permission decided by presenting an
+  // infobar to the user. Called on the UI thread.
+  virtual void DecidePermission(int render_process_id,
+                                int render_view_id,
+                                int bridge_id,
+                                const GURL& requesting_frame,
+                                const GURL& embedder,
+                                base::Callback<void(bool)> callback);
+
+  // Called when permission is granted without interactively asking
+  // the user. Can be overridden to introduce additional UI flow.
+  // Should ultimately ensure that NotifyPermissionSet is called.
+  // Called on the UI thread.
+  virtual void PermissionDecided(int render_process_id,
+                                 int render_view_id,
+                                 int bridge_id,
+                                 const GURL& requesting_frame,
+                                 const GURL& embedder,
+                                 base::Callback<void(bool)> callback,
+                                 bool allowed);
 
   // Notifies whether or not the corresponding bridge is allowed to use
   // geolocation via
   // GeolocationPermissionContext::SetGeolocationPermissionResponse().
+  // Called on the UI thread.
   void NotifyPermissionSet(int render_process_id,
                            int render_view_id,
                            int bridge_id,
                            const GURL& requesting_frame,
                            base::Callback<void(bool)> callback,
                            bool allowed);
 
+  // Return an instance of the infobar queue controller, creating it
+  // if necessary.
+  GeolocationInfoBarQueueController* QueueController();
+
+  // Create an InfoBarQueueController. overriden in derived classes to provide
+  // additional UI flow.  Called on the UI thread.
+  virtual GeolocationInfoBarQueueController* CreateQueueController();
+
+  Profile* profile() const { return profile_; }

[bulach, 2012/10/17 12:59:33]

nit: I think this method and QueueController() should go up in 44, i.e., all non
virtuals before the virtual methods..

also, please add a similar header as 27 before the virtuals:
  // GeolocationPermissionContext implementation:

[Ramya, 2012/10/17 17:38:43]

Done.

+
+ private:
+  // Removes any pending InfoBar request.
+  void CancelPendingInfoBarRequest(int render_process_id,
+                                   int render_view_id,
+                                   int bridge_id);
+
   // This must only be accessed from the UI thread.
   Profile* const profile_;
 
   scoped_ptr<GeolocationInfoBarQueueController>
       geolocation_infobar_queue_controller_;
 
   DISALLOW_COPY_AND_ASSIGN(ChromeGeolocationPermissionContext);
 };
 
 #endif  // CHROME_BROWSER_GEOLOCATION_CHROME_GEOLOCATION_PERMISSION_CONTEXT_H_