Addition of optional giveFocus parameter to experimental.popup.show(...)

Addition of a new parameter to the popup.show(...) Chrome extension API that allows the caller to specify the behaviour of the window focus when the pop-up is displayed.

I added a test for the new parameter in the ExtensionApiTest.FLAKY_Popup test.  I also corrected a truncated string in the unit-test.

Because the pop-up actually uses two windows, I had to change the WidgetWin::Show() routine to also reposition the window at the top.  If a window is shown and activated, it is automatically brought to the front.  Because we don't activate the pop-up when we want to keep the focus in the current extension view,  I had to bring it to the front so that it wouldn't be hidden behind the 'chrome-bubble' window.

BUG=none
TEST=ExtensionApiTest.FLAKY_Popup


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

[Jeff Timanus, 2009-12-02 20:24:49]

Hi Guys,

Please review this CL when you have a moment.

Jeff

[rafaelw, 2009-12-03 21:16:36]

[+pkasting]

Peter, please review change in WidgetWin.

http://codereview.chromium.org/454019/diff/1001/1005
File chrome/browser/views/extensions/extension_popup.h (right):

http://codereview.chromium.org/454019/diff/1001/1005#newcode40
chrome/browser/views/extensions/extension_popup.h:40: bool activate_on_show);
I feel "activate" is a bit misleading here if the only thing this controls is
whether the popup takes focus.

Actually, how about "grab_focus", or maybe "focus"? Peter?

http://codereview.chromium.org/454019/diff/1001/1005#newcode46
chrome/browser/views/extensions/extension_popup.h:46: virtual void Show(bool
activate);
ditto here (activate vs focus)

[Peter Kasting, 2009-12-03 21:43:25]

http://codereview.chromium.org/454019/diff/1001/1002
File views/widget/widget_win.cc (right):

http://codereview.chromium.org/454019/diff/1001/1002#newcode203
views/widget/widget_win.cc:203: SWP_NOACTIVATE | SWP_NOSIZE | SWP_NOMOVE);
You cannot do this.  This can break existing cases, e.g. the address bar
dropdown with certain IMEs.

Show() should not change Z-order.  If you need to change the Z-order, do it
explicitly, not here.

[Peter Kasting, 2009-12-03 21:46:13]

http://codereview.chromium.org/454019/diff/1001/1005
File chrome/browser/views/extensions/extension_popup.h (right):

http://codereview.chromium.org/454019/diff/1001/1005#newcode40
chrome/browser/views/extensions/extension_popup.h:40: bool activate_on_show);
On 2009/12/03 21:16:36, rafaelw wrote:
> I feel "activate" is a bit misleading here if the only thing this controls is
> whether the popup takes focus.
> 
> Actually, how about "grab_focus", or maybe "focus"? Peter?

The only thing that's wrong is the use of the word "focus" in the comments above
and below.  "Activate" is the correct term if we're talking about shifting the
focus from one top-level HWND to another.

[rafaelw, 2009-12-03 22:49:36]

http://codereview.chromium.org/454019/diff/1001/1005
File chrome/browser/views/extensions/extension_popup.h (right):

http://codereview.chromium.org/454019/diff/1001/1005#newcode40
chrome/browser/views/extensions/extension_popup.h:40: bool activate_on_show);
Peter is the expert. Nevermind me.

http://codereview.chromium.org/454019/diff/1001/1005#newcode46
chrome/browser/views/extensions/extension_popup.h:46: virtual void Show(bool
activate);
ditto nevermind me

[Jeff Timanus, 2009-12-05 01:38:01]

Hi Guys,

I addressed all of your remarks.  Please have another look.

Jeff

[Erik does not do reviews, 2009-12-05 15:38:23]

one drive-by nit.  I'll defer to Raf and Peter on the rest.

http://codereview.chromium.org/454019/diff/7001/8004
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/454019/diff/7001/8004#newcode383
chrome/browser/views/browser_actions_container.cc:383: true);
nit: comment for bool args (multiple places)

[Jeff Timanus, 2009-12-07 18:22:00]

Added comments, as you suggest, Erik.

Please review again.

http://codereview.chromium.org/454019/diff/7001/8004
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/454019/diff/7001/8004#newcode383
chrome/browser/views/browser_actions_container.cc:383: true);
On 2009/12/05 15:38:23, Erik Kay wrote:
> nit: comment for bool args (multiple places)

Done.

[rafaelw, 2009-12-08 00:25:54]

Also, you removed the changes to WidgetWin, but I don't see any new code that
would accomplish the same thing. Was that not required?

http://codereview.chromium.org/454019/diff/8013/7003
File chrome/browser/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/454019/diff/8013/7003#newcode64
chrome/browser/views/extensions/extension_popup.cc:64:
border_widget_->MoveAbove(popup_);
Can you explain what's going on here? Why is the border_widget_ being moved
above the popup_?

[Jeff Timanus, 2009-12-08 01:05:38]

http://codereview.chromium.org/454019/diff/8013/7003
File chrome/browser/views/extensions/extension_popup.cc (right):

http://codereview.chromium.org/454019/diff/8013/7003#newcode64
chrome/browser/views/extensions/extension_popup.cc:64:
border_widget_->MoveAbove(popup_);
On 2009/12/08 00:25:54, rafaelw wrote:
> Can you explain what's going on here? Why is the border_widget_ being moved
> above the popup_?

Sorry for not making this clear.  The issue with the windows code is that
windows are ordered as a stack - first created, last in the order.  Because the
pop-up contents widget is created FIRST and then the border window, the border
window is actually ontop.

If we activate the window on show, then we bring it to the front, but if we
don't then the contents will be behind the popup.

This routine is poorly named - I had trouble with it until I realized that it
moves popup_ above border_widget_.  (!)

[rafaelw, 2009-12-08 02:24:35]

ok. thanks for the explanation. lgtm