DesktopWindowTreeHostX11::GetWindowBoundsInScreen returns window bounds.

DesktopWindowTreeHostX11::GetWindowBoundsInScreen returns window bounds.

Previously it returned the client bounds. It now queries the X
properties of the window to extend the client bounds by the border size.

BUG=357533

[Matt Giuca, 2014-03-28 10:44:57]

Hi Elliot,

As described by the bug, I caught this issue today trying to land my native app
windows patch.

I have discussed it with benwells and he agreed we could land the native app
windows patch without this fix (we will have slightly broken outerBounds
behaviour). So this should be treated as non-urgent. But it might still be nice
to have it in M35.

It all depends on how scary you think it is. If it's just straightforward "the
old behaviour was wrong, this is right" or if there are more nuances that I'm
not aware of.

Thanks

Matt

[Elliot Glaysher, 2014-03-28 17:44:55]

First, this patch is correct.

That said, this patch really *is* scary. XGetGeometry() and
XTranslateCoordinates() block the world waiting for a response from the X
server. This could be a performance regression if this is called in a loop.

[Matt Giuca, 2014-03-28 21:04:30]

On 2014/03/28 17:44:55, Elliot Glaysher wrote:
> First, this patch is correct.
> 
> That said, this patch really *is* scary. XGetGeometry() and
> XTranslateCoordinates() block the world waiting for a response from the X
> server. This could be a performance regression if this is called in a loop.

Ok, let's hold off for now until we can investigate. As I said earlier, I will
be on vacation for a week, so feel free to take this if you want to try and
merge it. Otherwise I'll look into it when I get back.

[Matt Giuca, 2014-04-08 01:57:22]

> That said, this patch really *is* scary. XGetGeometry() and
> XTranslateCoordinates() block the world waiting for a response from the X
> server. This could be a performance regression if this is called in a loop.

I'm assuming that XGetWindowProperty also blocks the world? If it doesn't, then
we have no problem... we can just use the existing known bounds_, combined with
the current value of _NET_FRAME_EXTENTS which we get from the window properties.
If that also blocks the world, then we'll probably want to cache the extents.

I have a vague plan to refactor x11_util as follows: GetWindowRect currently
returns the outer bounds by getting the inner bounds and extents (delta between
inner and outer), and insetting the inner bounds by the negative extents.

Break up the over-abstraction in x11_util -- add new functions
GetInnerWindowBounds and GetWindowExtents (returning a gfx::Insets), and
implement GetWindowRect in terms of these (possibly renaming to
GetOuterWindowBounds to clarify). Instead of calling GetWindowRect,
DesktopWindowTreeHostX11::GetWindowBoundsInScreen will directly use the known
bounds_ and GetWindowExtents. If necessary, we can cache the result of
GetWindowExtents (assuming that the extents do not change when the window size
changes). We'll have to listen for some kind of event that tells us when the
extents *do* change (i.e., the window manager theme changes) so we can
invalidate the cache.

Would that solve the performance concerns, or is it over-engineered? The two
issues are: a) is it still bad because the first call will block until a
response is received from the WM? and b) I don't know how we can reliably detect
when the extents change and invalidate the cache.

[Elliot Glaysher, 2014-04-08 22:04:36]

On 2014/04/08 01:57:22, Matt Giuca wrote:
> > That said, this patch really *is* scary. XGetGeometry() and
> > XTranslateCoordinates() block the world waiting for a response from the X
> > server. This could be a performance regression if this is called in a loop.
> 
> I'm assuming that XGetWindowProperty also blocks the world? If it doesn't,
then
> we have no problem... we can just use the existing known bounds_, combined
with
> the current value of _NET_FRAME_EXTENTS which we get from the window
properties.
> If that also blocks the world, then we'll probably want to cache the extents.

I believe so, but I'm not 100% sure.
 
> I have a vague plan to refactor x11_util as follows: GetWindowRect currently
> returns the outer bounds by getting the inner bounds and extents (delta
between
> inner and outer), and insetting the inner bounds by the negative extents.
> 
> Break up the over-abstraction in x11_util -- add new functions
> GetInnerWindowBounds and GetWindowExtents (returning a gfx::Insets), and
> implement GetWindowRect in terms of these (possibly renaming to
> GetOuterWindowBounds to clarify). Instead of calling GetWindowRect,
> DesktopWindowTreeHostX11::GetWindowBoundsInScreen will directly use the known
> bounds_ and GetWindowExtents. If necessary, we can cache the result of
> GetWindowExtents (assuming that the extents do not change when the window size
> changes). We'll have to listen for some kind of event that tells us when the
> extents *do* change (i.e., the window manager theme changes) so we can
> invalidate the cache.
> 
> Would that solve the performance concerns, or is it over-engineered? The two
> issues are: a) is it still bad because the first call will block until a
> response is received from the WM? and b) I don't know how we can reliably
detect
> when the extents change and invalidate the cache.

I am unsure about the details. I think you might want to talk to sadrul@ about
this, or possible varkha@ (since he mentioned accelerating this by making a
local cache that listened for all window change events on windows).

[Matt Giuca, 2014-04-08 23:45:37]

sadrul, varkha: Do you have any opinions about the above plan before I implement
it?

[varkha, 2014-04-09 00:35:23]

