Store non-fast scrollable regions on ScrollNode

Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2714043002
Cr-Commit-Position: refs/heads/master@{#453752}
Committed: https://chromium.googlesource.com/chromium/src/+/0283fabfe2bfe34496d303543962bed973237927

[pdr., 2017-02-24 01:02:20]

Description was changed from

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
==========

to

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-24 01:02:21]

Description was changed from

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2017-02-24 01:03:05]

pdr@chromium.org changed reviewers:
+ ajuma@chromium.org

[pdr., 2017-02-24 01:03:13]

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

[commit-bot: I haz the power, 2017-02-24 01:04:01]

Dry run: 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, 2017-02-24 03:33:42]

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

[commit-bot: I haz the power, 2017-02-24 03:33:43]

Dry run: This issue passed the CQ dry run.

[ajuma, 2017-02-24 14:27:57]

lgtm

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:1166: node.non_fast_scrollable_region =
layer->non_fast_scrollable_region();
Is LayerImpl::non_fast_scrollable_region_ now unused (except maybe in tests)?
Can we move it to LayerImplTestProperties?

[pdr., 2017-02-24 19:04:31]

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

[pdr., 2017-02-24 19:04:45]

+reviewer wkorman, you may also be interested in this patch

[pdr., 2017-02-27 23:40:23]

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:1166: node.non_fast_scrollable_region =
layer->non_fast_scrollable_region();
On 2017/02/24 at 14:27:57, ajuma wrote:
> Is LayerImpl::non_fast_scrollable_region_ now unused (except maybe in tests)?
Can we move it to LayerImplTestProperties?

This is used by debug_rect_history too. I started pulling on that thread but it
goes pretty deep.

[wkorman, 2017-02-27 23:59:25]

lgtm

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.cc
File cc/trees/scroll_node.cc (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.cc#new...
cc/trees/scroll_node.cc:37: non_fast_scrollable_region ==
other.non_fast_scrollable_region &&
And the SkRegion operator== logic is more complex than a bool:

https://cs.chromium.org/chromium/src/third_party/skia/src/core/SkRegion.cpp?t...

Again I think it's ok but worth commenting while reading.

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h
File cc/trees/scroll_node.h (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h#newc...
cc/trees/scroll_node.h:42: Region non_fast_scrollable_region;
Just to note, this replaces a per-node bool with an SkIRect and a pointer
(cc::Region has an SkRegion field, and see SkRegion::{fBounds,fRunHead}). Still
seems worthwhile for the code evolution goal.

[pdr., 2017-02-28 19:36:48]

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:1166: node.non_fast_scrollable_region =
layer->non_fast_scrollable_region();
On 2017/02/27 at 23:40:22, pdr. wrote:
> On 2017/02/24 at 14:27:57, ajuma wrote:
> > Is LayerImpl::non_fast_scrollable_region_ now unused (except maybe in
tests)? Can we move it to LayerImplTestProperties?
> 
> This is used by debug_rect_history too. I started pulling on that thread but
it goes pretty deep.

Are you okay with not doing this in this patch?

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h
File cc/trees/scroll_node.h (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h#newc...
cc/trees/scroll_node.h:42: Region non_fast_scrollable_region;
On 2017/02/27 at 23:59:24, wkorman wrote:
> Just to note, this replaces a per-node bool with an SkIRect and a pointer
(cc::Region has an SkRegion field, and see SkRegion::{fBounds,fRunHead}). Still
seems worthwhile for the code evolution goal.

This is a good point and I hadn't fully groked it. Do you think we should add a
comment to the code, or were you just noting this difference?

[ajuma, 2017-02-28 19:39:00]

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/property_tree_buil...
cc/trees/property_tree_builder.cc:1166: node.non_fast_scrollable_region =
layer->non_fast_scrollable_region();
On 2017/02/28 19:36:47, pdr. wrote:
> On 2017/02/27 at 23:40:22, pdr. wrote:
> > On 2017/02/24 at 14:27:57, ajuma wrote:
> > > Is LayerImpl::non_fast_scrollable_region_ now unused (except maybe in
> tests)? Can we move it to LayerImplTestProperties?
> > 
> > This is used by debug_rect_history too. I started pulling on that thread but
> it goes pretty deep.
> 
> Are you okay with not doing this in this patch?

Yeah, leaving that for later sgtm.

[wkorman, 2017-02-28 20:50:17]

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h
File cc/trees/scroll_node.h (right):

https://codereview.chromium.org/2714043002/diff/1/cc/trees/scroll_node.h#newc...
cc/trees/scroll_node.h:42: Region non_fast_scrollable_region;
On 2017/02/28 19:36:47, pdr. wrote:
> On 2017/02/27 at 23:59:24, wkorman wrote:
> > Just to note, this replaces a per-node bool with an SkIRect and a pointer
> (cc::Region has an SkRegion field, and see SkRegion::{fBounds,fRunHead}).
Still
> seems worthwhile for the code evolution goal.
> 
> This is a good point and I hadn't fully groked it. Do you think we should add
a
> comment to the code, or were you just noting this difference?

I don't think we need a comment, I was just noting it as I was reading, to make
sure we knew about the add'l size.

[pdr., 2017-02-28 20:54:08]

The CQ bit was checked by pdr@chromium.org

[pdr., 2017-02-28 20:54:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org,
wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2714043002/#ps20001
(title: "Rebase from space")

[commit-bot: I haz the power, 2017-02-28 20:54:55]

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, 2017-02-28 22:19:24]

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

[commit-bot: I haz the power, 2017-02-28 22:19:27]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pdr., 2017-02-28 23:04:48]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-02-28 23:05:19]

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, 2017-02-28 23:53:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488323088196460,
"parent_rev": "8bb17c830c302017416c375ab42516e789dc6908", "commit_rev":
"0283fabfe2bfe34496d303543962bed973237927"}

[commit-bot: I haz the power, 2017-02-28 23:54:09]

Description was changed from

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Store non-fast scrollable regions on ScrollNode

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2714043002
Cr-Commit-Position: refs/heads/master@{#453752}
Committed:
https://chromium.googlesource.com/chromium/src/+/0283fabfe2bfe34496d303543962...
==========

[commit-bot: I haz the power, 2017-02-28 23:54:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/0283fabfe2bfe34496d303543962...