Enforce that only one constrained window is displayed at once per tab.

Enforce that only one constrained window is displayed at once per tab.

BUG=26900, 27620
TEST=Go to http://www/~thakis/cgi-bin/test.html with chrome mac, win, linux. Try using the page, try quitting chrome when the first or the second auth sheet are still open, etc. (The broken image icons are ok btw). On linux, check that every of the auth dialogs has focus.

zelidrag@ made this work on windows too and landed it.

[Nico, 2009-11-13 00:01:02]

I still need to check stuff still works on windows/linux, but the idea should be
clear.

[Elliot Glaysher, 2009-11-13 00:08:57]

I'm slightly concerned about the design here.

1) I'm generally supportive of this, but I'm wondering if you've though out edge
cases of a ConstrainedWindow subclass being constructed but not displayed. Could
it hang something?

2) If you're going to do this, let's get rid of the name "ConstrainedWindow".
This name only makes sense if you know the history of the popup blocker.
Something like TabModalDialog or TabModalSheet would be better, as we no longer
use this interface for...well constrained windows. On Linux, it's a centered
dialog and, as you said on IRC, on mac it's a tab modal sheet.

http://codereview.chromium.org/392018/diff/1002/2021
File chrome/browser/tab_contents/tab_contents_view_gtk.cc (right):

http://codereview.chromium.org/392018/diff/1002/2021#newcode452
Line 452: for (constrained_window_) {
This compiles?

[Nico, 2009-11-13 00:25:29]

> I'm slightly concerned about the design here.
>
> 1) I'm generally supportive of this, but I'm wondering if you've though out
> edge cases of a ConstrainedWindow subclass being constructed but not
displayed.
> Could it hang something?

I can't think of any obvious problems. The only thing this is
currently used for is the http auth thing. It should not make a
difference to the auth connection if the window is visible or not
imho.

> 2) If you're going to do this, let's get rid of the name
> "ConstrainedWindow".
> This name only makes sense if you know the history of the popup blocker.
> Something like TabModalDialog or TabModalSheet would be better, as we no
> longer
> use this interface for...well constrained windows. On Linux, it's a centered
> dialog and, as you said on IRC, on mac it's a tab modal sheet.

"TabModalDialog" sounds like a great name. Would you mind if I file a
bug on me to do this and do this in a week or so, and not in this CL?

> http://codereview.chromium.org/392018/diff/1002/2021#newcode452
> Line 452: for (constrained_window_) {
> This compiles?

:-)

[Elliot Glaysher, 2009-11-13 00:47:19]

On 2009/11/13 00:25:29, Nico wrote:
> I can't think of any obvious problems. The only thing this is
> currently used for is the http auth thing. It should not make a
> difference to the auth connection if the window is visible or not
> imho.

Well, LGTM then.

> "TabModalDialog" sounds like a great name. Would you mind if I file a
> bug on me to do this and do this in a week or so, and not in this CL?

Ah yes, forgot that we weren't supposed to be doing major refactorings right
now. Go ahead.

[viettrungluu, 2009-11-13 01:17:13]

Is there anything which could go wrong if the TabContents is destroyed with
things still in the queue?

http://codereview.chromium.org/392018/diff/2028/2032
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/392018/diff/2028/2032#newcode63
Line 63: bool visible_;
Why is it |should_be_visible_| on Mac and just |visible_| here?

http://codereview.chromium.org/392018/diff/2028/2033
File chrome/browser/tab_contents/constrained_window.h (right):

http://codereview.chromium.org/392018/diff/2028/2033#newcode38
Line 38: // The ConstrainedWindow* should not be visible after creation.
s/The Constrained Window\*/The Constrained Window/

http://codereview.chromium.org/392018/diff/2028/2033#newcode43
Line 43: // Maks the Constrained Window visible. Only one Constrained Window is
shown
s/Maks/Makes

[Nico, 2009-11-13 04:23:36]

This works on OS X and Linux, but is crashy on windows. My views-fu is not
strong enough to fix windows reasonably quick. I will table this approach for
now and try to cook up something OS X-specific :-/

http://codereview.chromium.org/392018/diff/2028/2032
File chrome/browser/gtk/constrained_window_gtk.h (right):

http://codereview.chromium.org/392018/diff/2028/2032#newcode63
Line 63: bool visible_;
On 2009/11/13 01:17:13, viettrungluu wrote:
> Why is it |should_be_visible_| on Mac and just |visible_| here?

Because here the constrained window is made visible immediately in
ShowConstrainedWindow(), while on OS X the sheet might stay invisble until the
tab the sheet is attached to becomes the active tab.

http://codereview.chromium.org/392018/diff/2028/2033
File chrome/browser/tab_contents/constrained_window.h (right):

http://codereview.chromium.org/392018/diff/2028/2033#newcode38
Line 38: // The ConstrainedWindow* should not be visible after creation.
On 2009/11/13 01:17:13, viettrungluu wrote:
> s/The Constrained Window\*/The Constrained Window/

Done.

http://codereview.chromium.org/392018/diff/2028/2033#newcode43
Line 43: // Maks the Constrained Window visible. Only one Constrained Window is
shown
On 2009/11/13 01:17:13, viettrungluu wrote:
> s/Maks/Makes

Done.

[Nico, 2009-11-13 15:20:32]

For the records: For the near future, I'd like to go with
http://codereview.chromium.org/384113 . But I'd like to eventually get this
landed, as it fixes a focus problem on GTK and sounds like something we want to
do anyway.