Panels not getting focus on Linux when expanded from API on Linux.

Panels not getting focus on Linux when expanded from API on Linux.

When panel goes to minimized state, we set the window to not accept
focus. We do this to prevent panel getting focus assigned by the OS in
minimized state. When panel gets expanded programmatically, we're not
changing the window state to be able to accept focus.

Fixed by having PanelBrowserWindowGtk listen to expansion state change
notification.

BUG=112030
TEST=Repro steps in bug.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120797

[jennb, 2012-02-01 00:31:47]

I wish there was a way to do this without watching for expansion state
notifications. Is there an equivalent of Mac's canBecomeKeyWindow() on GTK where
you decide 'on the spot' if the window can accept focus?

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:245:
gtk_widget_get_window(GTK_WIDGET(window())), accept_focus);
This also disables focus for OVERFLOW and TITLE_ONLY. That's different from
previous behavior - checking that's intentional...

[prasadt, 2012-02-01 00:37:47]

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:245:
gtk_widget_get_window(GTK_WIDGET(window())), accept_focus);
On 2012/02/01 00:31:47, jennb wrote:
> This also disables focus for OVERFLOW and TITLE_ONLY. That's different from
> previous behavior - checking that's intentional...

Yes, its intentional.

[prasadt, 2012-02-01 00:39:43]

On 2012/02/01 00:31:47, jennb wrote:
> I wish there was a way to do this without watching for expansion state
> notifications. Is there an equivalent of Mac's canBecomeKeyWindow() on GTK
where
> you decide 'on the spot' if the window can accept focus?

I looked for it before but couldn't find anything equivalent.

>
http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
> File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):
> 
>
http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
> chrome/browser/ui/panels/panel_browser_window_gtk.cc:245:
> gtk_widget_get_window(GTK_WIDGET(window())), accept_focus);
> This also disables focus for OVERFLOW and TITLE_ONLY. That's different from
> previous behavior - checking that's intentional...

[Dmitry Titov, 2012-02-01 01:30:57]

lgtm with a question.

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:247:
BrowserWindowGtk::Observe(type, source, details);
Having this method overridden looks dangerous. What if someone adds 'else if()'
in the future for the same type of notification that base class is expecting?
Don't know the better way out of my head... 

Maybe remove the NOTREACHED in base class and then remove 'else' here?

[prasadt, 2012-02-01 01:52:27]

estade - Please take a look at browser_window_gtk.cc changes.

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/9318018/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:247:
BrowserWindowGtk::Observe(type, source, details);
On 2012/02/01 01:30:57, Dmitry Titov wrote:
> Having this method overridden looks dangerous. What if someone adds 'else
if()'
> in the future for the same type of notification that base class is expecting?
> Don't know the better way out of my head... 
> 
> Maybe remove the NOTREACHED in base class and then remove 'else' here?

Done. Makes sense now that the method is overridden in a derived class.

[Dmitry Titov, 2012-02-01 21:55:24]

lgtm++

[Evan Stade, 2012-02-01 22:19:02]

http://codereview.chromium.org/9318018/diff/6001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_window_gtk.cc (left):

http://codereview.chromium.org/9318018/diff/6001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_window_gtk.cc:1267: default:
I don't understand why you are deleting this check. What notification type is
hitting the NOTREACHED?

[prasadt, 2012-02-01 22:40:30]

http://codereview.chromium.org/9318018/diff/6001/chrome/browser/ui/gtk/browse...
File chrome/browser/ui/gtk/browser_window_gtk.cc (left):

http://codereview.chromium.org/9318018/diff/6001/chrome/browser/ui/gtk/browse...
chrome/browser/ui/gtk/browser_window_gtk.cc:1267: default:
On 2012/02/01 22:19:02, Evan Stade wrote:
> I don't understand why you are deleting this check. What notification type is
> hitting the NOTREACHED?

This method is now overridden in PanelBrowserWindowGtk class, part of this
patch. Now chrome::NOTIFICATION_PANEL_CHANGED_EXPANSION_STATE will also get
here. There is one more notification that I'll be adding in another patch
that'll also now reach here.

[Evan Stade, 2012-02-02 23:10:49]

instead you should return after handling the notification

[prasadt, 2012-02-02 23:22:23]

On 2012/02/02 23:10:49, Evan Stade wrote:
> instead you should return after handling the notification

That's what I was doing in my first patch. I changed that following Dmitry's
comment. Take a look at the first patch, Dmitry's comments and my response.

[prasadt, 2012-02-03 23:40:43]

On 2012/02/02 23:22:23, prasadt wrote:
> On 2012/02/02 23:10:49, Evan Stade wrote:
> > instead you should return after handling the notification
> 
> That's what I was doing in my first patch. I changed that following Dmitry's
> comment. Take a look at the first patch, Dmitry's comments and my response.

ping?

[Evan Stade, 2012-02-06 20:27:08]

if (notification_type == PANEL_FOO_BAR) {
  foo();
  return;
} else if (notification_type ==
GENERIC_FOO_BAR_THAT_BROWSER_WINDOW_GTK_ALSO_REGISTERED_FOR) {
  bar();
  // no return
}

BrowserWindowGtk::Observe(...);

[prasadt, 2012-02-06 21:06:06]

On 2012/02/06 20:27:08, Evan Stade wrote:
> if (notification_type == PANEL_FOO_BAR) {
>   foo();
>   return;
> } else if (notification_type ==
> GENERIC_FOO_BAR_THAT_BROWSER_WINDOW_GTK_ALSO_REGISTERED_FOR) {
>   bar();
>   // no return
> }
> 
> BrowserWindowGtk::Observe(...);

That works. But that'd be coupling the base and derived class implementations
too closely. Anytime you add a new notification to
PanelBrowserWindowGtk::Observe, you'll need to refer to the base class
implementation to see if it should be propagated or not. Similarly if you add a
new notification to BrowserWindowGtk::Observe, you'll need to check
PanelBrowserWindowGtk::Observe to make sure that its not being eaten by the
derived class. That feels broken to me.

[Evan Stade, 2012-02-07 17:29:31]

On 2012/02/06 21:06:06, prasadt wrote:
> On 2012/02/06 20:27:08, Evan Stade wrote:
> > if (notification_type == PANEL_FOO_BAR) {
> >   foo();
> >   return;
> > } else if (notification_type ==
> > GENERIC_FOO_BAR_THAT_BROWSER_WINDOW_GTK_ALSO_REGISTERED_FOR) {
> >   bar();
> >   // no return
> > }
> > 
> > BrowserWindowGtk::Observe(...);
> 
> That works. But that'd be coupling the base and derived class implementations
> too closely. Anytime you add a new notification to
> PanelBrowserWindowGtk::Observe, you'll need to refer to the base class
> implementation to see if it should be propagated or not.

well, yea, derived classes do tend to depend on their parents.

> Similarly if you add a
> new notification to BrowserWindowGtk::Observe, you'll need to check
> PanelBrowserWindowGtk::Observe to make sure that its not being eaten by the
> derived class. That feels broken to me.

The code change would not be in BrowserWindowGtk, it would be adding/removing a
return in PanelBrowserWindowGtk, so this is actually another example where the
derived class has to have knowledge of the workings of the base class, which is
entirely reasonable. In fact with the patch you have, you're not really dodging
the situation you just described: if BrowserWindowGtk suddenly starts observing
a new notification which PanelBrowserWindowGtk already uses, it seems equally
likely that the new behavior will somehow break PanelBrowserWindowGtk.

but I guess I don't really care that much, so lgtm.