Have panels respond to changes in work area on Linux.

Have panels respond to changes in work area on Linux. This includes switching taskbar to/from auto-hide as well as changes to screen resolution.

- Factored out the part of code that listens to X property changes from
  ActiveWindowWatcherX into a separate class.
- Implemented a watcher for work area changes.
- Add hook-up for PanelManager to respond to the changes.
- This change also handles user switching between auto-hide vs non auto-hide mode of taskbar.

TEST=Manual. Create panels. Switch to auto-hide and see them move them move.
BUG=102719
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111171

[prasadt, 2011-11-18 04:21:49]

dimich - Please do a overall review.
sky - Please review change under ui/base/x. You're listed as one of the two
owners.

Thanks,
Prasad

[Dmitry Titov, 2011-11-18 04:56:37]

Does it also help with screen resolution change? If Yes, please add to
description.

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

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:97: // Strictly speaking we
should have a single observer for work area changed
Not sure that long comment has to be here. If it's not a TODO, I don't see what
it adds. Remove?

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

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.h:29: public
ui::WorkAreaWatcherX::Observer,
If these are in alphabetic order, then last two should be swapped.

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.h:46: //
ui::WorkAreaWatcherX::Observer overrides
overrides -> override?

[sky, 2011-11-18 17:54:22]

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.cc:8: #include
"ui/base/x/active_window_watcher_x.h"
This include should be first, then system includes.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:35: static Atom GetPropertyAtom();
Add a description.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:39: static void Notify();
This doesn't notify through the NOtificationService.

I don't like exposing this as public as folks shouldn't be using it directly.
Maybe RootWindowPropertyWatcherX should notify via an interface that this class
(and WorkAreaWatcherX) implement. That way RootWindowPropertyWatcherX doesn't
need to know about this class (or WorkAreaWatcherX).

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:48: ActiveWindowWatcherX() {}
Don't inline these.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
File ui/base/x/root_window_property_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.cc:8: #include
"ui/base/x/root_window_property_watcher_x.h"
This include should be first, then system includes.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
File ui/base/x/root_window_property_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.h:15: namespace ui {
Put this in an internal namespace (ui::internal) if it's not to be used
externally. Also, you shouldn't need need UI_EXPORT if the class is only used in
ui.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.h:17: // This class keeps track of
changes to properties on root window. This is not
'on root' -> 'on the root'

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:20: class Observer {
Move this into it's own header.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:40: WorkAreaWatcherX() { }
Don't inline these.

[sadrul, 2011-11-18 17:56:42]

drive-by

http://codereview.chromium.org/8595003/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/x11_util.cc#newcode814
ui/base/x/x11_util.cc:814: return gdk_x11_atom_to_xatom_for_display(
Please use non-gtk functions (e.g. XInternAtom)

[prasadt, 2011-11-18 22:31:42]

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

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:97: // Strictly speaking we
should have a single observer for work area changed
On 2011/11/18 04:56:37, Dmitry Titov wrote:
> Not sure that long comment has to be here. If it's not a TODO, I don't see
what
> it adds. Remove?

Just a clarification in case someone wonders why its done this way. Removed it
as it doesn't seem to help.

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

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.h:29: public
ui::WorkAreaWatcherX::Observer,
On 2011/11/18 04:56:37, Dmitry Titov wrote:
> If these are in alphabetic order, then last two should be swapped.

Moved things around to keep them in order.

I was not really going for any order. I guess I was just keeping the two
Observers together. I didn't think there was any convention for this.

http://codereview.chromium.org/8595003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_window_gtk.h:46: //
ui::WorkAreaWatcherX::Observer overrides
On 2011/11/18 04:56:37, Dmitry Titov wrote:
> overrides -> override?

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.cc:8: #include
"ui/base/x/active_window_watcher_x.h"
On 2011/11/18 17:54:22, sky wrote:
> This include should be first, then system includes.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:35: static Atom GetPropertyAtom();
On 2011/11/18 17:54:22, sky wrote:
> Add a description.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:39: static void Notify();
On 2011/11/18 17:54:22, sky wrote:
> This doesn't notify through the NOtificationService.
> 
> I don't like exposing this as public as folks shouldn't be using it directly.
> Maybe RootWindowPropertyWatcherX should notify via an interface that this
class
> (and WorkAreaWatcherX) implement. That way RootWindowPropertyWatcherX doesn't
> need to know about this class (or WorkAreaWatcherX).

I did consider that option. Didn't do it for a few reasons:
1) That would entail ActiveWindowWatcherX and WorkAreaWatcherX classes
registering with RootWindowPropertyWatcherX to be notified for their specific
properties and RootWindowPropertyWatcherX maintaining a map to associate each
property with a specific watcher. Seemed unnecessarily heavy weight to me
considering we're only interested in two properties at this time.
2) If I defined that interface, it'd be strange not to add AddObserver and
RemoveObserver methods to that interface. That'd make them instance methods and
will mean changing the call sites for ActiveWindowWatcherX.
3) And adding this interface won't really make Notify() private any way, as
RootWindowPropertyWatcherX will still need to be able to call it.

