Prevent frameless packaged apps from being resized to 0x0 on CrOS

Prevent frameless packaged apps from being resized to 0x0 on CrOS

BUG=408049
TEST=Manual, see bug

Committed: https://crrev.com/e72536c13ce577b06cdf06f07778e8c3c5598485
Cr-Commit-Position: refs/heads/master@{#296133}

[pkotwicz, 2014-09-21 14:22:20]

pkotwicz@chromium.org changed reviewers:
+ jackhou@chromium.org

[pkotwicz, 2014-09-21 14:22:20]

jackhou@ PTAL

I am unsure if this is the right fix (but we should probably land this anyways
for the sake of backporting)

app.window.innerBounds and app.window.resizeTo() API methods are inconsistent in
how they handle setting the window bounds to a small size. I have filed
crbug.com/416273 about this

It might be possible to prevent windows from having zero size at a very low
level (views::Widget / aura::Window) but I am scared to break something. In the
Desktop Linux case, attempting to set an X11 window's bounds to 0x0 is really
bad (but it is possible to do so on ToT)

Do you have any suggestions for how to add interactive ui test for this change?

[jackhou1, 2014-09-22 04:19:15]

On 2014/09/21 14:22:20, pkotwicz wrote:
> jackhou@ PTAL
> 
> I am unsure if this is the right fix (but we should probably land this anyways
> for the sake of backporting)
> 
> app.window.innerBounds and app.window.resizeTo() API methods are inconsistent
in
> how they handle setting the window bounds to a small size. I have filed
> crbug.com/416273 about this
> 
> It might be possible to prevent windows from having zero size at a very low
> level (views::Widget / aura::Window) but I am scared to break something. In
the
> Desktop Linux case, attempting to set an X11 window's bounds to 0x0 is really
> bad (but it is possible to do so on ToT)
> 
> Do you have any suggestions for how to add interactive ui test for this
change?

I don't think this CL does what you want because
(Desktop)NativeWidgetAura::GetMinimumSize don't call Widget::GetMinimumSize. It
seems reasonable that they should return the maximum of Widget::GetMinimumSize
and whatever they return now. You could try that and see if it breaks any tests.
You can then test this behavior in (desktop_)native_widget_aura_unittest.cc.

[pkotwicz, 2014-09-22 05:06:48]

Both NativeWidgetAura::GetMinimumSize() and
DesktopNativeWidgetAura::GetMinimumSize() do call Widget::GetMinimumSize()

views::Widget is NativeWidgetAura/DesktopNativeWidgetAura's delegate

[jackhou1, 2014-09-22 05:52:20]

On 2014/09/22 05:06:48, pkotwicz wrote:
> Both NativeWidgetAura::GetMinimumSize() and
> DesktopNativeWidgetAura::GetMinimumSize() do call Widget::GetMinimumSize()
> 
> views::Widget is NativeWidgetAura/DesktopNativeWidgetAura's delegate

Ah, yes you're right.

Let me try another explanation (because I did patch this and it doesn't seem to
work).

NativeWidgetAura::SetBounds calls aura::Window::SetBounds which ensures that the
bounds are at least the minimum size. 

OTOH, DesktopNativeWidgetAura::SetBounds calls directly to
DesktopWindowTreeHost::SetBounds. 

So maybe the solution is to have either DesktopNativeWidgetAura::SetBounds or
DesktopWindowTreeHostX11::SetBounds enforce that the window does not get small
enough to hide the caption buttons.

[pkotwicz, 2014-09-22 14:51:46]

Thanks for trying the CL out. I did some more investigation.
CrOS: The CL prevents resizing to 0x0 via a JS call and by using the window
handles
Desktop Windows: The CL is a no-op. The OS prevents resizing to 0x0 via a JS
call and by using the window handles
Desktop Linux: The CL is a no-op. The OS prevents resizing to 0x0 using the
window handles. The OS does not prevent resizing to 0x0 via a JS call and the
window freezes upon doing so

I think that we should go forward and land this CL because it is trivial (and
hence easily backportable)

I have filed crbug.com/416455 and have assigned it to myself to fix this
properly.

[pkotwicz, 2014-09-22 15:11:21]

For the sake of completeness, an option would be to in addition to this CL for
DesktopWindowTreeHostX11::SetBounds() to preadjust the requested bounds based on
the minimum size. That would fix the bug on Desktop Linux

[jackhou1, 2014-09-22 23:52:14]

On 2014/09/22 14:51:46, pkotwicz wrote:
> Thanks for trying the CL out. I did some more investigation.
> CrOS: The CL prevents resizing to 0x0 via a JS call and by using the window
> handles
> Desktop Windows: The CL is a no-op. The OS prevents resizing to 0x0 via a JS
> call and by using the window handles
> Desktop Linux: The CL is a no-op. The OS prevents resizing to 0x0 using the
> window handles. The OS does not prevent resizing to 0x0 via a JS call and the
> window freezes upon doing so
> 
> I think that we should go forward and land this CL because it is trivial (and
> hence easily backportable)
> 
> I have filed crbug.com/416455 and have assigned it to myself to fix this
> properly.

lgtm

Sounds good, thanks.

[pkotwicz, 2014-09-22 23:56:19]

pkotwicz@chromium.org changed reviewers:
+ tapted@chromium.org

[pkotwicz, 2014-09-22 23:56:19]

tapted@ for OWNERS

[tapted, 2014-09-23 00:01:03]

lgtm

[pkotwicz, 2014-09-23 00:14:28]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2014-09-23 00:15:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/586323002/1

[commit-bot: I haz the power, 2014-09-23 02:46:49]

Committed patchset #1 (id:1) as b2997171e297af1cbd19379582df671b6a16a659

[commit-bot: I haz the power, 2014-09-23 02:47:23]

Patchset 1 (id:??) landed as
https://crrev.com/e72536c13ce577b06cdf06f07778e8c3c5598485
Cr-Commit-Position: refs/heads/master@{#296133}