gtk: Skip over minimized top-level windows when finding a window to dock to. ...

gtk: Fix a typo that breaks tab dragging when another gtk window is minimized.

BUG=20228, 20513
TEST=Extensive tab dragging.

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

[Evan Martin, 2009-08-31 21:03:39]

LGTM

http://codereview.chromium.org/182034/diff/1/2
File chrome/browser/dock_info_gtk.cc (right):

http://codereview.chromium.org/182034/diff/1/2#newcode70
Line 70: return false;
maybe delete this doubled line while you're at it?
maybe that's the cause of this bug?

http://codereview.chromium.org/182034/diff/1/4
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/182034/diff/1/4#newcode151
Line 151: Atom net_wm_state_atom = XInternAtom(GetXDisplay(), "_NET_WM_STATE",
True);
can you use "static Atom ... = ..." here?  it's a round-trip to the x server
each time this runs but we only need it once.

http://codereview.chromium.org/182034/diff/1/4#newcode162
Line 162: ~0L,                // Max length to get.
Doesn't this mean it can return 4GB?  I guess that's ok.

http://codereview.chromium.org/182034/diff/1/4#newcode173
Line 173: Atom hidden_atom = XInternAtom(GetXDisplay(), "_NET_WM_STATE_HIDDEN",
True);
same "static" suggestion here.

http://codereview.chromium.org/182034/diff/1/4#newcode178
Line 178: minimized = true;
i guess you could break; here

http://codereview.chromium.org/182034/diff/1/5
File chrome/common/x11_util.h (right):

http://codereview.chromium.org/182034/diff/1/5#newcode14
Line 14: #include <string>
this looks unnecessary

[Daniel Erat, 2009-08-31 21:08:11]

http://codereview.chromium.org/182034/diff/1/4
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/182034/diff/1/4#newcode151
Line 151: Atom net_wm_state_atom = XInternAtom(GetXDisplay(), "_NET_WM_STATE",
True);
Please cache this so we don't need to make an extra round trip to the server
each time this is called.

Also, you should pass False for the 'only_if_exists' param, not True (i.e. you
want to create the atom if it's not present; otherwise you can get a BadAtom
error in the call to XGetWindowProperty()).

