Mac: Only show one per-tab sheet at a time per tab.

Mac: Only show one per-tab sheet at a time per tab.

BUG=26900
TEST=Go to http://www/~thakis/cgi-bin/test.html . Test that two sheets show up (the first expects "u" as user and no pass, the other "v" and no pass). The first sheet appears immediately, the second after 2 seconds. Try entering u for the first faster than 2 seconds and switch tabs, when coming back to the original tab, the 2nd sheet should wait for you. Try closing the window and the tab while a sheet is showing, both when the tab with the sheet is in the background and in the foreground.

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

[Nico, 2009-11-13 05:59:20]

More or less the same thing as last time, but mac-only.

[viettrungluu, 2009-11-13 18:17:48]

Basically LGTM, with my comments (mostly demands for more comments :-) )
addressed.

http://codereview.chromium.org/384113/diff/1/2
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/384113/diff/1/2#newcode163
Line 163: - (BOOL)attachConstrainedWindow:(ConstrainedWindowMac*)window;
This didn't have a comment before, but it deserves one. Now even more so, since
it returns something.

http://codereview.chromium.org/384113/diff/1/2#newcode164
Line 164: - (void)removeConstrainedWindow:(ConstrainedWindowMac*)window;
This too.

http://codereview.chromium.org/384113/diff/1/5
File chrome/browser/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/384113/diff/1/5#newcode87
Line 87: }
Could you please add a comment here about why we don't immediately make it
visible?

http://codereview.chromium.org/384113/diff/1/6
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/384113/diff/1/6#newcode102
Line 102: std::map<NSView*, std::deque<ConstrainedWindowMac*> >
constrainedWindows_;
Please add an explanation indicating that we have per-tab queues.

http://codereview.chromium.org/384113/diff/1/6#newcode176
Line 176: - (BOOL)attachConstrainedWindow:(ConstrainedWindowMac*)window;
Comments here too.

http://codereview.chromium.org/384113/diff/1/7
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/384113/diff/1/7#newcode1370
Line 1370: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
Someone (apparently me) should point out that using std::map's operator[] to
access elements is evil (if the key isn't in the dictionary, it'll add it).
<shrug>

http://codereview.chromium.org/384113/diff/1/7#newcode1372
Line 1372: find(windows.begin(), windows.end(), window);
Is this necessary? Are we really allowed to -attachConstrainedWindow: with the
same window more than once? Should this be a DCHECK() instead?

I guess this is okay.

http://codereview.chromium.org/384113/diff/1/7#newcode1401
Line 1401: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
Evil, I say!

http://codereview.chromium.org/384113/diff/1/7#newcode1418
Line 1418:
windows[0]->Realize(static_cast<BrowserWindowController*>(controller));
I'm not a big fan of windows[0] as an idiom here; maybe windows.front() instead?

[Nico, 2009-11-13 18:35:15]

Thanks for the swift review!

http://codereview.chromium.org/384113/diff/1/2
File chrome/browser/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/384113/diff/1/2#newcode163
Line 163: - (BOOL)attachConstrainedWindow:(ConstrainedWindowMac*)window;
On 2009/11/13 18:17:48, viettrungluu wrote:
> This didn't have a comment before, but it deserves one. Now even more so,
since
> it returns something.

Done.

http://codereview.chromium.org/384113/diff/1/2#newcode164
Line 164: - (void)removeConstrainedWindow:(ConstrainedWindowMac*)window;
On 2009/11/13 18:17:48, viettrungluu wrote:
> This too.

Done.

http://codereview.chromium.org/384113/diff/1/5
File chrome/browser/cocoa/constrained_window_mac.mm (right):

http://codereview.chromium.org/384113/diff/1/5#newcode87
Line 87: }
On 2009/11/13 18:17:48, viettrungluu wrote:
> Could you please add a comment here about why we don't immediately make it
> visible?

Done?

http://codereview.chromium.org/384113/diff/1/6
File chrome/browser/cocoa/tab_strip_controller.h (right):

http://codereview.chromium.org/384113/diff/1/6#newcode102
Line 102: std::map<NSView*, std::deque<ConstrainedWindowMac*> >
constrainedWindows_;
On 2009/11/13 18:17:48, viettrungluu wrote:
> Please add an explanation indicating that we have per-tab queues.

Done.

http://codereview.chromium.org/384113/diff/1/6#newcode176
Line 176: - (BOOL)attachConstrainedWindow:(ConstrainedWindowMac*)window;
On 2009/11/13 18:17:48, viettrungluu wrote:
> Comments here too.

Done.

http://codereview.chromium.org/384113/diff/1/7
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/384113/diff/1/7#newcode1370
Line 1370: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
On 2009/11/13 18:17:48, viettrungluu wrote:
> Someone (apparently me) should point out that using std::map's operator[] to
> access elements is evil (if the key isn't in the dictionary, it'll add it).
> <shrug>

