Location chooser

chrome/browser/geolocation/geolocation_permission_context.cc

Index: chrome/browser/geolocation/geolocation_permission_context.cc
diff --git a/chrome/browser/geolocation/geolocation_permission_context.cc b/chrome/browser/geolocation/geolocation_permission_context.cc
index 496b500587b18ec69a63bc3795eb6710264e265c..42ec58ee94924f6e2aea529b34a75597c905d273 100644
--- a/chrome/browser/geolocation/geolocation_permission_context.cc
+++ b/chrome/browser/geolocation/geolocation_permission_context.cc
@@ -35,17 +35,19 @@ class GeolocationPermissionRequest : public PermissionBubbleRequest {
                                const GURL& requesting_frame,
                                const GURL& embedder,
                                bool user_gesture,
-                               base::Callback<void(bool)> callback,
+                               base::Callback<void(int, bool)> callback,
                                const std::string& display_languages);
   virtual ~GeolocationPermissionRequest();
 
   // PermissionBubbleDelegate:
+  virtual PermissionBubbleRequest::Type GetType() const OVERRIDE;
   virtual int GetIconID() const OVERRIDE;
   virtual base::string16 GetMessageText() const OVERRIDE;
   virtual base::string16 GetMessageTextFragment() const OVERRIDE;
   virtual bool HasUserGesture() const OVERRIDE;
   virtual GURL GetRequestingHostname() const OVERRIDE;
   virtual void PermissionGranted() OVERRIDE;
+  void PermissionGranted(int choice);

[meacer, 2014/08/15 22:20:25]

nit: Maybe you might want to rename this to something that implies precision.

[mhm, 2014/08/15 23:18:57]

Done.

   virtual void PermissionDenied() OVERRIDE;
   virtual void Cancelled() OVERRIDE;
   virtual void RequestFinished() OVERRIDE;
@@ -56,7 +58,7 @@ class GeolocationPermissionRequest : public PermissionBubbleRequest {
   GURL requesting_frame_;
   GURL embedder_;
   bool user_gesture_;
-  base::Callback<void(bool)> callback_;
+  base::Callback<void(int, bool)> callback_;
   std::string display_languages_;
 };
 
@@ -66,7 +68,7 @@ GeolocationPermissionRequest::GeolocationPermissionRequest(
     const GURL& requesting_frame,
     const GURL& embedder,
     bool user_gesture,
-    base::Callback<void(bool)> callback,
+    base::Callback<void(int, bool)> callback,
     const std::string& display_languages)
     : context_(context),
       id_(id),
@@ -74,10 +76,15 @@ GeolocationPermissionRequest::GeolocationPermissionRequest(
       embedder_(embedder),
       user_gesture_(user_gesture),
       callback_(callback),
-      display_languages_(display_languages) {}
+      display_languages_(display_languages) {
+}
 
 GeolocationPermissionRequest::~GeolocationPermissionRequest() {}
 
+PermissionBubbleRequest::Type GeolocationPermissionRequest::GetType() const {
+  return PermissionBubbleRequest::Type::kGeolocation;
+}
+
 int GeolocationPermissionRequest::GetIconID() const {
   return IDR_INFOBAR_GEOLOCATION;
 }
@@ -104,9 +111,15 @@ GURL GeolocationPermissionRequest::GetRequestingHostname() const {
 }
 
 void GeolocationPermissionRequest::PermissionGranted() {
+  PermissionGranted(-1);
+}
+
+void GeolocationPermissionRequest::PermissionGranted(int choice) {
+  // (mhm) Fix content setting to account for different choices.

[meacer, 2014/08/15 22:20:25]

nit: TODO(mohammed) since that's your Chromium username.

[mhm, 2014/08/15 23:18:57]

Done.

   context_->QueueController()->UpdateContentSetting(
       requesting_frame_, embedder_, true);
-  context_->NotifyPermissionSet(id_, requesting_frame_, callback_, true);
+  context_->NotifyPermissionSet(
+      id_, requesting_frame_, callback_, true, choice);
 }
 
 void GeolocationPermissionRequest::PermissionDenied() {
@@ -143,7 +156,7 @@ void GeolocationPermissionContext::RequestGeolocationPermission(
     int bridge_id,
     const GURL& requesting_frame,
     bool user_gesture,
-    base::Callback<void(bool)> result_callback,
+    base::Callback<void(int, bool)> result_callback,
     base::Closure* cancel_callback) {
   GURL requesting_frame_origin = requesting_frame.GetOrigin();
   if (shutting_down_)
@@ -204,7 +217,7 @@ void GeolocationPermissionContext::DecidePermission(
     const GURL& requesting_frame,
     bool user_gesture,
     const GURL& embedder,
-    base::Callback<void(bool)> callback) {
+    base::Callback<void(int, bool)> callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   ContentSetting content_setting =
@@ -219,6 +232,8 @@ void GeolocationPermissionContext::DecidePermission(
       PermissionDecided(id, requesting_frame, embedder, callback, false);
       break;
     case CONTENT_SETTING_ALLOW:
+      // We have to save the choice in the context setting to send it here.
+      // (mhm) fix.

[meacer, 2014/08/15 22:20:24]

nit: TODO(mohammed): ...

[mhm, 2014/08/15 23:18:56]

Done.

[felt, 2014/08/15 23:20:15]

No TODOs for interns who are about to leave. :) This should be assigned to an
engineer, with an accompanying crbug number for the TODO.

       PermissionDecided(id, requesting_frame, embedder, callback, true);
       break;
     default:
@@ -236,11 +251,18 @@ void GeolocationPermissionContext::DecidePermission(
         }
       } else {
         // setting == ask. Prompt the user.
+        base::Callback<void(bool)> InfoBarRequestCallback =
+            base::Bind(callback, -1);
         QueueController()->CreateInfoBarRequest(
-            id, requesting_frame, embedder,
-                base::Bind(
-                    &GeolocationPermissionContext::NotifyPermissionSet,
-                base::Unretained(this), id, requesting_frame, callback));
+            id,
+            requesting_frame,
+            embedder,
+            base::Bind(
+                &GeolocationPermissionContext::NotifyInfoBarPermissionSet,
+                base::Unretained(this),
+                id,
+                requesting_frame,
+                InfoBarRequestCallback));

[meacer, 2014/08/15 22:20:25]

Does passing base::Bind(callback, -1) directly not work here? If not, you should
rename InfoBarRequestCallback to infobar_request_callback

[mhm, 2014/08/15 23:18:57]

Done.

       }
   }
 }
