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

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

The last try broke at least 2 things which I've fixed in this CL:
- Attempts to access geolocation from an extension would hang.
- We'd sometimes crash when shutting down the browser with an ExtensionPopup
open.

BUG=84146
TEST=extensions still work

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110394

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

This is mostly the same as last time, except for the two changes I point out
below.

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:545:
tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {
This fixes the geolocation bug I mentioned.

http://codereview.chromium.org/8587001/diff/1/chrome/browser/ui/views/extensi...
File chrome/browser/ui/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/ui/views/extensi...
chrome/browser/ui/views/extensions/extension_popup.cc:76: set_delegate(NULL);
This fixes the crash bug I mention. (BrowserBubble::SetFocus was being called,
which tries to call back into the delegate if it's non-NULL.)

[Aaron Boodman, 2011-11-16 23:29:23]

lgtm

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:545:
tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {
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]

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:545:
tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {
On 2011/11/16 23:29:24, Aaron Boodman wrote:
> 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.

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

[Matt Perry, 2011-11-16 23:37:55]

joth: could you take a look at chrome_geolocation_permission_context.cc to see
if I'm doing the right thing there?

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

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:545:
tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {
On 2011/11/16 23:36:32, Matt Perry wrote:
> On 2011/11/16 23:29:24, Aaron Boodman wrote:
> > 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.
> 
> That's handled by the block above - we only reach here if the extension
doesn't
> have permission (or it's not an extension).

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]

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:545:
tab_contents->GetRenderViewType() != content::VIEW_TYPE_TAB_CONTENTS) {
On 2011/11/16 23:42:23, Aaron Boodman wrote:
> On 2011/11/16 23:36:32, Matt Perry wrote:
> > On 2011/11/16 23:29:24, Aaron Boodman wrote:
> > > 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.
> > 
> > That's handled by the block above - we only reach here if the extension
> doesn't
> > have permission (or it's not an extension).
> 
> 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.

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.

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

+jknotten as he's been doing more in the permission context recently.

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:549: // we
care? Shouldn't we still put an infobar up in the current tab?
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.

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:552: <<
bridge_id << " (can't prompt user without a visible tab)";
assuming this warning is useful, it would be as well to include (tab_contents ?
tab_contents->GetRenderViewType() : -1) in it too.

[joth, 2011-11-17 09:45:11]

talking to John reminded me of one other question...

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:539: }
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]

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
File chrome/browser/geolocation/chrome_geolocation_permission_context.cc
(right):

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:539: }
On 2011/11/17 09:45:12, joth wrote:
> 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?
> 

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.

http://codereview.chromium.org/8587001/diff/1/chrome/browser/geolocation/chro...
chrome/browser/geolocation/chrome_geolocation_permission_context.cc:549: // we
care? Shouldn't we still put an infobar up in the current tab?
Thanks, your explanation answers my question.

On 2011/11/17 08:58:33, joth wrote:
> 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.