pkotwicz@ was planning to implement the system-wide caching of X windows bounds
and stacking order to allow faster response to drag and drop. We are not
treating this as very urgent since the current performance is probably almost
acceptable.
This cache will need to be system-wide and not just for chrome windows since
drop target may be some other window and we don't want to block on enumerating
and filtering windows making multitude of X calls on every motion event. We
alleviate this somewhat now by coalescing the motion events and dispatching only
the latest event so we don't enumerate windows so often.
This CL is *always* about a chrome window so caching should be much simpler
since we are already monitoring configure notifications. All we will need is as
you say, to watch for the WM changing frame insets. I am not sure if all WMs
guarantee that the frame extents are same for all the windows (could they vary
from window to window or from desktop to desktop?) but maybe we can watch for
something on each window.
As-is I agree that this seems a bit scary from performance point of view, it
would be nice to understand which if any scenarios call GetWindowBoundsInScreen
frequently or in a loop before committing to this plan.

[sadrul, 2014-04-09 00:56:38]

For this particular case, can we keep track of the frame-extents in addition to
the bounds? We would need to update it when we receive a PropertyNotify event
for _NET_FRAME_EXTENTS. The frame-extents shouldn't change frequently enough for
this to cause a perf-issue.

[pkotwicz, 2014-04-09 04:32:02]

A couple of things to maybe think about:
- If we want to merge with M-35, I suggest hacking something which is as close
to the app code which uses it as possible.
  (Something ugly similar to https://codereview.chromium.org/197283008)

Thank you very much for looking into a proper fix
- My main concern is to make sure that we return the outer bounds in the proper
places and the inner bounds in the proper places
  A couple methods we need to figure out if they should take in the outer or
inner bounds:
  DesktopWindowTreeHostX11::SetBounds()
  DesktopWindowTreeHostX11::GetBounds()
  there's unfortunately others too

[Matt Giuca, 2014-04-09 05:54:01]

On 2014/04/09 04:32:02, pkotwicz wrote:
> A couple of things to maybe think about:
> - If we want to merge with M-35, I suggest hacking something which is as close
> to the app code which uses it as possible.
>   (Something ugly similar to https://codereview.chromium.org/197283008)

Right. I don't think merging with M35 is strictly necessary. The original bug
we're trying to fix is the return value of chrome.app.window.outerBounds and
innerBounds, which are not available in M35 stable. It will also affect the old
chrome.app.window.bounds, but that function was already broken in M34.
(Basically it was fixed in M35 Aura, and me switching to native windows broke it
again, but it isn't a regression compared to M34.)

> 
> Thank you very much for looking into a proper fix
> - My main concern is to make sure that we return the outer bounds in the
proper
> places and the inner bounds in the proper places
>   A couple methods we need to figure out if they should take in the outer or
> inner bounds:
>   DesktopWindowTreeHostX11::SetBounds()
>   DesktopWindowTreeHostX11::GetBounds()
>   there's unfortunately others too

I think we should wherever possible rename such methods to use the terms
"InnerBounds" or "OuterBounds" where appropriate. It is currently very confusing
to figure out which one any given method is talking about, requiring close
inspection of the code and often transitive dependencies.

[benwells, 2014-04-09 16:22:36]

On Wed, Apr 9, 2014 at 3:54 PM, <mgiuca@chromium.org> wrote:

> On 2014/04/09 04:32:02, pkotwicz wrote:
>
>> A couple of things to maybe think about:
>> - If we want to merge with M-35, I suggest hacking something which is as
>> close
>> to the app code which uses it as possible.
>>    (Something ugly similar to https://codereview.chromium.org/197283008)
>>
>
> Right. I don't think merging with M35 is strictly necessary. The original
> bug
> we're trying to fix is the return value of chrome.app.window.outerBounds
> and
> innerBounds, which are not available in M35 stable. It will also affect
> the old
> chrome.app.window.bounds, but that function was already broken in M34.
> (Basically it was fixed in M35 Aura, and me switching to native windows
> broke it
> again, but it isn't a regression compared to M34.)
>
>
>
>  Thank you very much for looking into a proper fix
>> - My main concern is to make sure that we return the outer bounds in the
>>
> proper
>
>> places and the inner bounds in the proper places
>>    A couple methods we need to figure out if they should take in the
>> outer or
>> inner bounds:
>>    DesktopWindowTreeHostX11::SetBounds()
>>    DesktopWindowTreeHostX11::GetBounds()
>>    there's unfortunately others too
>>
>
> I think we should wherever possible rename such methods to use the terms
> "InnerBounds" or "OuterBounds" where appropriate. It is currently very
> confusing
> to figure out which one any given method is talking about, requiring close
> inspection of the code and often transitive dependencies.
>

+1 we have had many bugs in the app window API due to this. It is super
confusing.


>
> https://codereview.chromium.org/216763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2017-08-28 01:48:40]

Description was changed from

==========
DesktopWindowTreeHostX11::GetWindowBoundsInScreen returns window bounds.

Previously it returned the client bounds. It now queries the X
properties of the window to extend the client bounds by the border size.

BUG=357533
==========

to

==========
DesktopWindowTreeHostX11::GetWindowBoundsInScreen returns window bounds.

Previously it returned the client bounds. It now queries the X
properties of the window to extend the client bounds by the border size.

BUG=357533
==========