http://codereview.chromium.org/182034/diff/1/4#newcode172
Line 172: if (type != None) {
Check that 'format' is 32 as well.

http://codereview.chromium.org/182034/diff/1/4#newcode173
Line 173: Atom hidden_atom = XInternAtom(GetXDisplay(), "_NET_WM_STATE_HIDDEN",
True);
Cache this too and tell the server to create it if it doesn't exist (otherwise
you'll get None and use that for the comparison in the for loop).

http://codereview.chromium.org/182034/diff/1/4#newcode178
Line 178: minimized = true;
I think the code is easier to read if you break here instead of including
!minimized in the loop condition.

[Evan Martin, 2009-08-31 21:11:39]

http://codereview.chromium.org/182034/diff/1/2
File chrome/browser/dock_info_gtk.cc (right):

http://codereview.chromium.org/182034/diff/1/2#newcode70
Line 70: return false;
On 2009/08/31 21:03:39, Evan Martin wrote:
> maybe delete this doubled line while you're at it?
> maybe that's the cause of this bug?

... i swear this line was doubled across L70 and 71 when i looked before.  maybe
a rietveld glitch.  :(

[Daniel Erat, 2009-08-31 21:14:35]

http://codereview.chromium.org/182034/diff/1/2
File chrome/browser/dock_info_gtk.cc (right):

http://codereview.chromium.org/182034/diff/1/2#newcode83
Line 83: if (x11_util::IsWindowMinimized(window)) {
Higher-level comment: I wouldn't expect this to be necessary.  If the window is
minimized it should either be unmapped (and hence fail the IsWindowVisible()
check) or offscreen (and fail the rect.Contains() check).  What's the reason for
checking the EWMH property?  Should the 'window == target_' check be moved after
the other two checks?

[James Hawkins, 2009-08-31 21:31:19]

On 2009/08/31 21:14:35, Daniel Erat wrote:
> http://codereview.chromium.org/182034/diff/1/2
> File chrome/browser/dock_info_gtk.cc (right):
> 
> http://codereview.chromium.org/182034/diff/1/2#newcode83
> Line 83: if (x11_util::IsWindowMinimized(window)) {
> Higher-level comment: I wouldn't expect this to be necessary.  If the window
is
> minimized it should either be unmapped (and hence fail the IsWindowVisible()
> check) or offscreen (and fail the rect.Contains() check).  What's the reason
for
> checking the EWMH property?  Should the 'window == target_' check be moved
after
> the other two checks?

Spot on.  There was a typo in the check for a visible window.  Patch set 2 fixes
this.

[use derat at chromium.org, 2009-08-31 22:07:19]

On Mon, Aug 31, 2009 at 2:31 PM, <jhawkins@chromium.org> wrote:
> Spot on. =A0There was a typo in the check for a visible window. =A0Patch =
set
> 2 fixes this.

Ah, yikes.  Nice catch.  But I'm still not sure why the test for
window =3D=3D target_ comes before the other two tests in the method.
Actually, I'm kind of confused by more stuff now. :-)

I'm guessing that:

- DockInfo::GetLocalProcessWindowAtPoint() is invoked from the outside
- LocalProcessWindowFinder::GetProcessWindowAtPoint() is called
- it creates a LocalProcessWindowFinder is created, which calls
gtk_util::EnumerateTopLevelWindows() using
LocalProcessWindowFinder::ShouldStopIterating() as a predicate to stop
iterating
- LocalProcessWindowFinder::ShouldStopIterating() checks that the
window is in-process, visible, and under the pointer
- then GetProcessWindowAtPoint() passes its result to
TopMostFinder::IsTopMostWindowAtPoint(), which looks like it does the
same iterating again but with a subtly different predicate

What's the reason for TopMostFinder -- why isn't the iteration by
LocalProcessWindowFinder sufficient?

I also don't understand why dock_info_gtk.cc needs to check whether
the window is mapped; isn't gtk_util::EnumerateTopLevelWindows()
already doing this itself via a call to gdk_window_is_visible(), at
least when _NET_CLIENT_LIST_STACKING is available?  For symmetry,
x11_util::EnumerateChildren() should do the same thing using
IsWindowVisible() -- I'd think that the tests could be removed from
dock_info_gtk.cc then.

(Unrelated side note: x11_util::EnumerateChildren() looks like it's
doing an extra level of recursion that could be avoided -- it should
compare 'depth' to 'max_depth' before iterating through all of its
children at the end of the method.)

> http://codereview.chromium.org/182034
>

[James Hawkins, 2009-08-31 23:07:48]

On 2009/08/31 22:07:19, derat wrote:
> On Mon, Aug 31, 2009 at 2:31 PM, <mailto:jhawkins@chromium.org> wrote:
> > Spot on. =A0There was a typo in the check for a visible window. =A0Patch =
> set
> > 2 fixes this.
> 
> Ah, yikes.  Nice catch.  But I'm still not sure why the test for
> window =3D=3D target_ comes before the other two tests in the method.
> Actually, I'm kind of confused by more stuff now. :-)
> 
> I'm guessing that:
> 
> - DockInfo::GetLocalProcessWindowAtPoint() is invoked from the outside

Correct

> - LocalProcessWindowFinder::GetProcessWindowAtPoint() is called

Correct

> - it creates a LocalProcessWindowFinder is created, which calls
> gtk_util::EnumerateTopLevelWindows() using
> LocalProcessWindowFinder::ShouldStopIterating() as a predicate to stop
> iterating

Correct

> - LocalProcessWindowFinder::ShouldStopIterating() checks that the
> window is in-process, visible, and under the pointer

Correct

> - then GetProcessWindowAtPoint() passes its result to
> TopMostFinder::IsTopMostWindowAtPoint(), which looks like it does the
> same iterating again but with a subtly different predicate
> 

Correct

> What's the reason for TopMostFinder -- why isn't the iteration by
> LocalProcessWindowFinder sufficient?
> 

LocalProcessWindowFinder only finds the window in our process that is under the
pointer.  We use this explicitly when raising another browser window which the
pointer is over that is lower than the top-most browser window.  TopMostFinder
makes sure that the window we found is not obscured by any other top-level
window, whether it's in our process or not.  We can place the check for window
== target_ first because we already know (from LocalProcessWindowFinder) that
the window we're looking for is visible and the pointer is inside the window
bounds.

> I also don't understand why dock_info_gtk.cc needs to check whether
> the window is mapped; isn't gtk_util::EnumerateTopLevelWindows()
> already doing this itself via a call to gdk_window_is_visible(), at
> least when _NET_CLIENT_LIST_STACKING is available?  For symmetry,
> x11_util::EnumerateChildren() should do the same thing using
> IsWindowVisible() -- I'd think that the tests could be removed from
> dock_info_gtk.cc then.
> 

The call to gdk_window_is_visible is a vestige that I need to remove.  The
top-level window enumeration should plainly enumerate all top-level windows as
provided by either _NET_CLIENT_LIST_STACKING or our own window enumeration code,
whether the enumerated windows are visible or not.

> (Unrelated side note: x11_util::EnumerateChildren() looks like it's
> doing an extra level of recursion that could be avoided -- it should
> compare 'depth' to 'max_depth' before iterating through all of its
> children at the end of the method.)
> 

Ok, I'll make a second CL for this.

[use derat at chromium.org, 2009-08-31 23:17:36]

LGTM

On Mon, Aug 31, 2009 at 4:07 PM, <jhawkins@chromium.org> wrote:
> On 2009/08/31 22:07:19, derat wrote:
>> What's the reason for TopMostFinder -- why isn't the iteration by
>> LocalProcessWindowFinder sufficient?
>
> LocalProcessWindowFinder only finds the window in our process that is
> under the pointer. =A0We use this explicitly when raising another browser
> window which the pointer is over that is lower than the top-most browser
> window. =A0TopMostFinder makes sure that the window we found is not
> obscured by any other top-level window, whether it's in our process or
> not. =A0We can place the check for window =3D=3D target_ first because we
> already know (from LocalProcessWindowFinder) that the window we're
> looking for is visible and the pointer is inside the window bounds.

Ah.  I was about to ask why these couldn't be combined into a single
class, but I'm guessing that this is all done this way to match the
Windows code.

>
> http://codereview.chromium.org/182034
>