cc: Replace Region with SimpleEnclosedRegion for occlusion tracking

cc: Replace Region with SimpleEnclosedRegion for occlusion tracking

Instead of using an arbitrary Region, which is costly (slow), use
a new SimpleEnclosedRegion. This class tracks only a single Rect
at a given time so it is very fast and small. It tries to get
something like the largest rect enclosed in the actual Region
(were we to track such a Region) in an online fashion, ie it
doesn't remember anything except its current largest possible
rect.

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

[danakj, 2014-08-21 01:28:28]

https://codereview.chromium.org/202523002/diff/60001/cc/trees/occlusion_track...
File cc/trees/occlusion_tracker_unittest.cc (left):

https://codereview.chromium.org/202523002/diff/60001/cc/trees/occlusion_track...
cc/trees/occlusion_tracker_unittest.cc:668:
EXPECT_TRUE(occlusion.OccludedLayer(parent, gfx::Rect(30, 30, 70, 70)));
I removed all the checks for OccludedLayer() all over this file, since we query
the occlusion state directly instead. And added a test at the bottom that checks
OccludedLayer() does the right thing for various occlusion states.

https://codereview.chromium.org/202523002/diff/60001/cc/trees/occlusion_track...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/202523002/diff/60001/cc/trees/occlusion_track...
cc/trees/occlusion_tracker_unittest.cc:555: EXPECT_EQ(gfx::Rect(29, 30, 1, 70),
I changed all EXPECT_RECT_EQ to EXPECT_EQ in this file, because I think that
helped with dealing with Rect vs SimpleEnclosedRegion, but anyway EXPECT_RECT_EQ
is redundant now thanks to jamesr.

https://codereview.chromium.org/202523002/diff/60001/cc/trees/occlusion_track...
cc/trees/occlusion_tracker_unittest.cc:2665: // Make a 50x50 filtered surface
that is adjacent to occluding layers
I did this test in 4 passes so we can test each side still, because we can't
test all 4 at once with a single rect.

[enne (OOO), 2014-08-21 20:48:05]

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
File cc/base/simple_enclosed_region.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
cc/base/simple_enclosed_region.cc:96: if (new_top <= top && new_bottom >=
bottom) {
So many conditionals.  O_O

Could you leave a brief comment to explain what this is doing and make it less
dense?  As far as I can tell, you're checking vertically and horizontally both
the new and the old rect to see if it entirely encloses (on that axis) the
other, and if so, then for the other axis you check if it overlaps the other so
that you can safely expand out that side?

Would it be more readable to just check if the old and new don't intersect at
all and early out at the top? And then the first conditional would just be
something like:

  if (new_top <= top && new_bottom >= bottom) {
    left = std::min(left, new_left);
    right = std::max(right, new_right);
  }

(If I'm reading this correctly?)

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker.cc:612: unoccluded_rect_in_target_surface.Subtract(
I didn't even know we had gfx::Rect::Subtract!

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (left):

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:1058: +---------------------+
This beautiful ascii art diagram, nuoooo~!

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:2667: // surface isscaled to test that
the pixel moving is done in the target
typo?

[danakj, 2014-08-21 21:15:31]

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
File cc/base/simple_enclosed_region.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
cc/base/simple_enclosed_region.cc:96: if (new_top <= top && new_bottom >=
bottom) {
On 2014/08/21 20:48:05, enne wrote:
> So many conditionals.  O_O
> 
> Could you leave a brief comment to explain what this is doing and make it less
> dense?  As far as I can tell, you're checking vertically and horizontally both
> the new and the old rect to see if it entirely encloses (on that axis) the
> other, and if so, then for the other axis you check if it overlaps the other
so
> that you can safely expand out that side?
> 
> Would it be more readable to just check if the old and new don't intersect at
> all and early out at the top? And then the first conditional would just be
> something like:
> 
>   if (new_top <= top && new_bottom >= bottom) {
>     left = std::min(left, new_left);
>     right = std::max(right, new_right);
>   }
> 
> (If I'm reading this correctly?)

I think that would regress the case of two rectangles beside each other. They
don't intersect but they can be expanded.

Other than that yes you have it right. I will add a comment, but I was hoping
the unit tests would also document this mess of fun :)

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (left):

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:1058: +---------------------+
On 2014/08/21 20:48:05, enne wrote:
> This beautiful ascii art diagram, nuoooo~!

Don't worry there is more still xD

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:2667: // surface isscaled to test that
the pixel moving is done in the target
On 2014/08/21 20:48:05, enne wrote:
> typo?

Done.

[enne (OOO), 2014-08-21 21:19:40]

lgtm

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
File cc/base/simple_enclosed_region.cc (right):

https://codereview.chromium.org/202523002/diff/100001/cc/base/simple_enclosed...
cc/base/simple_enclosed_region.cc:96: if (new_top <= top && new_bottom >=
bottom) {
On 2014/08/21 21:15:31, danakj wrote:
> On 2014/08/21 20:48:05, enne wrote:
> > So many conditionals.  O_O
> > 
> > Could you leave a brief comment to explain what this is doing and make it
less
> > dense?  As far as I can tell, you're checking vertically and horizontally
both
> > the new and the old rect to see if it entirely encloses (on that axis) the
> > other, and if so, then for the other axis you check if it overlaps the other
> so
> > that you can safely expand out that side?
> > 
> > Would it be more readable to just check if the old and new don't intersect
at
> > all and early out at the top? And then the first conditional would just be
> > something like:
> > 
> >   if (new_top <= top && new_bottom >= bottom) {
> >     left = std::min(left, new_left);
> >     right = std::max(right, new_right);
> >   }
> > 
> > (If I'm reading this correctly?)
> 
> I think that would regress the case of two rectangles beside each other. They
> don't intersect but they can be expanded.
> 
> Other than that yes you have it right. I will add a comment, but I was hoping
> the unit tests would also document this mess of fun :)

Aha, ok.  That case makes sense.

[danakj, 2014-08-21 21:19:44]

PTAL

I can add ascii art to the comment if that would help. Let me know what you
think of it.

[danakj, 2014-08-21 21:21:29]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-08-21 21:21:32]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-08-21 21:46:25]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-21 21:49:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/202523002/120001

[commit-bot: I haz the power, 2014-08-21 22:51:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-22 01:17:01]

Committed patchset #4 (120001) as 291292