The other option I considered is making RootWindowPropertyWatcherX a friend of
ActiveWindowWatcherX and WorkAreaWatcherX. That doesn't solve the issue of
RootWindowPropertyWatcherX knowing about these other classes but will keep
Notify private. I just avoided it on principle but can do that if Notify being
public is the big issue.

I settled on what I got right now because its simple and adequate for the
current requirements. Notify() even though not desirable to be public, its not
private either as RootWindowPropertyWatcherX needs to be able to call it.

Let me know what you think.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:48: ActiveWindowWatcherX() {}
On 2011/11/18 17:54:22, sky wrote:
> Don't inline these.

Done.

Curious though why this is discouraged.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
File ui/base/x/root_window_property_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.cc:8: #include
"ui/base/x/root_window_property_watcher_x.h"
On 2011/11/18 17:54:22, sky wrote:
> This include should be first, then system includes.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
File ui/base/x/root_window_property_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.h:15: namespace ui {
On 2011/11/18 17:54:22, sky wrote:
> Put this in an internal namespace (ui::internal) if it's not to be used
> externally. Also, you shouldn't need need UI_EXPORT if the class is only used
in
> ui.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/root_window_property_...
ui/base/x/root_window_property_watcher_x.h:17: // This class keeps track of
changes to properties on root window. This is not
On 2011/11/18 17:54:22, sky wrote:
> 'on root' -> 'on the root'

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:20: class Observer {
On 2011/11/18 17:54:22, sky wrote:
> Move this into it's own header.

I see nested classes being in the same header as a common pattern in code. Any
reason why it is different in this case?

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:40: WorkAreaWatcherX() { }
On 2011/11/18 17:54:22, sky wrote:
> Don't inline these.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/x11_util.cc#newcode814
ui/base/x/x11_util.cc:814: return gdk_x11_atom_to_xatom_for_display(
On 2011/11/18 17:56:42, sadrul wrote:
> Please use non-gtk functions (e.g. XInternAtom)

Done.

As it turns, there is an existing helper function that I can use.

[sky, 2011-11-18 22:44:58]

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:39: static void Notify();
On 2011/11/18 22:31:42, prasadt wrote:
> On 2011/11/18 17:54:22, sky wrote:
> > This doesn't notify through the NOtificationService.
> > 
> > I don't like exposing this as public as folks shouldn't be using it
directly.
> > Maybe RootWindowPropertyWatcherX should notify via an interface that this
> class
> > (and WorkAreaWatcherX) implement. That way RootWindowPropertyWatcherX
doesn't
> > need to know about this class (or WorkAreaWatcherX).
> 
> I did consider that option. Didn't do it for a few reasons:
> 1) That would entail ActiveWindowWatcherX and WorkAreaWatcherX classes
> registering with RootWindowPropertyWatcherX to be notified for their specific
> properties and RootWindowPropertyWatcherX maintaining a map to associate each
> property with a specific watcher. Seemed unnecessarily heavy weight to me
> considering we're only interested in two properties at this time.
> 2) If I defined that interface, it'd be strange not to add AddObserver and
> RemoveObserver methods to that interface. That'd make them instance methods
and
> will mean changing the call sites for ActiveWindowWatcherX.
> 3) And adding this interface won't really make Notify() private any way, as
> RootWindowPropertyWatcherX will still need to be able to call it.
> 
> The other option I considered is making RootWindowPropertyWatcherX a friend of
> ActiveWindowWatcherX and WorkAreaWatcherX. That doesn't solve the issue of
> RootWindowPropertyWatcherX knowing about these other classes but will keep
> Notify private. I just avoided it on principle but can do that if Notify being
> public is the big issue.
> 
> I settled on what I got right now because its simple and adequate for the
> current requirements. Notify() even though not desirable to be public, its not
> private either as RootWindowPropertyWatcherX needs to be able to call it.
> 
> Let me know what you think.

I'm ok with it as long as you make RootWindowPropertyWatcherX a friend of these
classes so Notify can be private.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:48: ActiveWindowWatcherX() {}
On 2011/11/18 22:31:42, prasadt wrote:
> On 2011/11/18 17:54:22, sky wrote:
> > Don't inline these.
> 
> Done.
> 
> Curious though why this is discouraged.

