Have ExtensionHost use TabContents instead of RenderViewHost. Try #3.

chrome/browser/geolocation/chrome_geolocation_permission_context.cc

 // Copyright (c) 2011 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.
 
 #include "chrome/browser/geolocation/chrome_geolocation_permission_context.h"
 
 #include <functional>
 #include <string>
 #include <vector>
 
@@ skipping 518 matching lines @@
     const Extension* ext = extensions->GetExtensionByURL(requesting_frame);
     if (!ext)
       ext = extensions->GetExtensionByWebExtent(requesting_frame);
     if (ext && ext->HasAPIPermission(ExtensionAPIPermission::kGeolocation)) {
       // Make sure the extension is in the calling process.
       if (extensions->process_map()->Contains(ext->id(), render_process_id)) {
         NotifyPermissionSet(render_process_id, render_view_id, bridge_id,
                             requesting_frame, true);
         return;
       }
     }

[joth, 2011/11/17 09:45:12]

I just recalled a question from a previous review thread, I never quite
understood why we don't just have "if (ext) return" here?

What is the use case for a permission request originating from an extension
being handled by any of the code-blocks below?

[Matt Perry, 2011/11/17 20:08:04]

That I don't know. I tried to preserve the code as-is because this is a
refactor, and I didn't want to unintentionally break something.

   }
 
   TabContents* tab_contents =
       tab_util::GetTabContentsByID(render_process_id, render_view_id);
-  if (!tab_contents) {
+  if (!tab_contents ||
+      tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {

[Matt Perry, 2011/11/16 21:44:10]

This fixes the geolocation bug I mentioned.

[Aaron Boodman, 2011/11/16 23:29:24]

This seems like it could be a breaking change. I vaguely recall that it is
possible to dynamically request geolocation this way from an extension
background page today.

[Matt Perry, 2011/11/16 23:36:32]

That's handled by the block above - we only reach here if the extension doesn't
have permission (or it's not an extension).

[Aaron Boodman, 2011/11/16 23:42:23]

Right, what I mean is that I worry that before it may have been possible to have
an extension that does _not_ have geolocation API permission in the manifest,
and call whatever the JS function is to request permission, and you'd get the
infobar and everything would work.

[Matt Perry, 2011/11/16 23:46:06]

I see. There was a test that broke without this change (marked flaky so I didn't
catch it), but that suite is now happy.

Also, just based on the code, I'm pretty sure this is equivalent to the behavior
before my refactor. There was no associated tab for a BG page or popup before,
which are the only cases where the tab's view_type is not TAB_CONTENTS.

     // The tab may have gone away, or the request may not be from a tab at all.
+    // TODO(mpcomplete): the request could be from a background page or
+    // extension popup (tab_contents will have a different ViewType). But why do
+    // we care? Shouldn't we still put an infobar up in the current tab?

[joth, 2011/11/17 08:58:33]

I don't quite understand this last question: are you suggesting that an
extension background page should cause whatever tab happens to be focused to get
an info bar? In the current focused window or all windows? What if I change or
close tab?

My understanding was extensions either request (and get) permission at install
time or they don't and that's it. No runtime popups permitted.
The geolocation UI is very much targetted to the tab it is in requesting
permission, and is already complicated by the corner case of an embedded frame
(from differing origin) requesting permission. I wouldn't want to complicate it
further with non-tab related things unless the UX team really intend it to.

[Matt Perry, 2011/11/17 20:08:04]

Thanks, your explanation answers my question.

On 2011/11/17 08:58:33, joth wrote:

     LOG(WARNING) << "Attempt to use geolocation tabless renderer: "
                  << render_process_id << "," << render_view_id << ","
                  << bridge_id << " (can't prompt user without a visible tab)";

[joth, 2011/11/17 08:58:33]

assuming this warning is useful, it would be as well to include (tab_contents ?
tab_contents->GetRenderViewType() : -1) in it too.

     NotifyPermissionSet(render_process_id, render_view_id, bridge_id,
                         requesting_frame, false);
     return;
   }
 
   GURL embedder = tab_contents->GetURL();
   if (!requesting_frame.is_valid() || !embedder.is_valid()) {
     LOG(WARNING) << "Attempt to use geolocation from an invalid URL: "
                  << requesting_frame << "," << embedder
                  << " (geolocation is not supported in popups)";
@@ skipping 72 matching lines @@
         BrowserThread::UI, FROM_HERE,
         base::Bind(
             &ChromeGeolocationPermissionContext::CancelPendingInfoBarRequest,
             this, render_process_id, render_view_id, bridge_id));
      return;
   }
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   geolocation_infobar_queue_controller_->CancelInfoBarRequest(render_process_id,
       render_view_id, bridge_id);
 }