Don't activate POPUP window

Don't activate POPUP window
 Clicking keyboard (and any popup like menu) was activating the widget, which
 hides VirtualKeyboard as it was stealing focus from browser window.

 This also moves the code that handles window activation to WindowManaer.

 Removed desktop_window_root_view.cc|h as it's no longer necesssary.

BUG=none
TEST=VirtualKeyboard on views desktop should work.

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

[oshima, 2011-08-25 22:55:53]

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc#newco...
views/widget/widget.cc:88: return can_activate_;
CanActivate is used only in native_widget_win.cc, and I'm not 100% certain if
this won't cause issue on windows. Please let me know if I should ifdef to non
windows.

[Ben Goodger (Google), 2011-08-25 23:12:44]

LGTM

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc#newco...
views/widget/widget.cc:88: return can_activate_;
Shouldn't do.

[sadrul, 2011-08-26 16:21:36]

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
File views/desktop/desktop_window_manager.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:163: ->OnKeyEvent(event) : false;
Could this just be 'active_widget_->OnKeyEvent(event)'?

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:200: void
DesktopWindowManager::Register(Widget* widget) {
Perhaps DCHECK(!widget->HasObserver(this))?

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:245: widget->AddObserver(this);
Perhaps just 'DCHECK(widget->HasObserver(this));' here? this will make sure all
windows are registered to the WM.

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc#newco...
views/widget/widget.cc:69: explicit DefaultWidgetDelegate(
'explicit' not needed.

[oshima, 2011-08-26 17:20:28]

sadrul PTAL

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
File views/desktop/desktop_window_manager.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:163: ->OnKeyEvent(event) : false;
On 2011/08/26 16:21:36, sadrul wrote:
> Could this just be 'active_widget_->OnKeyEvent(event)'?

We need to send to NativeViewViews as it needs to send key to InputMethod object
of the widget. It'd require some refactoring if we want to send it directly to
widget.

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:200: void
DesktopWindowManager::Register(Widget* widget) {
On 2011/08/26 16:21:36, sadrul wrote:
> Perhaps DCHECK(!widget->HasObserver(this))?

Done.

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc
File views/widget/widget.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc#newco...
views/widget/widget.cc:69: explicit DefaultWidgetDelegate(
On 2011/08/26 16:21:36, sadrul wrote:
> 'explicit' not needed.

Done.

http://codereview.chromium.org/7740039/diff/2001/views/widget/widget.cc#newco...
views/widget/widget.cc:88: return can_activate_;
On 2011/08/25 23:12:44, Ben Goodger (Google) wrote:
> Shouldn't do.

ok

[sadrul, 2011-08-26 18:41:05]

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
File views/desktop/desktop_window_manager.cc (right):

http://codereview.chromium.org/7740039/diff/2001/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:163: ->OnKeyEvent(event) : false;
On 2011/08/26 17:20:28, oshima wrote:
> On 2011/08/26 16:21:36, sadrul wrote:
> > Could this just be 'active_widget_->OnKeyEvent(event)'?
> 
> We need to send to NativeViewViews as it needs to send key to InputMethod
object
> of the widget. It'd require some refactoring if we want to send it directly to
> widget.

Ah. That seems a little unfortunate. Thanks for the explanation.

http://codereview.chromium.org/7740039/diff/1014/views/desktop/desktop_window...
File views/desktop/desktop_window_manager.cc (right):

http://codereview.chromium.org/7740039/diff/1014/views/desktop/desktop_window...
views/desktop/desktop_window_manager.cc:246: widget->AddObserver(this);
Perhaps just 'DCHECK(widget->HasObserver(this));' here? this will make sure all
windows are registered to the WM.

[sadrul, 2011-08-26 18:43:03]

Forgot LGTM