See threads from erg on this. Your class is probably fine since it doesn't have
much state, but there is nothing worse than landing a change a breaking a clang
bot because some change caused the threshold to be exceeded.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:20: class Observer {
On 2011/11/18 22:31:42, prasadt wrote:
> On 2011/11/18 17:54:22, sky wrote:
> > Move this into it's own header.
> 
> I see nested classes being in the same header as a common pattern in code. Any
> reason why it is different in this case?

From the style guide: 'Prefer putting delegate classes in their own header
files. Implementors of the delegate interface will often be included elsewhere,
which will often cause more coupling with the header of the main class.' We need
to update more classes to follow this pattern.

[sky, 2011-11-18 22:46:39]

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:48: ActiveWindowWatcherX() {}
On 2011/11/18 22:44:58, sky wrote:
> On 2011/11/18 22:31:42, prasadt wrote:
> > On 2011/11/18 17:54:22, sky wrote:
> > > Don't inline these.
> > 
> > Done.
> > 
> > Curious though why this is discouraged.
> 
> See threads from erg on this. Your class is probably fine since it doesn't
have
> much state, but there is nothing worse than landing a change a breaking a
clang
> bot because some change caused the threshold to be exceeded.

Ha. I was just asked to review a change that caused such bustage. See
http://codereview.chromium.org/8558010/ .

[dcheng, 2011-11-18 23:02:29]

Drive by.

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
File ui/base/x/active_window_watcher_x.h (left):

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
ui/base/x/active_window_watcher_x.h:11: #include "base/basictypes.h"
This should remain for DISALLOW_COPY_AND_ASSIGN. This applies to all the other
.h files as well.

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
ui/base/x/active_window_watcher_x.h:36: // Gets the atom for the default display
for the propert this class is
property

[prasadt, 2011-11-21 23:36:49]

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/active_window_watcher...
ui/base/x/active_window_watcher_x.h:39: static void Notify();
On 2011/11/18 22:44:58, sky wrote:
> On 2011/11/18 22:31:42, prasadt wrote:
> > On 2011/11/18 17:54:22, sky wrote:
> > > This doesn't notify through the NOtificationService.
> > > 
> > > I don't like exposing this as public as folks shouldn't be using it
> directly.
> > > Maybe RootWindowPropertyWatcherX should notify via an interface that this
> > class
> > > (and WorkAreaWatcherX) implement. That way RootWindowPropertyWatcherX
> doesn't
> > > need to know about this class (or WorkAreaWatcherX).
> > 
> > I did consider that option. Didn't do it for a few reasons:
> > 1) That would entail ActiveWindowWatcherX and WorkAreaWatcherX classes
> > registering with RootWindowPropertyWatcherX to be notified for their
specific
> > properties and RootWindowPropertyWatcherX maintaining a map to associate
each
> > property with a specific watcher. Seemed unnecessarily heavy weight to me
> > considering we're only interested in two properties at this time.
> > 2) If I defined that interface, it'd be strange not to add AddObserver and
> > RemoveObserver methods to that interface. That'd make them instance methods
> and
> > will mean changing the call sites for ActiveWindowWatcherX.
> > 3) And adding this interface won't really make Notify() private any way, as
> > RootWindowPropertyWatcherX will still need to be able to call it.
> > 
> > The other option I considered is making RootWindowPropertyWatcherX a friend
of
> > ActiveWindowWatcherX and WorkAreaWatcherX. That doesn't solve the issue of
> > RootWindowPropertyWatcherX knowing about these other classes but will keep
> > Notify private. I just avoided it on principle but can do that if Notify
being
> > public is the big issue.
> > 
> > I settled on what I got right now because its simple and adequate for the
> > current requirements. Notify() even though not desirable to be public, its
not
> > private either as RootWindowPropertyWatcherX needs to be able to call it.
> > 
> > Let me know what you think.
> 
> I'm ok with it as long as you make RootWindowPropertyWatcherX a friend of
these
> classes so Notify can be private.

