Move visual rect unioning between paired items to cc::DisplayItemList.

Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

Visual rects now only have to be explicitly passed for drawing display
items.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625
Cr-Commit-Position: refs/heads/master@{#411195}

[wkorman, 2016-08-09 23:24:30]

Description was changed from

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

BUG=634239
==========

to

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[wkorman, 2016-08-10 00:49:02]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 00:49:53]

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

[wkorman, 2016-08-10 00:52:19]

Description was changed from

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[wkorman, 2016-08-10 00:53:43]

Description was changed from

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

Visual rects now only have to be explicitly passed for drawing display
items.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-10 02:00:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 02:00:39]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[wkorman, 2016-08-10 19:26:28]

wkorman@chromium.org changed reviewers:
+ chrishtr@chromium.org, vmpstr@chromium.org

[wkorman, 2016-08-10 19:26:29]

Main changes are in {cc,blink}::DisplayItemList. Others are mainly method
signature rename/splitting to allow differentiating between drawing and
begin/end, and unit test porting.

There is follow-on cleanup we can do in Blink to make it clearer that now only
drawing display items require a visual rect to be stored and passed along to cc.
I've added TODOs for that work, which would also potentially remove some
unnecessary memory/computation Blink-side.

[wkorman, 2016-08-10 19:26:41]

The CQ bit was checked by wkorman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:27:09]

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

[vmpstr, 2016-08-10 19:47:31]

Just reviewed cc. Overall looks good, just some small comments.

https://codereview.chromium.org/2230513005/diff/60001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.h (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.h:45: void appendDrawingItem(const
blink::WebRect&,
While here, can you give these parameters a name?

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:87: size_t last_begin_index =
inputs_.begin_item_indices.back();
DCHECK(!inputs_.begin_item_indices.empty());

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:95:
GrowCurrentBeginItemVisualRect(inputs_.visual_rects[last_begin_index]);
I think inputs_.visual_rects.back() is cleaner. Either that or