I know. In this case, that's exactly what's intended though, since I want to add
something to said queue, so it's ok if it springs into existence.

(I realize I'm in the minority here, but I like this behavior of map.)

http://codereview.chromium.org/384113/diff/1/7#newcode1372
Line 1372: find(windows.begin(), windows.end(), window);
On 2009/11/13 18:17:48, viettrungluu wrote:
> Is this necessary? Are we really allowed to -attachConstrainedWindow: with the
> same window more than once? Should this be a DCHECK() instead?
> 
> I guess this is okay.

Yes, that does happen. ConstrainedWindow is supposed to call attach
attachConstrainedWindow: each time its Realize() is called until the function
returns YES. The first call will add the sheet to the queue. (I now mention this
explicitly in one of the comments I added.)

http://codereview.chromium.org/384113/diff/1/7#newcode1401
Line 1401: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
On 2009/11/13 18:17:48, viettrungluu wrote:
> Evil, I say!

:-)

When removeConstrainedWindow: is called, at least one sheet must've been
visible, hence the deque already exists anyway.

http://codereview.chromium.org/384113/diff/1/7#newcode1418
Line 1418:
windows[0]->Realize(static_cast<BrowserWindowController*>(controller));
On 2009/11/13 18:17:48, viettrungluu wrote:
> I'm not a big fan of windows[0] as an idiom here; maybe windows.front()
instead?

But then it's over 80 cols! :-D

Done, but what's wrong with [0]? I get the impression that you don't like
operator overloading in general :-)

[viettrungluu, 2009-11-13 22:48:54]

http://codereview.chromium.org/384113/diff/1/7
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/384113/diff/1/7#newcode1401
Line 1401: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
On 2009/11/13 18:35:15, Nico wrote:
> On 2009/11/13 18:17:48, viettrungluu wrote:
> > Evil, I say!
> 
> :-)
> 
> When removeConstrainedWindow: is called, at least one sheet must've been
> visible, hence the deque already exists anyway.

And if something goes horribly wrong, and it doesn't exist, it'll conveniently
and silently create one for you!

http://codereview.chromium.org/384113/diff/1/7#newcode1418
Line 1418:
windows[0]->Realize(static_cast<BrowserWindowController*>(controller));
On 2009/11/13 18:35:15, Nico wrote:
> On 2009/11/13 18:17:48, viettrungluu wrote:
> > I'm not a big fan of windows[0] as an idiom here; maybe windows.front()
> instead?
> 
> But then it's over 80 cols! :-D

The 80-columns thing almost makes it okay. But only almost. :-P

> Done, but what's wrong with [0]? I get the impression that you don't like
> operator overloading in general :-)

I just think deques (I hate that as a word) should have a front and a back (or
equivalent), rather than having random access. I could try to make an argument
that .front() might be slightly more efficient (not having a parameter), but I
won't (since I don't want to look at a disassembly).

[Nico, 2009-11-13 22:55:38]

http://codereview.chromium.org/384113/diff/1/7
File chrome/browser/cocoa/tab_strip_controller.mm (right):

http://codereview.chromium.org/384113/diff/1/7#newcode1401
Line 1401: std::deque<ConstrainedWindowMac*>& windows =
constrainedWindows_[tab];
On 2009/11/13 22:48:54, viettrungluu wrote:
> On 2009/11/13 18:35:15, Nico wrote:
> > On 2009/11/13 18:17:48, viettrungluu wrote:
> > > Evil, I say!
> > 
> > :-)
> > 
> > When removeConstrainedWindow: is called, at least one sheet must've been
> > visible, hence the deque already exists anyway.
> 
> And if something goes horribly wrong, and it doesn't exist, it'll conveniently
> and silently create one for you!

But in that case, things have already gone horribly wrong anyway. AND I erase it
again just a few lines below.

http://codereview.chromium.org/384113/diff/1/7#newcode1418
Line 1418:
windows[0]->Realize(static_cast<BrowserWindowController*>(controller));
On 2009/11/13 22:48:54, viettrungluu wrote:
> On 2009/11/13 18:35:15, Nico wrote:
> > On 2009/11/13 18:17:48, viettrungluu wrote:
> > > I'm not a big fan of windows[0] as an idiom here; maybe windows.front()
> > instead?
> > 
> > But then it's over 80 cols! :-D
> 
> The 80-columns thing almost makes it okay. But only almost. :-P
> 
> > Done, but what's wrong with [0]? I get the impression that you don't like
> > operator overloading in general :-)
> 
> I just think deques (I hate that as a word) should have a front and a back (or
> equivalent), rather than having random access.

Why? I thought the main selling point of a deque (instead of a list) is that it
has fast random access _and_ fast push_front and push_back operations?

> I could try to make an argument
> that .front() might be slightly more efficient (not having a parameter), but I
> won't (since I don't want to look at a disassembly).

Big-O is all that matters :-)