Done.

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/1/ui/base/x/work_area_watcher_x.h...
ui/base/x/work_area_watcher_x.h:20: class Observer {
On 2011/11/18 22:44:58, sky wrote:
> On 2011/11/18 22:31:42, prasadt wrote:
> > On 2011/11/18 17:54:22, sky wrote:
> > > Move this into it's own header.
> > 
> > I see nested classes being in the same header as a common pattern in code.
Any
> > reason why it is different in this case?
> 
> From the style guide: 'Prefer putting delegate classes in their own header
> files. Implementors of the delegate interface will often be included
elsewhere,
> which will often cause more coupling with the header of the main class.' We
need
> to update more classes to follow this pattern.

Done.

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
File ui/base/x/active_window_watcher_x.h (left):

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
ui/base/x/active_window_watcher_x.h:11: #include "base/basictypes.h"
On 2011/11/18 23:02:29, dcheng wrote:
> This should remain for DISALLOW_COPY_AND_ASSIGN. This applies to all the other
> .h files as well.

Done.

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/4004/ui/base/x/active_window_watc...
ui/base/x/active_window_watcher_x.h:36: // Gets the atom for the default display
for the propert this class is
On 2011/11/18 23:02:29, dcheng wrote:
> property

Done.

[Dmitry Titov, 2011-11-21 23:43:13]

lgtm for overall.

[sky, 2011-11-21 23:50:32]

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_titlebar.h:180: virtual void
ActiveWindowChanged(GdkWindow* active_window);
OVERRIDE

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.h:47: void WorkAreaChanged()
OVERRIDE;
virtual

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:20: 
nit: generally we don't use newlines for namespaces forward declarations like
this.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:36: static Atom GetPropertyAtom();
Can this be private too?

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:49: ActiveWindowWatcherX();
constructor/destructor before other methods.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
File ui/base/x/active_window_watcher_x_observer.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x_observer.h:21: virtual
~ActiveWindowWatcherXObserver();
You can inline this. By that I mean put the {} right here. Inlining destructors
of pure virtual classes is one the exceptions to not inlining in headers.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.cc:13: static const char* kNetWorkArea =
"_NET_WORKAREA";
const char* const

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:20: 
nit: remove empty lines.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:35: static Atom GetPropertyAtom();
Can this be private?

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:44: WorkAreaWatcherX();
constructor/destructor should be before other methods.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:46: void NotifyWorkAreaChanged();
newline between 45 and 46 and add a descrption.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x_observer.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x_observer.h:18: virtual
~WorkAreaWatcherXObserver();
inline destructor.

http://codereview.chromium.org/8595003/diff/11001/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8595003/diff/11001/ui/ui.gyp#newcode561
ui/ui.gyp:561: 'base/x/active_window_watcher_x.cc',
Can we exclude the whole directory?

[prasadt, 2011-11-22 00:46:34]

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_titlebar.h (right):

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_titlebar.h:180: virtual void
ActiveWindowChanged(GdkWindow* active_window);
On 2011/11/21 23:50:32, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.h (right):

http://codereview.chromium.org/8595003/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.h:47: void WorkAreaChanged()
OVERRIDE;
On 2011/11/21 23:50:32, sky wrote:
> virtual

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
File ui/base/x/active_window_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:20: 
On 2011/11/21 23:50:32, sky wrote:
> nit: generally we don't use newlines for namespaces forward declarations like
> this.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:36: static Atom GetPropertyAtom();
On 2011/11/21 23:50:32, sky wrote:
> Can this be private too?

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x.h:49: ActiveWindowWatcherX();
On 2011/11/21 23:50:32, sky wrote:
> constructor/destructor before other methods.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
File ui/base/x/active_window_watcher_x_observer.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/active_window_wat...
ui/base/x/active_window_watcher_x_observer.h:21: virtual
~ActiveWindowWatcherXObserver();
On 2011/11/21 23:50:32, sky wrote:
> You can inline this. By that I mean put the {} right here. Inlining
destructors
> of pure virtual classes is one the exceptions to not inlining in headers.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x.cc (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.cc:13: static const char* kNetWorkArea =
"_NET_WORKAREA";
On 2011/11/21 23:50:32, sky wrote:
> const char* const

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:20: 
On 2011/11/21 23:50:32, sky wrote:
> nit: remove empty lines.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:35: static Atom GetPropertyAtom();
On 2011/11/21 23:50:32, sky wrote:
> Can this be private?

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:44: WorkAreaWatcherX();
On 2011/11/21 23:50:32, sky wrote:
> constructor/destructor should be before other methods.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x.h:46: void NotifyWorkAreaChanged();
On 2011/11/21 23:50:32, sky wrote:
> newline between 45 and 46 and add a descrption.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
File ui/base/x/work_area_watcher_x_observer.h (right):

http://codereview.chromium.org/8595003/diff/11001/ui/base/x/work_area_watcher...
ui/base/x/work_area_watcher_x_observer.h:18: virtual
~WorkAreaWatcherXObserver();
On 2011/11/21 23:50:32, sky wrote:
> inline destructor.

Done.

http://codereview.chromium.org/8595003/diff/11001/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/8595003/diff/11001/ui/ui.gyp#newcode561
ui/ui.gyp:561: 'base/x/active_window_watcher_x.cc',
On 2011/11/21 23:50:32, sky wrote:
> Can we exclude the whole directory?

Done.

[sky, 2011-11-22 00:55:17]

LGTM