const auto& visual_rect = inputs_.visual_rects[last_begin_index];
and then use visual_rect both here and in the push_back above.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:48: #define EXPECT_RECT_EQ(expected,
actual)               \
Can you add this to cc/test/geometry_test_utils.h please.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:61: sk_sp<const SkPicture>
createRectPicture(const gfx::Rect& bounds) {
Capital c please.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:676: TEST(DisplayItemListTest,
Size_Empty) {
s/_//g here and throughout.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:686: EXPECT_EQ(static_cast<size_t>(1),
list->size());
1u instead of static_cast here and throughout.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:770: // One block with one drawing:
B1, Da (escapes), E1.
What does "escapes" mean in this case?

[chrishtr, 2016-08-10 19:55:58]

lgtm

[wkorman, 2016-08-10 21:04:26]

https://codereview.chromium.org/2230513005/diff/60001/cc/blink/web_display_it...
File cc/blink/web_display_item_list_impl.h (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/blink/web_display_it...
cc/blink/web_display_item_list_impl.h:45: void appendDrawingItem(const
blink::WebRect&,
On 2016/08/10 at 19:47:31, vmpstr wrote:
> While here, can you give these parameters a name?

Done.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:87: size_t last_begin_index =
inputs_.begin_item_indices.back();
On 2016/08/10 at 19:47:31, vmpstr wrote:
> DCHECK(!inputs_.begin_item_indices.empty());

Done.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:95:
GrowCurrentBeginItemVisualRect(inputs_.visual_rects[last_begin_index]);
On 2016/08/10 at 19:47:31, vmpstr wrote:
> I think inputs_.visual_rects.back() is cleaner. Either that or
> 
> const auto& visual_rect = inputs_.visual_rects[last_begin_index];
> and then use visual_rect both here and in the push_back above.

Neither seems equivalent to existing logic:
1. back() doesn't reference the same rect in all circumstances since
last_begin_index could reference an item other than the currently-last one
2. const auto& grabs a ref to an element in the visual_rects vector which does
not look to survive through vector mutation in line 91.

Sample test logging below to illustrate:

[ RUN      ] DisplayItemListTest.AppendVisualRectTwoBlocksTwoDrawings
[26331:26331:0810/134228:702083247033:ERROR:display_item_list.h(93)] 1:
last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto rect=7,8 1x1
[26331:26331:0810/134228:702083247127:ERROR:display_item_list.h(100)] 1:
last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto
rect=-842150451,-842150451 -842150451x-842150451
[26331:26331:0810/134228:702083247198:ERROR:display_item_list.h(93)] 1:
last_begin_index=0, rect=5,6 3x3, back() rect=7,8 1x1, auto rect=5,6 3x3
[26331:26331:0810/134228:702083247254:ERROR:display_item_list.h(100)] 1:
last_begin_index=0, rect=5,6 3x3, back() rect=5,6 3x3, auto rect=5,6 3x3

Perhaps we should add a comment here to make this clearer though, since all
three of us have now at one point or another thought this could be streamlined,
I took a pass, open to thoughts.

Or, we can do it with your second option (const auto&) but we'd need to make a
copy of the rect rather than use a reference. I prefer it as is but open to
switching if you don't mind the copy.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:48: #define EXPECT_RECT_EQ(expected,
actual)               \
On 2016/08/10 at 19:47:31, vmpstr wrote:
> Can you add this to cc/test/geometry_test_utils.h please.

Done.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:61: sk_sp<const SkPicture>
createRectPicture(const gfx::Rect& bounds) {
On 2016/08/10 at 19:47:31, vmpstr wrote:
> Capital c please.

Done.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:676: TEST(DisplayItemListTest,
Size_Empty) {
On 2016/08/10 at 19:47:31, vmpstr wrote:
> s/_//g here and throughout.

Ah, that's unfortunate, I find this style much easier to interpret, but ok.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:686: EXPECT_EQ(static_cast<size_t>(1),
list->size());
On 2016/08/10 at 19:47:31, vmpstr wrote:
> 1u instead of static_cast here and throughout.

Done.

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:770: // One block with one drawing:
B1, Da (escapes), E1.
On 2016/08/10 at 19:47:31, vmpstr wrote:
> What does "escapes" mean in this case?

It means "escapes the clip", as in, the drawing's bounds are outside of the clip
bounds. Prior to this patch the naming made a bit more sense, since the clip had
its own visual rect, so in addition to the contained drawing "escaping the
clip", it also escaped the visual rect of the clip.

I think the tests are still useful. Their focus has changed to one where they
validate that paired items end up with a visual rect unioned from their
contained drawings, whereas previously it was their pre-existing visual rect,
unioned with that of the drawing's.

I didn't immediately think of better naming, but open to thoughts.

[vmpstr, 2016-08-10 21:09:43]

cc lgtm

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/2230513005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:95:
GrowCurrentBeginItemVisualRect(inputs_.visual_rects[last_begin_index]);
On 2016/08/10 21:04:25, wkorman wrote:
> On 2016/08/10 at 19:47:31, vmpstr wrote:
> > I think inputs_.visual_rects.back() is cleaner. Either that or
> > 
> > const auto& visual_rect = inputs_.visual_rects[last_begin_index];
> > and then use visual_rect both here and in the push_back above.
> 
> Neither seems equivalent to existing logic:
> 1. back() doesn't reference the same rect in all circumstances since
> last_begin_index could reference an item other than the currently-last one
> 2. const auto& grabs a ref to an element in the visual_rects vector which does
> not look to survive through vector mutation in line 91.
> 
> Sample test logging below to illustrate:
> 
> [ RUN      ] DisplayItemListTest.AppendVisualRectTwoBlocksTwoDrawings
> [26331:26331:0810/134228:702083247033:ERROR:display_item_list.h(93)] 1:
> last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto rect=7,8 1x1
> [26331:26331:0810/134228:702083247127:ERROR:display_item_list.h(100)] 1:
> last_begin_index=2, rect=7,8 1x1, back() rect=7,8 1x1, auto
> rect=-842150451,-842150451 -842150451x-842150451
> [26331:26331:0810/134228:702083247198:ERROR:display_item_list.h(93)] 1:
> last_begin_index=0, rect=5,6 3x3, back() rect=7,8 1x1, auto rect=5,6 3x3
> [26331:26331:0810/134228:702083247254:ERROR:display_item_list.h(100)] 1:
> last_begin_index=0, rect=5,6 3x3, back() rect=5,6 3x3, auto rect=5,6 3x3
> 
> Perhaps we should add a comment here to make this clearer though, since all
> three of us have now at one point or another thought this could be
streamlined,
> I took a pass, open to thoughts.
> 
> Or, we can do it with your second option (const auto&) but we'd need to make a
> copy of the rect rather than use a reference. I prefer it as is but open to
> switching if you don't mind the copy.

Err, yeah you're right. You can ignore my comment.

[wkorman, 2016-08-10 21:19:30]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2016-08-10 21:19:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2230513005/#ps80001
(title: "Code review feedback.")

[commit-bot: I haz the power, 2016-08-10 21:20:02]

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-08-10 21:29:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 21:29:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[wkorman, 2016-08-10 21:29:34]

wkorman@chromium.org changed reviewers:
+ sky@chromium.org

[wkorman, 2016-08-10 21:29:35]

+sky for chrome/browser/ui/views, ui/compositor, and ui/views OWNERS. Changes
are nearly all just method name/signature changes. We no longer have to pass
size() as we build up the visual rects for involved display items within
cc::DisplayItemList directly from any contained drawing display items.
ClipRecorder had code deleted that was used to track bounds, now obsolete for
same reason.

[sky, 2016-08-10 22:27:06]

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

[sky, 2016-08-10 22:27:06]

LGTM

[wkorman, 2016-08-10 22:29:14]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2016-08-10 22:30:41]

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-08-11 00:04:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-11 00:07:04]

Description was changed from

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

Visual rects now only have to be explicitly passed for drawing display
items.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Move visual rect unioning between paired items to cc::DisplayItemList.

http://crrev.com/1814013003 added this for Blink-driven visual rects,
and it turns out we need it for ui/views as well, so we centralize
in cc directly.

Visual rects now only have to be explicitly passed for drawing display
items.

BUG=634239
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625
Cr-Commit-Position: refs/heads/master@{#411195}
==========

[commit-bot: I haz the power, 2016-08-11 00:07:06]

Patchset 5 (id:??) landed as
https://crrev.com/923cfdd728ac4e0f7f35333c994c79e6f9403625
Cr-Commit-Position: refs/heads/master@{#411195}