aura: Take transformation into account when computing window bounds in root-window/screen.

aura: Take transformation into account when computing window bounds in root-window/screen.

BUG=none

[sadrul, 2014-02-20 18:45:09]

The tests seem to be pretty happy about this change (except for some dock tests,
which is addressed in https://codereview.chromium.org/173843003/).

sky@ PTAL.

oshima@ Can you check if the change in the root_window_controller_unittest.cc
makes sense to you?

Thanks!

[oshima, 2014-02-20 19:39:34]

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
ash/root_window_controller_unittest.cc:156:
EXPECT_TRUE(minimized->GetWindowBoundsInScreen().IsEmpty());
Do you know why this becomes empty? AFAIK, the minimized window is simply hidden
and doesn't change the bounds or transform.

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
ui/aura/window.cc:393: transform.TransformRect(&rect);
While I understand the motivation and the current impl is also not accurate, I
wonder if this can cause problem in the place where we are using the value of
GetBounsdInRootWindow to later compute & set the new bounds to the same window.
(which will change the value of GetBoundsInRootWindow again)

[sadrul, 2014-02-20 19:59:58]

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
ash/root_window_controller_unittest.cc:156:
EXPECT_TRUE(minimized->GetWindowBoundsInScreen().IsEmpty());
On 2014/02/20 19:39:35, oshima wrote:
> Do you know why this becomes empty? AFAIK, the minimized window is simply
hidden
> and doesn't change the bounds or transform.

A transform is applied on he window on minimize, with scale = 0! That causes the
window bounds to be empty (I was pretty surprised when I noticed this).

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
ui/aura/window.cc:393: transform.TransformRect(&rect);
On 2014/02/20 19:39:35, oshima wrote:
> While I understand the motivation and the current impl is also not accurate, I
> wonder if this can cause problem in the place where we are using the value of
> GetBounsdInRootWindow to later compute & set the new bounds to the same
window.
> (which will change the value of GetBoundsInRootWindow again)

If there are such cases, perhaps we should find and fix them? (I expected more
tests to fail if that were the case, but the lack such failures makes me think
perhaps there are not many places that do this)

[oshima, 2014-02-20 20:18:51]

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ash/root_window_control...
ash/root_window_controller_unittest.cc:156:
EXPECT_TRUE(minimized->GetWindowBoundsInScreen().IsEmpty());
On 2014/02/20 19:59:59, sadrul wrote:
> On 2014/02/20 19:39:35, oshima wrote:
> > Do you know why this becomes empty? AFAIK, the minimized window is simply
> hidden
> > and doesn't change the bounds or transform.
> 
> A transform is applied on he window on minimize, with scale = 0! That causes
the
> window bounds to be empty (I was pretty surprised when I noticed this).

Ah, I think it's coming from minimize animation. Let me double check if we're
keeping that by design or something else.

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
ui/aura/window.cc:393: transform.TransformRect(&rect);
On 2014/02/20 19:59:59, sadrul wrote:
> On 2014/02/20 19:39:35, oshima wrote:
> > While I understand the motivation and the current impl is also not accurate,
I
> > wonder if this can cause problem in the place where we are using the value
of
> > GetBounsdInRootWindow to later compute & set the new bounds to the same
> window.
> > (which will change the value of GetBoundsInRootWindow again)
> 
> If there are such cases, perhaps we should find and fix them? (I expected more
> tests to fail if that were the case, but the lack such failures makes me think
> perhaps there are not many places that do this)

I think the reason is that such scenario is rare (and probably not important)
and not covered by tests.

I'm not against this change BTW. I was just wondering if there is better way to
deal with transform.

For example, the above case probably should use

GetBoundsInRootWindowNoTransform()

to compute the bounds. Or the ideal form of the following code

window->GetBoundsInRootWindow().Contains(poitn_in_root)

may be something like

window->ContainsPointInRoot(point_in_root);

[sadrul, 2014-02-20 21:53:48]

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
File ui/aura/window.cc (right):

https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
ui/aura/window.cc:393: transform.TransformRect(&rect);
On 2014/02/20 20:18:51, oshima wrote:
> On 2014/02/20 19:59:59, sadrul wrote:
> > On 2014/02/20 19:39:35, oshima wrote:
> > > While I understand the motivation and the current impl is also not
accurate,
> I
> > > wonder if this can cause problem in the place where we are using the value
> of
> > > GetBounsdInRootWindow to later compute & set the new bounds to the same
> > window.
> > > (which will change the value of GetBoundsInRootWindow again)
> > 
> > If there are such cases, perhaps we should find and fix them? (I expected
more
> > tests to fail if that were the case, but the lack such failures makes me
think
> > perhaps there are not many places that do this)
> 
> I think the reason is that such scenario is rare (and probably not important)
> and not covered by tests.
> 
> I'm not against this change BTW. I was just wondering if there is better way
to
> deal with transform.
> 
> For example, the above case probably should use
> 
> GetBoundsInRootWindowNoTransform()
> 
> to compute the bounds. Or the ideal form of the following code
> 
> window->GetBoundsInRootWindow().Contains(poitn_in_root)
> 
> may be something like
> 
> window->ContainsPointInRoot(point_in_root);
> 

Indeed. The motivation from this patch is from here:
https://codereview.chromium.org/167323004/#msg2 For the change in that patch, I
considered adding Window::GetTransformedBoundsInRootWindow(), but I decided
against adding yet another Get*Bounds*() function in the API so there's less
chance of someone using the wrong function for some quick fix. Instead, I think
it makes more sense to have GetBoundsInRootWindow/InScreen() take the transform
into account, and if some code expects the non-transformed bounds, then find
some alternate solution for those cases (like the one you mention above). This
may very well mean adding GetBoundsInRootWindowNoTransform() if it's needed in a
lot of places, but hopefully we can avoid that.

[sky, 2014-02-20 22:24:44]

Do we have a feel for which is more common? Wanting transformed or
untransformed? I don't have a good feel for all the callers to
GetBoundsInScreen.

  -Scott

On Thu, Feb 20, 2014 at 1:53 PM,  <sadrul@chromium.org> wrote:
>
> https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
> File ui/aura/window.cc (right):
>
>
https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
> ui/aura/window.cc:393: transform.TransformRect(&rect);
> On 2014/02/20 20:18:51, oshima wrote:
>>
>> On 2014/02/20 19:59:59, sadrul wrote:
>> > On 2014/02/20 19:39:35, oshima wrote:
>> > > While I understand the motivation and the current impl is also not
>
> accurate,
>>
>> I
>> > > wonder if this can cause problem in the place where we are using
>
> the value
>>
>> of
>> > > GetBounsdInRootWindow to later compute & set the new bounds to the
>
> same
>>
>> > window.
>> > > (which will change the value of GetBoundsInRootWindow again)
>> >
>> > If there are such cases, perhaps we should find and fix them? (I
>
> expected more
>>
>> > tests to fail if that were the case, but the lack such failures
>
> makes me think
>>
>> > perhaps there are not many places that do this)
>
>
>> I think the reason is that such scenario is rare (and probably not
>
> important)
>>
>> and not covered by tests.
>
>
>> I'm not against this change BTW. I was just wondering if there is
>
> better way to
>>
>> deal with transform.
>
>
>> For example, the above case probably should use
>
>
>> GetBoundsInRootWindowNoTransform()
>
>
>> to compute the bounds. Or the ideal form of the following code
>
>
>> window->GetBoundsInRootWindow().Contains(poitn_in_root)
>
>
>> may be something like
>
>
>> window->ContainsPointInRoot(point_in_root);
>
>
>
> Indeed. The motivation from this patch is from here:
> https://codereview.chromium.org/167323004/#msg2 For the change in that
> patch, I considered adding Window::GetTransformedBoundsInRootWindow(),
> but I decided against adding yet another Get*Bounds*() function in the
> API so there's less chance of someone using the wrong function for some
> quick fix. Instead, I think it makes more sense to have
> GetBoundsInRootWindow/InScreen() take the transform into account, and if
> some code expects the non-transformed bounds, then find some alternate
> solution for those cases (like the one you mention above). This may very
> well mean adding GetBoundsInRootWindowNoTransform() if it's needed in a
> lot of places, but hopefully we can avoid that.
>
> https://codereview.chromium.org/173443002/

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

[oshima, 2014-02-20 23:07:50]

On 2014/02/20 21:53:48, sadrul wrote:
> https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
> File ui/aura/window.cc (right):
> 
>
https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
> ui/aura/window.cc:393: transform.TransformRect(&rect);
> On 2014/02/20 20:18:51, oshima wrote:
> > On 2014/02/20 19:59:59, sadrul wrote:
> > > On 2014/02/20 19:39:35, oshima wrote:
> > > > While I understand the motivation and the current impl is also not
> accurate,
> > I
> > > > wonder if this can cause problem in the place where we are using the
value
> > of
> > > > GetBounsdInRootWindow to later compute & set the new bounds to the same
> > > window.
> > > > (which will change the value of GetBoundsInRootWindow again)
> > > 
> > > If there are such cases, perhaps we should find and fix them? (I expected
> more
> > > tests to fail if that were the case, but the lack such failures makes me
> think
> > > perhaps there are not many places that do this)
> > 
> > I think the reason is that such scenario is rare (and probably not
important)
> > and not covered by tests.
> > 
> > I'm not against this change BTW. I was just wondering if there is better way
> to
> > deal with transform.
> > 
> > For example, the above case probably should use
> > 
> > GetBoundsInRootWindowNoTransform()
> > 
> > to compute the bounds. Or the ideal form of the following code
> > 
> > window->GetBoundsInRootWindow().Contains(poitn_in_root)
> > 
> > may be something like
> > 
> > window->ContainsPointInRoot(point_in_root);
> > 
> 
> Indeed. The motivation from this patch is from here:
> https://codereview.chromium.org/167323004/#msg2 For the change in that patch,
I
> considered adding Window::GetTransformedBoundsInRootWindow(), but I decided
> against adding yet another Get*Bounds*() function in the API so there's less
> chance of someone using the wrong function for some quick fix. Instead, I
think
> it makes more sense to have GetBoundsInRootWindow/InScreen() take the
transform
> into account, and if some code expects the non-transformed bounds, then find
> some alternate solution for those cases (like the one you mention above). This
> may very well mean adding GetBoundsInRootWindowNoTransform() if it's needed in
a
> lot of places, but hopefully we can avoid that.

So the empty bounds was the side effect of using the empty rect as a target
bounds of the minimized window in ash_shell/ash_unittests.
On chrome, it'll be the icon bounds on a shelf (it may still use an empty rect
as a fallback though). Good news is that this is not required
and I can reset the transform after the animation is completed.

