ui: Clean up damaged rects and clear them after painting.

ui: Clean up damaged rects and clear them after painting.

This cleans up some of the damage rects code by converting the SkRegion
to a cc::Region, allowing use of gfx::Rects. It moves the recursion
over the Layer tree out to Compositor instead of on Layer.

And we keep the damaged_region_ valid until the layer is painted. This
will allow us to pass that damaged_region_ to the painting code with
impl-side slimming paint, because with impl-side painting, the paint
clip rect can be larger than the invalidations (as large as the whole
layer).

R=piman@chromium.org, sky
BUG=466426
Committed: https://crrev.com/a5e585868e16ce53c990f764d4943592f11749aa
Cr-Commit-Position: refs/heads/master@{#326547}

Committed: https://crrev.com/0b5eae6c645b9b25e34322d328eaef00d14aef0f
Cr-Commit-Position: refs/heads/master@{#326912}

[sky, 2015-04-21 23:56:55]

https://codereview.chromium.org/1080633009/diff/1/content/browser/compositor/...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/1080633009/diff/1/content/browser/compositor/...
content/browser/compositor/reflector_impl_unittest.cc:92: const gfx::Rect
kSubRect(0, 0, 64, 64);
Style guide says statics/file-locals like this should only be POD (eg not
instances).

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:745: ClearDamagedRects();
What happens if as part of painting something gets damaged? Should the clear
happen before calling to paint?

[piman, 2015-04-22 03:32:14]

https://codereview.chromium.org/1080633009/diff/1/content/browser/compositor/...
File content/browser/compositor/reflector_impl_unittest.cc (right):

https://codereview.chromium.org/1080633009/diff/1/content/browser/compositor/...
content/browser/compositor/reflector_impl_unittest.cc:92: const gfx::Rect
kSubRect(0, 0, 64, 64);
On 2015/04/21 23:56:54, sky wrote:
> Style guide says statics/file-locals like this should only be POD (eg not
> instances).

IIRC style guide allows it in unit tests.

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:745: ClearDamagedRects();
On 2015/04/21 23:56:54, sky wrote:
> What happens if as part of painting something gets damaged? Should the clear
> happen before calling to paint?

Conversely, what happens if damage to the layer is added between SendDamageRects
and PaintContents?

OTOH, with display lists I wonder if we can remove the damage logic altogether.
It was added so that we could send damage and then cancel it because the
delegate would go away but we may have lost pixels. With display lists, assuming
we can keep them around, this wouldn't be a problem any more.

[danakj, 2015-04-22 16:32:21]

PTAL

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1080633009/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:745: ClearDamagedRects();
On 2015/04/22 03:32:13, piman (Very slow to review) wrote:
> On 2015/04/21 23:56:54, sky wrote:
> > What happens if as part of painting something gets damaged? Should the clear
> > happen before calling to paint?

It's possible, I wanted to think of that as a bug, but we have a unit test
covering this behaviour (which was failing), so I guess it's something we have
explicitly supported in the past.

I've moved the Clear above, which also fixes the test.

> Conversely, what happens if damage to the layer is added between
SendDamageRects
> and PaintContents?

Ya, this has always been kinda sketchy lookin'. I think the good news here is
that the ui embedder code has no chance to do anything between Layout() which
sends the rects and Paint() which uses/clears them. These both happen in the
same task (BeginMainFrame) that the compositor controls.

It means that invalidations during paint will show up /next/ frame, but that's
the behaviour we seem to want with a unit test guarding it.

> OTOH, with display lists I wonder if we can remove the damage logic
altogether.
> It was added so that we could send damage and then cancel it because the
> delegate would go away but we may have lost pixels. With display lists,
assuming
> we can keep them around, this wouldn't be a problem any more.

The ui::Layer isn't the thing holding the cache, it would still need its
delegate around to give back DisplayItems. Without a delegate, the next
recording for the layer would be empty.

[danakj, 2015-04-22 16:33:30]

On 2015/04/22 16:32:21, danakj wrote:
> The ui::Layer isn't the thing holding the cache, it would still need its
> delegate around to give back DisplayItems. Without a delegate, the next
> recording for the layer would be empty.

(We /could/ make a copy of the whole display list in the ui::Layer but I think
that would be a hefty memory regression based on what's being measured with
blink-side slimming paint and things they've had to do to avoid a copy of the
display list)

[danakj, 2015-04-22 21:08:54]

ping

[danakj, 2015-04-22 22:27:15]

Patchset #3 (id:40001) has been deleted

[sky, 2015-04-23 15:53:25]

https://codereview.chromium.org/1080633009/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1080633009/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:331: for (auto* child : layer->children())
Why are you moving the recursion here?

[danakj, 2015-04-23 16:02:57]

https://codereview.chromium.org/1080633009/diff/20001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/1080633009/diff/20001/ui/compositor/composito...
ui/compositor/compositor.cc:331: for (auto* child : layer->children())
On 2015/04/23 15:53:25, sky wrote:
> Why are you moving the recursion here?

To make the Layer function a simpler method. The Compositor knows it wants to
send it for all Layers. Layer itself can be more dumb.

[sky, 2015-04-23 16:38:55]

Ok, LGTM

[danakj, 2015-04-23 16:40:56]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2015-04-23 16:41:44]

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

[commit-bot: I haz the power, 2015-04-23 16:46:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-23 16:47:20]

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

[danakj, 2015-04-23 20:19:44]

This patch somehow breaks 4 ash unit tests /only in debug builds/:

WindowSelectorTest.MultipleDisplays
ShelfViewTest.CheckDragInsertBoundsWithMultiMonitor
PanelLayoutManagerTest.PanelMoveBetweenMultipleDisplays
AutoclickTest.MovementThreshold

[danakj, 2015-04-23 20:49:14]

On 2015/04/23 20:19:44, danakj wrote:
> This patch somehow breaks 4 ash unit tests /only in debug builds/:
> 
> WindowSelectorTest.MultipleDisplays
> ShelfViewTest.CheckDragInsertBoundsWithMultiMonitor
> PanelLayoutManagerTest.PanelMoveBetweenMultipleDisplays
> AutoclickTest.MovementThreshold

These tests suddenly take 30+ seconds in debug. Moving the ClearDamagedRects()
back into SendDamageRects() instead of PaintContents fixes it somehow.

[danakj, 2015-04-23 20:50:56]

On 2015/04/23 20:49:14, danakj wrote:
> On 2015/04/23 20:19:44, danakj wrote:
> > This patch somehow breaks 4 ash unit tests /only in debug builds/:
> > 
> > WindowSelectorTest.MultipleDisplays
> > ShelfViewTest.CheckDragInsertBoundsWithMultiMonitor
> > PanelLayoutManagerTest.PanelMoveBetweenMultipleDisplays
> > AutoclickTest.MovementThreshold
> 
> These tests suddenly take 30+ seconds in debug. Moving the ClearDamagedRects()
> back into SendDamageRects() instead of PaintContents fixes it somehow.

Putting LOG messages in SendDamageRects fixes it ^_^;

[Justin Donnelly, 2015-04-23 20:53:20]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1101103004/ by jdonnelly@chromium.org.

The reason for reverting is: ash_unittests are timing out on Win8 Aura and Linux
ChromiumOS Tests (dbg)(1) bot and this CL is suspected to be the cause of the
problem..

[danakj, 2015-04-23 21:26:38]

OK I think I see.

A layer that isn't part of the scene doesn't clear its damage, so it keeps
trying to send the damage every commit. This would be fine!

But the problem is that (unlike thread proxy) single-thread proxy will see the
request for a new commit inside the Layout() and schedule /another/ commit to
happen after the current one. WOOPS.

[danakj, 2015-04-23 22:19:36]

Fix is over here: https://codereview.chromium.org/1061993009/

[danakj, 2015-04-24 21:45:10]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-04-24 21:45:19]

That landed so going to reland this.

[commit-bot: I haz the power, 2015-04-24 21:46:06]

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

[commit-bot: I haz the power, 2015-04-24 22:35:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-24 22:37:13]

Patchset 2 (id:??) landed as
https://crrev.com/0b5eae6c645b9b25e34322d328eaef00d14aef0f
Cr-Commit-Position: refs/heads/master@{#326912}