cc: Correct inverted directions in occlusion tracker background filter logic.

cc: Correct inverted directions in occlusion tracker background filter logic.

While this logic is correct for symmetric filters, such as blur, it is not
correct for asymmetric ones, such as drop-shadow. In these cases, the affected
area should be expanded, but since the pixel-moving is not due to pixels in the
occlusion, but rather due to pixels outside the occlusion moving into it, the
left edge of the affected area should be expanded with the right outset (and so
on).

A followup CL will update this logic again (to generalize it and remove the use
of FilterOperations::GetOutsets, but correcting it (with a unit test) allows
the behaviour change to be separated from the refactor.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/19a18b17a0596d17fb7e4d6242c375518ba9a0db
Cr-Commit-Position: refs/heads/master@{#404034}

[jbroman, 2016-07-02 01:26:50]

Description was changed from

==========
cc: Correct inverted directions in occlusion tracker background filter logic.

While this logic is correct for symmetric filters, such as blur, it is not
correct for asymmetric ones, such as drop-shadow. In these cases, the affected
area should be expanded, but since the pixel-moving is not due to pixels in the
occlusion, but rather due to pixels outside the occlusion moving into it, the
left edge of the affected area should be expanded with the right outset (and so
on).

A followup CL will update this logic again (to generalize it and remove the use
of FilterOperations::GetOutsets, but correcting it (with a unit test) allows
the behaviour change to be separated from the refactor.

BUG=600821
==========

to

==========
cc: Correct inverted directions in occlusion tracker background filter logic.

While this logic is correct for symmetric filters, such as blur, it is not
correct for asymmetric ones, such as drop-shadow. In these cases, the affected
area should be expanded, but since the pixel-moving is not due to pixels in the
occlusion, but rather due to pixels outside the occlusion moving into it, the
left edge of the affected area should be expanded with the right outset (and so
on).

A followup CL will update this logic again (to generalize it and remove the use
of FilterOperations::GetOutsets, but correcting it (with a unit test) allows
the behaviour change to be separated from the refactor.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jbroman, 2016-07-02 02:02:31]

jbroman@chromium.org changed reviewers:
+ danakj@chromium.org

[jbroman, 2016-07-02 02:02:31]

I'm fairly sure I've got this right, but please take a close look just in case.
:)

[enne (OOO), 2016-07-06 18:20:30]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2016-07-06 18:20:31]

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1584: occlusion_rect = gfx::Rect(100, 0,
50, 200);
Shouldn't an adjacent occlusion layer be (150, 0) 50x200? Since you have a
200x200 root layer and the filtering layer is (50, 50) 100x100?

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1622: // needs should be removed some the
occluded area so that when we get to
grammar?

[jbroman, 2016-07-06 18:42:48]

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1584: occlusion_rect = gfx::Rect(100, 0,
50, 200);
On 2016/07/06 at 18:20:30, enne wrote:
> Shouldn't an adjacent occlusion layer be (150, 0) 50x200? Since you have a
200x200 root layer and the filtering layer is (50, 50) 100x100?

That would also work. This matches what the test above does.

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1622: // needs should be removed some the
occluded area so that when we get to
On 2016/07/06 at 18:20:30, enne wrote:
> grammar?

Copied from the previous unit test. Would you like both reworded?

[enne (OOO), 2016-07-06 19:50:00]

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1584: occlusion_rect = gfx::Rect(100, 0,
50, 200);
On 2016/07/06 at 18:42:48, jbroman wrote:
> On 2016/07/06 at 18:20:30, enne wrote:
> > Shouldn't an adjacent occlusion layer be (150, 0) 50x200? Since you have a
200x200 root layer and the filtering layer is (50, 50) 100x100?
> 
> That would also work. This matches what the test above does.

I'm just saying the comment above that this is adjacent doesn't really make
sense here.  I think making it 150 would make the test more understandable. 
What do you think?

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1622: // needs should be removed some the
occluded area so that when we get to
On 2016/07/06 at 18:42:48, jbroman wrote:
> On 2016/07/06 at 18:20:30, enne wrote:
> > grammar?
> 
> Copied from the previous unit test. Would you like both reworded?

Yes, please!

[jbroman, 2016-07-06 20:26:06]

PTAL

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1584: occlusion_rect = gfx::Rect(100, 0,
50, 200);
On 2016/07/06 at 19:50:00, enne wrote:
> On 2016/07/06 at 18:42:48, jbroman wrote:
> > On 2016/07/06 at 18:20:30, enne wrote:
> > > Shouldn't an adjacent occlusion layer be (150, 0) 50x200? Since you have a
200x200 root layer and the filtering layer is (50, 50) 100x100?
> > 
> > That would also work. This matches what the test above does.
> 
> I'm just saying the comment above that this is adjacent doesn't really make
sense here.  I think making it 150 would make the test more understandable. 
What do you think?

Turns out it _is_ adjacent, because it's only 50x50 after the scale. The comment
above tries to explain this, but you and I both missed it, so I'm going to try
and clarify.

https://codereview.chromium.org/2119043002/diff/1/cc/trees/occlusion_tracker_...
cc/trees/occlusion_tracker_unittest.cc:1622: // needs should be removed some the
occluded area so that when we get to
On 2016/07/06 at 19:49:59, enne wrote:
> On 2016/07/06 at 18:42:48, jbroman wrote:
> > On 2016/07/06 at 18:20:30, enne wrote:
> > > grammar?
> > 
> > Copied from the previous unit test. Would you like both reworded?
> 
> Yes, please!

Reworded in a way that's hopefully clearer.

[enne (OOO), 2016-07-06 21:28:48]

lgtm

[jbroman, 2016-07-07 00:19:48]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-07-07 00:20:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-07 02:38:04]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-07 02:39:35]

Description was changed from

==========
cc: Correct inverted directions in occlusion tracker background filter logic.

While this logic is correct for symmetric filters, such as blur, it is not
correct for asymmetric ones, such as drop-shadow. In these cases, the affected
area should be expanded, but since the pixel-moving is not due to pixels in the
occlusion, but rather due to pixels outside the occlusion moving into it, the
left edge of the affected area should be expanded with the right outset (and so
on).

A followup CL will update this logic again (to generalize it and remove the use
of FilterOperations::GetOutsets, but correcting it (with a unit test) allows
the behaviour change to be separated from the refactor.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Correct inverted directions in occlusion tracker background filter logic.

While this logic is correct for symmetric filters, such as blur, it is not
correct for asymmetric ones, such as drop-shadow. In these cases, the affected
area should be expanded, but since the pixel-moving is not due to pixels in the
occlusion, but rather due to pixels outside the occlusion moving into it, the
left edge of the affected area should be expanded with the right outset (and so
on).

A followup CL will update this logic again (to generalize it and remove the use
of FilterOperations::GetOutsets, but correcting it (with a unit test) allows
the behaviour change to be separated from the refactor.

BUG=600821
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/19a18b17a0596d17fb7e4d6242c375518ba9a0db
Cr-Commit-Position: refs/heads/master@{#404034}
==========

[commit-bot: I haz the power, 2016-07-07 02:39:36]

Patchset 4 (id:??) landed as
https://crrev.com/19a18b17a0596d17fb7e4d6242c375518ba9a0db
Cr-Commit-Position: refs/heads/master@{#404034}