Here are a couple of my thoughts:

1) GetBoundsInRootWindow is used in RenderWidgetHostImpl to send window's bounds
to render
 This means it will send wrong bounds when minimized, and ti won't get updated
when unminimized.
 I can change the minimized animation to reset to minimize the chance but I'm
afraid that we may still get weird bug.
 Note:this seems to be only place in cros that can be problematic.
  
2) GetBoundsInRootWindow() is used more often on win aura in
content/browser/renderer_host. I haven't look at them, so I don't know
 if that's cause any issue. You probably should go though them and check if this
can cause any issue.

3) This change will make GetBoundsInRootWindow and GetBoundsInScreen quite
different, which makes me think that we should either have
 a separate method, or at least rename to GetTransformedBoundsInRootWindow. (or
rename GetBoundsInScreen to GetUntransformedBoundsInScreen())

[sky, 2014-02-21 00:24:22]

Since it seems we need both transformed and untransformed it seems
safest to keep the meaning of GetBoundsXXX to be untransformed.
Sadrul, if your case is the only one that needs transformed maybe we
should keep the transformed ones private and with the code that needs
them.

  -Scott

On Thu, Feb 20, 2014 at 3:07 PM,  <oshima@chromium.org> wrote:
> On 2014/02/20 21:53:48, sadrul wrote:
>>
>> https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc
>> File ui/aura/window.cc (right):
>
>
>
>
https://codereview.chromium.org/173443002/diff/170001/ui/aura/window.cc#newco...
>>
>> ui/aura/window.cc:393: transform.TransformRect(&rect);
>> On 2014/02/20 20:18:51, oshima wrote:
>> > On 2014/02/20 19:59:59, sadrul wrote:
>> > > On 2014/02/20 19:39:35, oshima wrote:
>> > > > While I understand the motivation and the current impl is also not
>> accurate,
>> > I
>> > > > wonder if this can cause problem in the place where we are using the
>
> value
>>
>> > of
>> > > > GetBounsdInRootWindow to later compute & set the new bounds to the
>> > > > same
>> > > window.
>> > > > (which will change the value of GetBoundsInRootWindow again)
>> > >
>> > > If there are such cases, perhaps we should find and fix them? (I
>> > > expected
>> more
>> > > tests to fail if that were the case, but the lack such failures makes
>> > > me
>> think
>> > > perhaps there are not many places that do this)
>> >
>> > I think the reason is that such scenario is rare (and probably not
>
> important)
>>
>> > and not covered by tests.
>> >
>> > I'm not against this change BTW. I was just wondering if there is better
>> > way
>> to
>> > deal with transform.
>> >
>> > For example, the above case probably should use
>> >
>> > GetBoundsInRootWindowNoTransform()
>> >
>> > to compute the bounds. Or the ideal form of the following code
>> >
>> > window->GetBoundsInRootWindow().Contains(poitn_in_root)
>> >
>> > may be something like
>> >
>> > window->ContainsPointInRoot(point_in_root);
>> >
>
>
>> Indeed. The motivation from this patch is from here:
>> https://codereview.chromium.org/167323004/#msg2 For the change in that
>> patch,
>
> I
>>
>> considered adding Window::GetTransformedBoundsInRootWindow(), but I
>> decided
>> against adding yet another Get*Bounds*() function in the API so there's
>> less
>> chance of someone using the wrong function for some quick fix. Instead, I
>
> think
>>
>> it makes more sense to have GetBoundsInRootWindow/InScreen() take the
>
> transform
>>
>> into account, and if some code expects the non-transformed bounds, then
>> find
>> some alternate solution for those cases (like the one you mention above).
>> This
>> may very well mean adding GetBoundsInRootWindowNoTransform() if it's
>> needed in
>
> a
>>
>> lot of places, but hopefully we can avoid that.
>
>
> So the empty bounds was the side effect of using the empty rect as a target
> bounds of the minimized window in ash_shell/ash_unittests.
> On chrome, it'll be the icon bounds on a shelf (it may still use an empty
> rect
> as a fallback though). Good news is that this is not required
> and I can reset the transform after the animation is completed.
>
> Here are a couple of my thoughts:
>
> 1) GetBoundsInRootWindow is used in RenderWidgetHostImpl to send window's
> bounds
> to render
>  This means it will send wrong bounds when minimized, and ti won't get
> updated
> when unminimized.
>  I can change the minimized animation to reset to minimize the chance but
> I'm
> afraid that we may still get weird bug.
>  Note:this seems to be only place in cros that can be problematic.
>
> 2) GetBoundsInRootWindow() is used more often on win aura in
> content/browser/renderer_host. I haven't look at them, so I don't know
>  if that's cause any issue. You probably should go though them and check if
> this
> can cause any issue.
>
> 3) This change will make GetBoundsInRootWindow and GetBoundsInScreen quite
> different, which makes me think that we should either have
>  a separate method, or at least rename to GetTransformedBoundsInRootWindow.
> (or
> rename GetBoundsInScreen to GetUntransformedBoundsInScreen())
>
>
>
>
>
> https://codereview.chromium.org/173443002/

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

[sadrul, 2014-02-21 20:30:59]

On 2014/02/21 00:24:22, sky wrote:
> Since it seems we need both transformed and untransformed it seems
> safest to keep the meaning of GetBoundsXXX to be untransformed.
> Sadrul, if your case is the only one that needs transformed maybe we
> should keep the transformed ones private and with the code that needs
> them.

Yep. SGTM