@@ -250,10 +272,15 @@ void GeolocationPermissionContext::CreateInfoBarRequest(
     const GURL& requesting_frame,
     const GURL& embedder,
     base::Callback<void(bool)> callback) {
-    QueueController()->CreateInfoBarRequest(
-        id, requesting_frame, embedder, base::Bind(
-            &GeolocationPermissionContext::NotifyPermissionSet,
-            base::Unretained(this), id, requesting_frame, callback));
+  QueueController()->CreateInfoBarRequest(
+      id,
+      requesting_frame,
+      embedder,
+      base::Bind(&GeolocationPermissionContext::NotifyInfoBarPermissionSet,
+                 base::Unretained(this),
+                 id,
+                 requesting_frame,
+                 callback));
 }
 
 void GeolocationPermissionContext::RequestFinished(
@@ -279,7 +306,7 @@ void GeolocationPermissionContext::PermissionDecided(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     const GURL& embedder,
-    base::Callback<void(bool)> callback,
+    base::Callback<void(int, bool)> callback,
     bool allowed) {
   NotifyPermissionSet(id, requesting_frame, callback, allowed);
 }
@@ -287,6 +314,34 @@ void GeolocationPermissionContext::PermissionDecided(
 void GeolocationPermissionContext::NotifyPermissionSet(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
+    base::Callback<void(int, bool)> callback,
+    bool allowed) {
+  NotifyPermissionSet(id, requesting_frame, callback, -1, allowed);

[meacer, 2014/08/15 22:20:24]

nit: You can add a constant such as kUnknownPrecision=-1 and pass the constant
by name here.

[mhm, 2014/08/15 23:18:57]

Done.

+}
+
+void GeolocationPermissionContext::NotifyPermissionSet(
+    const PermissionRequestID& id,
+    const GURL& requesting_frame,
+    base::Callback<void(int, bool)> callback,
+    bool allowed,
+    int choice) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // WebContents may have gone away (or not exists for extension).
+  TabSpecificContentSettings* content_settings =
+      TabSpecificContentSettings::Get(id.render_process_id(),
+                                      id.render_view_id());
+  if (content_settings) {
+    content_settings->OnGeolocationPermissionSet(requesting_frame.GetOrigin(),
+                                                 allowed);

[meacer, 2014/08/15 22:20:25]

This is fine, but do you need a TODO here, as choice doesn't seem to be passed
to content_settings?

[mhm, 2014/08/15 23:18:57]

Done.

+  }
+
+  callback.Run(allowed, choice);
+}
+
+void GeolocationPermissionContext::NotifyInfoBarPermissionSet(
+    const PermissionRequestID& id,
+    const GURL& requesting_frame,
     base::Callback<void(bool)> callback,
     bool allowed) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));