Make sure we don't dismiss extension popups when the focus...

Make sure we don't dismiss extension popups when the focus
changes for a child window of the popup, as it is the case
with select popups.

BUG=28110
TEST=Make sure extension popups are still working as expected.
     Open an extension popup with a select popup (combo box)
     in it. Click on the select to bring up its popup, the
     extension popup should stay opened.


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

[Erik does not do reviews, 2009-12-02 01:26:11]

LGTM

Could you check if this happens to also fix the related window.alert problem?  I
thought there was a separate bug filed, but apparently not.

http://codereview.chromium.org/459005/diff/1/8
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/459005/diff/1/8#newcode470
chrome/browser/views/browser_actions_container.cc:470: // TODO(jcampan):
http://crbugs.com/29131 make that work on toolkit views
typo: crbug.com

[jcampan, 2009-12-02 01:47:33]

Sadly it does not fix the alert bug, as I believe the alert is not
parented to the popup.
I filed 29147 for the alert bug.
I'll land this CL.
Let me know if you think 29147 should be fixed for the release, I can
look at it.

Jay

On Tue, Dec 1, 2009 at 5:26 PM,  <erikkay@chromium.org> wrote:
> LGTM
>
> Could you check if this happens to also fix the related window.alert
> problem?  I
> thought there was a separate bug filed, but apparently not.
>
>
>
> http://codereview.chromium.org/459005/diff/1/8
> File chrome/browser/views/browser_actions_container.cc (right):
>
> http://codereview.chromium.org/459005/diff/1/8#newcode470
> chrome/browser/views/browser_actions_container.cc:470: // TODO(jcampan):
> http://crbugs.com/29131 make that work on toolkit views
> typo: crbug.com
>
> http://codereview.chromium.org/459005
>

[Erik does not do reviews, 2009-12-02 03:17:19]

Thanks for checking.

If you think it will wind up being relatively low risk, then I'd say go for
it.  Until recently, this wound up crashing the browser.  We fixed that
crasher, but there may be other side effects we haven't discovered yet.

Erik


On Tue, Dec 1, 2009 at 5:47 PM, Jay Campan <jcampan@chromium.org> wrote:

> Sadly it does not fix the alert bug, as I believe the alert is not
> parented to the popup.
> I filed 29147 for the alert bug.
> I'll land this CL.
> Let me know if you think 29147 should be fixed for the release, I can
> look at it.
>
> Jay
>
> On Tue, Dec 1, 2009 at 5:26 PM,  <erikkay@chromium.org> wrote:
> > LGTM
> >
> > Could you check if this happens to also fix the related window.alert
> > problem?  I
> > thought there was a separate bug filed, but apparently not.
> >
> >
> >
> > http://codereview.chromium.org/459005/diff/1/8
> > File chrome/browser/views/browser_actions_container.cc (right):
> >
> > http://codereview.chromium.org/459005/diff/1/8#newcode470
> > chrome/browser/views/browser_actions_container.cc:470: // TODO(jcampan):
> > http://crbugs.com/29131 make that work on toolkit views
> > typo: crbug.com
> >
> > http://codereview.chromium.org/459005
> >
>

[pamg, 2009-12-02 04:12:53]

See http://crbug.com/27758 . In particular, if popups don't go away when
they create alerts, some of the hack in app_modal_dialog.cc to prevent that
crash can be removed. We won't need to cache the TabContents anymore.
However, we should still observe EXTENSION_HOST_DESTROYED and call Cleanup()
if it's triggered, in case the ExtensionHost goes away in some other way
(the background page closes it?).

I'll be glad to fix that up once it's not needed anymore. It shouldn't get
in the way of fixing the alert-focus bug, since it only takes effect if the
popup goes away when the alert appears.

- Pam

On Tue, Dec 1, 2009 at 7:17 PM, Erik Kay <erikkay@chromium.org> wrote:

> Thanks for checking.
>
> If you think it will wind up being relatively low risk, then I'd say go for
> it.  Until recently, this wound up crashing the browser.  We fixed that
> crasher, but there may be other side effects we haven't discovered yet.
>
> Erik
>
>
> On Tue, Dec 1, 2009 at 5:47 PM, Jay Campan <jcampan@chromium.org> wrote:
>
>> Sadly it does not fix the alert bug, as I believe the alert is not
>> parented to the popup.
>> I filed 29147 for the alert bug.
>> I'll land this CL.
>> Let me know if you think 29147 should be fixed for the release, I can
>> look at it.
>>
>> Jay
>>
>> On Tue, Dec 1, 2009 at 5:26 PM,  <erikkay@chromium.org> wrote:
>> > LGTM
>> >
>> > Could you check if this happens to also fix the related window.alert
>> > problem?  I
>> > thought there was a separate bug filed, but apparently not.
>> >
>> >
>> >
>> > http://codereview.chromium.org/459005/diff/1/8
>> > File chrome/browser/views/browser_actions_container.cc (right):
>> >
>> > http://codereview.chromium.org/459005/diff/1/8#newcode470
>> > chrome/browser/views/browser_actions_container.cc:470: // TODO(jcampan):
>> > http://crbugs.com/29131 make that work on toolkit views
>> > typo: crbug.com
>> >
>> > http://codereview.chromium.org/459005
>> >
>>
>
>