Further plumb visual rect into cc:DisplayItemList.

Further plumb visual rect into cc:DisplayItemList.

Save rects off in cc:DisplayItemList and clear them
out in Finalize(), with TODO(vmpstr) to actually use
them to build the RTree.

Pass along the Blink visual rect obtained via work in
http://crrev.com/1426963002. Add logic to UI compositor
display item list callsites to pass along a relevant
visual rect.

Note the Blink side is still passing dummy visual rects.

BUG=529938
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/b969018fe0b7eb4b71478199249e58c85fa26847
Cr-Commit-Position: refs/heads/master@{#361612}

[wkorman, 2015-11-04 01:03:00]

Description was changed from

==========
Further plumb visual rect into cc:DisplayItemList.

Pass along the Blink visual rect obtained via work in
http://crrev.com/1426963002. Add logic to UI compositor display item
list callsites to pass along a relevant visual rect.

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

to

==========
Further plumb visual rect into cc:DisplayItemList.

Save rects off in cc:DisplayItemList and clear them
out in Finalize(), with TODO(vmpstr) to actually use
them to build the RTree.

Pass along the Blink visual rect obtained via work in
http://crrev.com/1426963002. Add logic to UI compositor
display item list callsites to pass along a relevant
visual rect.

Note the Blink side is still passing dummy visual rects.

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

[wkorman, 2015-11-04 01:03:31]

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

[wkorman, 2015-11-04 01:03:32]

Seeking initial feedback. Still reviewing visual rects in one or two places to
make sure we're doing the right thing.

[wkorman, 2015-11-04 01:10:31]

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

[commit-bot: I haz the power, 2015-11-04 01:13:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/1

[commit-bot: I haz the power, 2015-11-04 01:41:47]

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

[commit-bot: I haz the power, 2015-11-04 01:41:47]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wkorman, 2015-11-04 19:00:41]

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

[commit-bot: I haz the power, 2015-11-04 19:03:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/20001

[commit-bot: I haz the power, 2015-11-04 19:42:35]

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

[commit-bot: I haz the power, 2015-11-04 19:42:36]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[danakj, 2015-11-05 23:54:27]

The cc parts make sense, couple small comments, will review the ui stuff
shortly.

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.cc:204: // TODO(vmpstr): Build and make use of an
RTree from the visual
put the TODO on the clear()

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.cc:207: DCHECK(items_.size() ==
visual_rects_.size());
DCHECK_EQ

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

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.h:119: // position in the vector matches the
position of the item in the
nit: the position in |visual_rects_| matches the position of the item in
|items_|

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.h:121: // from the items so as to keep the display
items small.
because they are not needed while walking the |items_| for raster.

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

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:39: static const gfx::Rect
kVisualRect(0, 0, 42, 42);
gfx::Rect is non-pod it can't be a static. or is that allowed in tests? you
could make a function. or just write gfx::Rect(1, 1) everywhere or something.

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_proto_factory.cc:22: scoped_refptr<DisplayItemList>
list,
er, why is this a scoped_refptr<DisplayItemList> and not just a
DisplayItemList*. Maybe you could change that while you're here.

[danakj, 2015-11-06 00:32:45]

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:45: gfx::Rect visual_rect = clip_rect +
context_.offset_;
use the same help on context as above?

also, comment about why you're not using visual_rect_ here?

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.h:35: // |visual_rect| is expected to be
in the coordinate space of the
This belongs on visual_rect_, or should talk about |visual_size|

maybe layer_size?

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.h:53: const gfx::Rect visual_rect_;
would it make sense to call it layer_bounds_ or layer_bounding_rect_ or
something?

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:12: #include "ui/gfx/geometry/size.h"
nit: forward declare size

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:25: // opaque. |visual_size| is the size of
the bounding rect for painting.
mention coordinate space of the layer? same q about "layer" in the nam

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:33: const gfx::Rect visual_rect_;
How about storing the size, and convert to a rect when you use it? that would
keep these smaller. same q on all the others

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
ui/compositor/paint_context.cc:58: return gfx::Rect(gfx::Point(offset_.x(),
offset_.y()), visual_size);
nit: return gfx::Rect(visual_size) + offset_ ?

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:29:
visual_rect_(context.ToVisualRect(recording_size)) {
ditto, store the recording_size instead?

[wkorman, 2015-11-16 19:01:03]

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:45: gfx::Rect visual_rect = clip_rect +
context_.offset_;
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> use the same help on context as above?
> 
> also, comment about why you're not using visual_rect_ here?

Renamed to clip_in_layer_space.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.h:35: // |visual_rect| is expected to be
in the coordinate space of the
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> This belongs on visual_rect_, or should talk about |visual_size|
> 
> maybe layer_size?

Renamed to layer_size.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.h:53: const gfx::Rect visual_rect_;
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> would it make sense to call it layer_bounds_ or layer_bounding_rect_ or
something?

Renamed to layer_bounds_.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:25: // opaque. |visual_size| is the size of
the bounding rect for painting.
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> mention coordinate space of the layer? same q about "layer" in the nam

Renamed to layer_size.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/compositi...
ui/compositor/compositing_recorder.h:33: const gfx::Rect visual_rect_;
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> How about storing the size, and convert to a rect when you use it? that would
keep these smaller. same q on all the others

Discussed in person, let's try it this way, can try the other way subsequently.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
File ui/compositor/paint_context.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
ui/compositor/paint_context.cc:58: return gfx::Rect(gfx::Point(offset_.x(),
offset_.y()), visual_size);
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> nit: return gfx::Rect(visual_size) + offset_ ?

Done.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
File ui/compositor/paint_context.h (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_con...
ui/compositor/paint_context.h:83: gfx::Rect ToVisualRect(const gfx::Size&
visual_size) const;
Renamed to ToLayerSpaceBounds.

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:29:
visual_rect_(context.ToVisualRect(recording_size)) {
On 2015/11/06 at 00:32:45, danakj (behind on reviews) wrote:
> ditto, store the recording_size instead?

Same as above, can explore subsequently.

[wkorman, 2015-11-16 20:22:01]

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

[commit-bot: I haz the power, 2015-11-16 20:23:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/40001

[commit-bot: I haz the power, 2015-11-16 21:05:13]

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

[commit-bot: I haz the power, 2015-11-16 21:05:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[wkorman, 2015-11-17 01:47:23]

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.cc:204: // TODO(vmpstr): Build and make use of an
RTree from the visual
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> put the TODO on the clear()

Done.

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.cc:207: DCHECK(items_.size() ==
visual_rects_.size());
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> DCHECK_EQ

Done, also commented out with a TODO(wkorman) so we can fix issues separately
from this change.

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

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.h:119: // position in the vector matches the
position of the item in the
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> nit: the position in |visual_rects_| matches the position of the item in
|items_|

Done.

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list.h:121: // from the items so as to keep the display
items small.
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> because they are not needed while walking the |items_| for raster.

Done.

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

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:39: static const gfx::Rect
kVisualRect(0, 0, 42, 42);
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> gfx::Rect is non-pod it can't be a static. or is that allowed in tests? you
could make a function. or just write gfx::Rect(1, 1) everywhere or something.

cc/output/overlay_unittest.cc does it like this. Am fine with whatever you
prefer.

https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/overlay_...

Also others such as cc/proto/gfx_conversions_unittest.cc

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
File cc/playback/display_item_proto_factory.cc (right):

https://codereview.chromium.org/1423653005/diff/20001/cc/playback/display_ite...
cc/playback/display_item_proto_factory.cc:22: scoped_refptr<DisplayItemList>
list,
On 2015/11/05 at 23:54:27, danakj (behind on reviews) wrote:
> er, why is this a scoped_refptr<DisplayItemList> and not just a
DisplayItemList*. Maybe you could change that while you're here.

I'll send a separate change to do that to avoid commingling any issues.

[wkorman, 2015-11-17 20:36:00]

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

[vmpstr, 2015-11-17 21:15:14]

I remember there was some talk about changing the name from visual_rect to
something else? What's the ltdr there? Are we sticking with visual_rect?

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

https://codereview.chromium.org/1423653005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:204: // TODO(wkorman): Uncomment the assert
below once we've investigated
These two TODOs seem to be a bit contradictory. I don't think vmpstr should
start using them until wkorman uncomments this?

What are the issues out of curiosity?

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

https://codereview.chromium.org/1423653005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:39: static const gfx::Rect
kVisualRect(0, 0, 42, 42);
static and anonymous namespaces are redundant, i think

[wkorman, 2015-11-17 21:43:37]

On 2015/11/17 at 21:15:14, vmpstr wrote:
> I remember there was some talk about changing the name from visual_rect to
something else? What's the ltdr there? Are we sticking with visual_rect?

From in person discussion, Dana's comments around this were focused on naming
within the ui/ module, so we've reworked there to add _in_layer_space or call
things layer_bounds_ as appropriate.

Within cc/ Dana seemed to think what we have is fine. If we decide to change it
we should make change across both blink (where I've already named some things
this) and cc for consistency, and could do that in a subsequent change.

[wkorman, 2015-11-17 21:49:47]

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

https://codereview.chromium.org/1423653005/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:204: // TODO(wkorman): Uncomment the assert
below once we've investigated
On 2015/11/17 at 21:15:13, vmpstr wrote:
> These two TODOs seem to be a bit contradictory. I don't think vmpstr should
start using them until wkorman uncomments this?

Yes, the first TODO will either be resolved or we will be looking at using this
code and addressing your TODO together in person.

> What are the issues out of curiosity?

I want to look at them in a separate patch to follow in the interest of
incremental code change progress. Since there are a few open issues (nearly
resolved now) wangxianzhu@ is working on in http://crbug.com/547119 anyway.

There are mismatches causing various unit tests to fail. See my CQ dry run
results:

http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...

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

https://codereview.chromium.org/1423653005/diff/80001/cc/playback/display_ite...
cc/playback/display_item_list.h:68: visual_rects_.push_back(visual_rect);
We could consider not storing these, and clearing them, unless a flag for the
feature is enabled. We have added a flag but it is in Blink
RuntimeEnabledFeatures:

http://crrev.com/1410573008

So I think what we'd want to do is something like:

- add a switch for Chromium side to enable feature for cc/
- somehow tie that switch to enabling the Blink REF
- do the hijinks we need to do to make sure this gets passed between processes
properly

I'm not really happy with what we did for Slimming Paint. chrishtr@ says he has
something similar afoot currently. It's particularly confusing because there are
regular Chromium switches, but also cc/base/switches.cc, and then Blink REF, and
then cc has some cc-specific flag UI, and then there's also the chrome://flags
UI, and then with SP the way we did it involved monkeying web
preferences/settings somehow in a manner that was in fact persistent and I
believe could get users into a state where even with the flag switched off in
the chrome://flags UI they still had at least part of SP enabled permanently
unless they cleared their user profile data.

I think this is fine for now, we can add flag in subsequent change, I wanted to
solicit feedback.

[vmpstr, 2015-11-17 21:55:55]

On 2015/11/17 21:49:47, wkorman wrote:
>
https://codereview.chromium.org/1423653005/diff/60001/cc/playback/display_ite...
> File cc/playback/display_item_list.cc (right):
> 
>
https://codereview.chromium.org/1423653005/diff/60001/cc/playback/display_ite...
> cc/playback/display_item_list.cc:204: // TODO(wkorman): Uncomment the assert
> below once we've investigated
> On 2015/11/17 at 21:15:13, vmpstr wrote:
> > These two TODOs seem to be a bit contradictory. I don't think vmpstr should
> start using them until wkorman uncomments this?
> 
> Yes, the first TODO will either be resolved or we will be looking at using
this
> code and addressing your TODO together in person.
> 
> > What are the issues out of curiosity?
> 
> I want to look at them in a separate patch to follow in the interest of
> incremental code change progress. Since there are a few open issues (nearly
> resolved now) wangxianzhu@ is working on in http://crbug.com/547119 anyway.
> 
> There are mismatches causing various unit tests to fail. See my CQ dry run
> results:
> 
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...
> 
>
https://codereview.chromium.org/1423653005/diff/80001/cc/playback/display_ite...
> File cc/playback/display_item_list.h (right):
> 
>
https://codereview.chromium.org/1423653005/diff/80001/cc/playback/display_ite...
> cc/playback/display_item_list.h:68: visual_rects_.push_back(visual_rect);
> We could consider not storing these, and clearing them, unless a flag for the
> feature is enabled. We have added a flag but it is in Blink
> RuntimeEnabledFeatures:
> 
> http://crrev.com/1410573008
> 
> So I think what we'd want to do is something like:
> 
> - add a switch for Chromium side to enable feature for cc/
> - somehow tie that switch to enabling the Blink REF
> - do the hijinks we need to do to make sure this gets passed between processes
> properly
> 
> I'm not really happy with what we did for Slimming Paint. chrishtr@ says he
has
> something similar afoot currently. It's particularly confusing because there
are
> regular Chromium switches, but also cc/base/switches.cc, and then Blink REF,
and
> then cc has some cc-specific flag UI, and then there's also the chrome://flags
> UI, and then with SP the way we did it involved monkeying web
> preferences/settings somehow in a manner that was in fact persistent and I
> believe could get users into a state where even with the flag switched off in
> the chrome://flags UI they still had at least part of SP enabled permanently
> unless they cleared their user profile data.
> 
> I think this is fine for now, we can add flag in subsequent change, I wanted
to
> solicit feedback.

FWIW, we can skip adding things to the list and clearing later until we actually
start using the rtree (that is, adding the list on display item list can be a
part of that patch, not this one). This probably would avoid the whole mess with
flags, since the perf impact would be negligible. Right?

[wkorman, 2015-11-17 21:58:16]

On 2015/11/17 at 21:55:55, vmpstr wrote:
> FWIW, we can skip adding things to the list and clearing later until we
actually start using the rtree (that is, adding the list on display item list
can be a part of that patch, not this one). This probably would avoid the whole
mess with flags, since the perf impact would be negligible. Right?

Yes, but there's also value to getting some insight right now into what perf
impact of adding/clearing would look like. I'd vote for submitting this and
seeing what happens, and in meantime we can discuss what we want to do flag
wise. Since we probably want to do something.

[danakj, 2015-11-17 22:14:33]

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:20: const gfx::Size& layer_size)
IIRC when we'd talked about this, we discussed "layer_size" being a bit
ambiguous, and "size_in_layer" being more obvious. i'd prefer that throughout as
the outcome.

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:45: gfx::Rect clip_in_layer_space =
clip_rect + context_.offset_;
similar to this (with or without _space, but maybe consistently?)

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:45: gfx::Rect clip_in_layer_space =
clip_rect + context_.offset_;
also i'd prefer adding a PaintContext method to convert the rect instead of
using offset_ directly here, but a helper method above

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.h:21: class Rect;
remove this, add Size though

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/compositi...
ui/compositor/compositing_recorder.cc:11: #include "ui/gfx/geometry/rect.h"
already in the .h file.

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:12: #include "ui/gfx/geometry/rect.h"
already in the .h file

https://codereview.chromium.org/1423653005/diff/80001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/views/view.cc#newcod...
ui/views/view.cc:803: if (is_invalidated || !paint_cache_.UseCache(context)) {
The thing that's going to be weird is that if the view moves around, it will use
the cache, but it cached a different layer bounds. So it's going to be wrong.

Maybe this works out today because we do invalidation by rects, and we probably
invalidate when things move. This would be less true if we invalidated based on
the /contents/ of the view, except now the contents include a visible rect.

How is blink handling this? Does it invalidate its cache of display items when
they move?

Alternately, UseCache() could take a size(), and it could change the bounds on
the cached item when it appends it.

[enne (OOO), 2015-11-17 22:28:20]

lgtm % danakj && vmpstr

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

https://codereview.chromium.org/1423653005/diff/80001/cc/playback/display_ite...
cc/playback/display_item_list.h:68: visual_rects_.push_back(visual_rect);
I don't think we need a flag for this.  We should make this fast enough and then
land it.

[wkorman, 2015-11-17 23:34:18]

On 2015/11/17 at 22:14:33, danakj wrote:
>
https://codereview.chromium.org/1423653005/diff/80001/ui/views/view.cc#newcod...
> ui/views/view.cc:803: if (is_invalidated || !paint_cache_.UseCache(context)) {
> The thing that's going to be weird is that if the view moves around, it will
use the cache, but it cached a different layer bounds. So it's going to be
wrong.
> 
> Maybe this works out today because we do invalidation by rects, and we
probably invalidate when things move. This would be less true if we invalidated
based on the /contents/ of the view, except now the contents include a visible
rect.
> 
> How is blink handling this? Does it invalidate its cache of display items when
they move?
> 
> Alternately, UseCache() could take a size(), and it could change the bounds on
the cached item when it appends it.

I can't reply on the file as the code review UI isn't showing it to me inline,
so replying here instead. I expect Blink invalidates when something moves, and
that we should do similar. Let's ask chrishtr@ in person.

[wkorman, 2015-11-17 23:43:05]

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
File ui/compositor/clip_transform_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:20: const gfx::Size& layer_size)
On 2015/11/17 at 22:14:33, danakj (behind on reviews) wrote:
> IIRC when we'd talked about this, we discussed "layer_size" being a bit
ambiguous, and "size_in_layer" being more obvious. i'd prefer that throughout as
the outcome.

Done.

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/clip_tran...
ui/compositor/clip_transform_recorder.cc:45: gfx::Rect clip_in_layer_space =
clip_rect + context_.offset_;
On 2015/11/17 at 22:14:33, danakj (behind on reviews) wrote:
> similar to this (with or without _space, but maybe consistently?)

Added and made use of PaintContext::ToLayerSpaceRect(). That method name is sort
of ambiguous since as implemented/described in the comment it does one specific
thing, but one could imagine lots of ways one might want to convert a rect to be
a rect in layer space. Anyway, it's documented, and we can iterate as we learn
more.

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/compositi...
File ui/compositor/compositing_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/compositi...
ui/compositor/compositing_recorder.cc:11: #include "ui/gfx/geometry/rect.h"
On 2015/11/17 at 22:14:33, danakj (behind on reviews) wrote:
> already in the .h file.

Done.

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/paint_rec...
File ui/compositor/paint_recorder.cc (right):

https://codereview.chromium.org/1423653005/diff/80001/ui/compositor/paint_rec...
ui/compositor/paint_recorder.cc:12: #include "ui/gfx/geometry/rect.h"
On 2015/11/17 at 22:14:33, danakj (behind on reviews) wrote:
> already in the .h file

Done.

[vmpstr, 2015-11-17 23:51:38]

cc lgtm

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:204: // TODO(wkorman): Uncomment the assert
below once we've investigated
nit: Can you reference a bug for this?

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:210: // them to stick around post-Finalize.
nit: Can you put "crbug.com/527245" in this comment?

[wkorman, 2015-11-18 19:37:35]

Dana, do you have any other comments?

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:204: // TODO(wkorman): Uncomment the assert
below once we've investigated
On 2015/11/17 at 23:51:38, vmpstr wrote:
> nit: Can you reference a bug for this?

Done.

https://codereview.chromium.org/1423653005/diff/100001/cc/playback/display_it...
cc/playback/display_item_list.cc:210: // them to stick around post-Finalize.
On 2015/11/17 at 23:51:38, vmpstr wrote:
> nit: Can you put "crbug.com/527245" in this comment?

Done.

[wkorman, 2015-11-18 20:49:11]

On 2015/11/18 at 19:37:35, wkorman wrote:
> Dana, do you have any other comments?

Addressed two things discussed in person:

- renamed layer_bounds to bounds_in_layer
- don't cache bounds_in_layer in paint_cache

[danakj, 2015-11-18 22:23:13]

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

[danakj, 2015-11-18 22:23:14]

chrome/ and ui/ LGTM (you'll need sky for owners)

+sky

https://codereview.chromium.org/1423653005/diff/140001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1423653005/diff/140001/ui/views/view.cc#newco...
ui/views/view.cc:803: if (is_invalidated || !paint_cache_.UseCache(context,
size())) {
can you leave a comment here saying that the cache is not aware of the offset of
the view relative to the parent since painting is always done relative to the
top left of the individual view?

[wkorman, 2015-11-18 23:01:31]

https://codereview.chromium.org/1423653005/diff/140001/ui/views/view.cc
File ui/views/view.cc (right):

https://codereview.chromium.org/1423653005/diff/140001/ui/views/view.cc#newco...
ui/views/view.cc:803: if (is_invalidated || !paint_cache_.UseCache(context,
size())) {
On 2015/11/18 at 22:23:14, danakj (behind on reviews) wrote:
> can you leave a comment here saying that the cache is not aware of the offset
of the view relative to the parent since painting is always done relative to the
top left of the individual view?

Done.

[sky, 2015-11-19 00:37:34]

ui/views/view LGTM - note that since you didn't document what the size supplied
to the recorder means, I couldn't figure out if this is right. I'm assuming Dana
knows what the size means and works as intended.

https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
File ui/compositor/clip_transform_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
ui/compositor/clip_transform_recorder.h:36: const gfx::Size& size_in_layer);
nit: no explicit, and you don't describe what size_in_layer means.

[danakj, 2015-11-19 00:45:17]

On Wed, Nov 18, 2015 at 4:37 PM, <sky@chromium.org> wrote:

> ui/views/view LGTM - note that since you didn't document what the size
> supplied
> to the recorder means, I couldn't figure out if this is right. I'm
> assuming Dana
> knows what the size means and works as intended.
>

The size() is a bounding area around everything that's visible. In this
case we just use the bounds of the entire view and consider it all visible.
Adding some documentation sounds good.


>
>
>
>
https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
> File ui/compositor/clip_transform_recorder.h (right):
>
>
>
https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
> ui/compositor/clip_transform_recorder.h:36: const gfx::Size&
> size_in_layer);
> nit: no explicit, and you don't describe what size_in_layer means.
>
> https://codereview.chromium.org/1423653005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wkorman, 2015-11-25 00:26:39]

danakj@ PTAL, notes on changes:
- added visual rect to trace output in display item AsValueInto()
- added unit tests to view_unittest.cc
- updated recorder constructor docs to describe size param

https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
File ui/compositor/clip_transform_recorder.h (right):

https://codereview.chromium.org/1423653005/diff/160001/ui/compositor/clip_tra...
ui/compositor/clip_transform_recorder.h:36: const gfx::Size& size_in_layer);
On 2015/11/19 at 00:37:34, sky wrote:
> nit: no explicit, and you don't describe what size_in_layer means.

Done.

[danakj, 2015-11-25 00:30:56]

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest...
ui/views/view_unittest.cc:533: const gfx::Rect& visual_rect_in_layer_space) {
expected_vis...

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest...
ui/views/view_unittest.cc:581: 
check the visual rects after moving, both with and without cache please

[wkorman, 2015-11-25 00:44:51]

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (right):

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest...
ui/views/view_unittest.cc:533: const gfx::Rect& visual_rect_in_layer_space) {
On 2015/11/25 at 00:30:56, danakj (behind on reviews) wrote:
> expected_vis...

Done.

https://codereview.chromium.org/1423653005/diff/220001/ui/views/view_unittest...
ui/views/view_unittest.cc:581: 
On 2015/11/25 at 00:30:56, danakj (behind on reviews) wrote:
> check the visual rects after moving, both with and without cache please

Done.

[wkorman, 2015-11-25 00:45:03]

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

[commit-bot: I haz the power, 2015-11-25 00:47:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/240001

[danakj, 2015-11-25 01:00:11]

Thanks for the unit tests and nicer comments! LGTM

[wkorman, 2015-11-25 01:27:48]

The CQ bit was checked by wkorman@chromium.org

[wkorman, 2015-11-25 01:27:49]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
vmpstr@chromium.org, sky@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1423653005/#ps260001
(title: "Fix clip recorder params in omnibox.")

[commit-bot: I haz the power, 2015-11-25 01:30:45]

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

[commit-bot: I haz the power, 2015-11-25 01:31:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/260001

[commit-bot: I haz the power, 2015-11-25 03:48:06]

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

[commit-bot: I haz the power, 2015-11-25 03:48:07]

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

[wkorman, 2015-11-25 06:47:47]

The CQ bit was checked by wkorman@chromium.org

[commit-bot: I haz the power, 2015-11-25 06:48:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1423653005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1423653005/260001

[commit-bot: I haz the power, 2015-11-25 09:13:37]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-25 09:15:06]

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