Plumb native AppWindow input region through to window shape under Aura.

Plumb native AppWindow input region through to window shape under Aura.

This CL also fixes gfk::NativeRegion leaks in DesktopRootWindowHost for Windows & X11.

BUG=310932
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233382

[Wez, 2013-11-01 00:57:11]

PTAL - as you suggested, plumbing through to SetShape() did most of the work.

I've had to add handling for NULL gfx::NativeRegion, so that callers can
un-shape windows if they want, but I've not yet verified how other SetShape()
implementations handle that case.

I've also fixed the gfx::NativeRegion being leaked by
DesktopRootWindowHostWin::SetShape().

IIUC this patch will cause the experimental window-shape API to work under both
Windows and X11, via OS-native mechanisms. Right now we additionally enforce the
input hit-region in NativeAppWindowViews - should the Aura implementation (e.g.
for ChromeOS) be pushed down into that specific RootWindowHost implementations
instead?

[Wez, 2013-11-01 01:04:40]

On 2013/11/01 00:57:11, Wez wrote:
> Right now we additionally enforce the
> input hit-region in NativeAppWindowViews - should the Aura implementation
(e.g.
> for ChromeOS) be pushed down into that specific RootWindowHost implementations
> instead?

i.e. should we push that implementation into
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...
 We'd need to also add a filter to apply the shape to the window's opacity, to
complete the SetShape implementation for NativeWidgetAura, of course.

[Ben Goodger (Google), 2013-11-01 16:39:47]

https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
chrome/browser/ui/views/apps/native_app_window_views.cc:811: #if
defined(USE_AURA)
is there a reason this has to be USE_AURA? It should work on non-Aura too?

[Wez, 2013-11-01 19:59:33]

Incidentally, the gfx::NativeRegion leak made me wonder whether a
ScopedNativeRegion would be advisable, which would e.g. typedef to
GenericScopedHandle<HRGN, traits> on Windows. WDYT?

https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
File chrome/browser/ui/views/apps/native_app_window_views.cc (right):

https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
chrome/browser/ui/views/apps/native_app_window_views.cc:811: #if
defined(USE_AURA)
On 2013/11/01 16:39:47, Ben Goodger (Google) wrote:
> is there a reason this has to be USE_AURA? It should work on non-Aura too?

UpdateInputRegion() takes scoped_ptr<SkRegion>, while SetShape() takes
gfx::NativeRegion. We will re-name UpdateInputRegion to SetWindowShape(), and we
can change the signature at the same time.

Alternatively, I can add the SkRegion->NativeRegion conversion here, for now -
let me know which you prefer.

[Wez, 2013-11-03 04:20:22]

On 2013/11/01 19:59:33, Wez wrote:
> Incidentally, the gfx::NativeRegion leak made me wonder whether a
> ScopedNativeRegion would be advisable, which would e.g. typedef to
> GenericScopedHandle<HRGN, traits> on Windows. WDYT?
> 
>
https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
> File chrome/browser/ui/views/apps/native_app_window_views.cc (right):
> 
>
https://codereview.chromium.org/54983005/diff/1/chrome/browser/ui/views/apps/...
> chrome/browser/ui/views/apps/native_app_window_views.cc:811: #if
> defined(USE_AURA)
> On 2013/11/01 16:39:47, Ben Goodger (Google) wrote:
> > is there a reason this has to be USE_AURA? It should work on non-Aura too?
> 
> UpdateInputRegion() takes scoped_ptr<SkRegion>, while SetShape() takes
> gfx::NativeRegion. We will re-name UpdateInputRegion to SetWindowShape(), and
we
> can change the signature at the same time.
> 
> Alternatively, I can add the SkRegion->NativeRegion conversion here, for now -
> let me know which you prefer.

Ping!

[Ben Goodger (Google), 2013-11-03 05:12:05]

https://codereview.chromium.org/54983005/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/54983005/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:244: delete
native_region;
was this called for by the method docs? if not, can you update the
documentation? were we leaking these regions?

[Wez, 2013-11-03 23:52:02]

https://codereview.chromium.org/54983005/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/54983005/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:244: delete
native_region;
On 2013/11/03 05:12:05, Ben Goodger (Google) wrote:
> was this called for by the method docs? if not, can you update the
> documentation? were we leaking these regions?

The method has no docs (see
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...),
but Widget defines it as taking ownership (see
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/wi...).

Since the DesktopNativeWidgetAura::SetShape() override just passes the region
through to the DesktopRootWindowHost implementations, it's currently leaking
under both X11 and Win32 (see
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...).

I've added a comment on DesktopRootWindowHost::SetShape(), and fixed the X11
implementation. Since this API is Aura-only and so gfx::NativeRegion is
SkRegion*, the param should really be scoped_ptr<SkRegion> - but perhaps you
prefer to avoid Skia dependencies in these headers?

[Wez, 2013-11-04 21:07:59]

Ping - we need this for M32.

[Ben Goodger (Google), 2013-11-04 21:22:02]

lgtm, and thanks for fixing the leak!

[commit-bot: I haz the power, 2013-11-04 21:36:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/180001

[commit-bot: I haz the power, 2013-11-04 22:44:47]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 02:37:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-05 03:14:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-05 03:59:58]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-11-05 04:47:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-05 05:32:35]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-11-05 06:01:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-05 09:35:23]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-05 21:23:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-06 01:33:34]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 01:50:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-06 06:37:16]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 07:07:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-06 10:14:46]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 18:32:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/54983005/380001

[commit-bot: I haz the power, 2013-11-06 21:45:39]

Change committed as 233382