Turn off computation of the interest rect in cc in synchronized paint mode.

Turn off computation of the interest rect in cc in synchronized paint mode.

To do this, introduces a new render process shared setting for synchronized
painting.

BUG=552545
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/01539b801aeca7e811e8441fdfad3011098576dd
Cr-Commit-Position: refs/heads/master@{#361282}

[chrishtr, 2015-11-17 23:15:59]

Description was changed from

==========
none


none


none


none

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

to

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronous
painting.
==========

[chrishtr, 2015-11-17 23:15:59]

Description was changed from

==========
none


none


none


none

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

to

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronous
painting.
==========

[chrishtr, 2015-11-17 23:24:23]

Description was changed from

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronous
painting.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronous
painting.

BUG=552545
==========

[chrishtr, 2015-11-17 23:24:30]

chrishtr@chromium.org changed reviewers:
+ danakj@chromium.org, wangxianzhu@chromium.org

[chrishtr, 2015-11-17 23:24:41]

Description was changed from

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronous
painting.

BUG=552545
==========

to

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronized
painting.

BUG=552545
==========

[chrishtr, 2015-11-17 23:24:56]

Description was changed from

==========
Turn off computation of the interest rect in cc.

To do this, introduces a new render process shared setting for synchronized
painting.

BUG=552545
==========

to

==========
Turn off computation of the interest rect in cc in synchronized paint mode.

To do this, introduces a new render process shared setting for synchronized
painting.

BUG=552545
==========

[enne (OOO), 2015-11-17 23:44:48]

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

[enne (OOO), 2015-11-17 23:44:49]

https://codereview.chromium.org/1452353002/diff/40001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:143: update_rect = gfx::Rect(layer_size);
You need to implement this in Blink as a part of synchronous painting, and then
this can go away.  Can you put a TODO here?

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
File cc/playback/display_list_recording_source.cc (right):

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:133:
potential_new_recorded_viewport.Intersect(gfx::Rect(GetSize()));
Because you still need to add invalidations for newly exposed recording areas
below, I think a better API would be to not pass a setting all the way through
and instead add ContentLayerClient::PaintableViewport() or some such.

Then you could ask for the recorded_viewport before you paint, and then go
through this same logic.  And then PaintContents wouldn't need a parameter at
all (in the future).

Alternatively, the synchronous paint path could do all this work?

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:141: // Invalidate newly-exposed
and no-longer-exposed areas.
This is still needed even for synchronous painting.

[chrishtr, 2015-11-18 21:09:40]

FYI the record-full-layers thing needs to be implemented in Blink before I
commit this patch, because now the patch is written to just switch over to
Blink's interest rect logic right away, with no flag. Will put up a patch
shortly for that.

https://codereview.chromium.org/1452353002/diff/40001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:143: update_rect = gfx::Rect(layer_size);
On 2015/11/17 at 23:44:49, enne wrote:
> You need to implement this in Blink as a part of synchronous painting, and
then this can go away.  Can you put a TODO here?

Done.

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
File cc/playback/display_list_recording_source.cc (right):

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:133:
potential_new_recorded_viewport.Intersect(gfx::Rect(GetSize()));
On 2015/11/17 at 23:44:49, enne wrote:
> Because you still need to add invalidations for newly exposed recording areas
below, I think a better API would be to not pass a setting all the way through
and instead add ContentLayerClient::PaintableViewport() or some such.
> 
> Then you could ask for the recorded_viewport before you paint, and then go
through this same logic.  And then PaintContents wouldn't need a parameter at
all (in the future).
> 
> Alternatively, the synchronous paint path could do all this work?

Done.

https://codereview.chromium.org/1452353002/diff/40001/cc/playback/display_lis...
cc/playback/display_list_recording_source.cc:141: // Invalidate newly-exposed
and no-longer-exposed areas.
On 2015/11/17 at 23:44:49, enne wrote:
> This is still needed even for synchronous painting.

Done.

[enne (OOO), 2015-11-18 21:31:58]

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_imag...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_imag...
cc/layers/picture_image_layer.cc:53: return gfx::Rect(0, 0, bounds().width(),
bounds().height());
"return bounds()"?

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:137: // TODO(chrishtr): Blink should no longer be
invalidating during paint.
This isn't a thing to fix in this patch, but similarly we have some code in
Layer::Update about paint properties where Blink tries to resize layers during
paint.  If we can ignore those things too, it'd be a great cleanup.

https://codereview.chromium.org/1452353002/diff/100001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:39: CHECK(bounds_set_);
I get that you're trying to check that you covered everything.  But, not all of
these tests actually paint anything.

What do you think about just setting this to a default value of an empty rect so
that every test that uses a FakeContentLayerClient doesn't have to update this
value?

https://codereview.chromium.org/1452353002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/LinkHighlightImpl.cpp (right):

https://codereview.chromium.org/1452353002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/LinkHighlightImpl.cpp:56: #include
"ui/gfx/geometry/rect.h"
Is this legit to include from here?

[chrishtr, 2015-11-18 22:33:11]

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_imag...
File cc/layers/picture_image_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_imag...
cc/layers/picture_image_layer.cc:53: return gfx::Rect(0, 0, bounds().width(),
bounds().height());
On 2015/11/18 at 21:31:58, enne wrote:
> "return bounds()"?

Done.

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_laye...
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/layers/picture_laye...
cc/layers/picture_layer.cc:137: // TODO(chrishtr): Blink should no longer be
invalidating during paint.
On 2015/11/18 at 21:31:58, enne wrote:
> This isn't a thing to fix in this patch, but similarly we have some code in
Layer::Update about paint properties where Blink tries to resize layers during
paint.  If we can ignore those things too, it'd be a great cleanup.

Ok I'll do that in a followup, nice.

https://codereview.chromium.org/1452353002/diff/100001/cc/test/fake_content_l...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1452353002/diff/100001/cc/test/fake_content_l...
cc/test/fake_content_layer_client.cc:39: CHECK(bounds_set_);
On 2015/11/18 at 21:31:58, enne wrote:
> I get that you're trying to check that you covered everything.  But, not all
of these tests actually paint anything.
> 
> What do you think about just setting this to a default value of an empty rect
so that every test that uses a FakeContentLayerClient doesn't have to update
this value?

Per offline conversation, there were actually a whole bunch of tests that failed
that tried to draw tiles. I don't know the exact percentage, but I think it was
enough to make it reasonable to just leave it.

[enne (OOO), 2015-11-18 22:39:50]

lgtm

https://codereview.chromium.org/1452353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
(right):

https://codereview.chromium.org/1452353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp:87: //   
fprintf(stderr, "ContentLayerDelegate::paintableRegion\n");
<_<

[chrishtr, 2015-11-18 22:41:06]

https://codereview.chromium.org/1452353002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp
(right):

https://codereview.chromium.org/1452353002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/ContentLayerDelegate.cpp:87: //   
fprintf(stderr, "ContentLayerDelegate::paintableRegion\n");
On 2015/11/18 at 22:39:50, enne wrote:
> <_<

Done.

[chrishtr, 2015-11-18 22:45:58]

+esprehn FYI: I have whitelisted ui/gfx/geometry in order to use gfx::Rect in
the implementations of WebContentLayerClient, and remove the use of WebRect. I
discussed offline with Dana and Enne, and I think this is explicitly in line
with plans to use gfx::Rect in core in the future when these intermediate
interfaces go away. For now core/ still can't include gfx::Rect since we haven't
done all of the same inlining etc as IntRect has.

[chrishtr, 2015-11-19 18:59:08]

We can't commit this until other known bugs with Blink interest rects are
finished.
In particular, two bugs for interest rects of squashing layers.

[chrishtr, 2015-11-22 00:31:02]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-22 00:31:02]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1452353002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-11-22 00:32:36]

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

[commit-bot: I haz the power, 2015-11-22 00:40:46]

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

[commit-bot: I haz the power, 2015-11-22 00:40:47]

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

[chrishtr, 2015-11-22 02:06:54]

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

[chrishtr, 2015-11-22 02:06:55]

+sy for ui/compositor approval.

[sky, 2015-11-22 15:41:16]

LGTM

[sky, 2015-11-22 15:42:09]

https://codereview.chromium.org/1452353002/diff/180001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/1452353002/diff/180001/ui/compositor/layer.h#...
ui/compositor/layer.h:350: gfx::Rect PaintableRegion() override;
Just to make sure, this is the bounds of the parent and not the bounds of this,
right?

[chrishtr, 2015-11-23 16:52:03]

https://codereview.chromium.org/1452353002/diff/180001/ui/compositor/layer.h
File ui/compositor/layer.h (right):

https://codereview.chromium.org/1452353002/diff/180001/ui/compositor/layer.h#...
ui/compositor/layer.h:350: gfx::Rect PaintableRegion() override;
On 2015/11/22 at 15:42:09, sky wrote:
> Just to make sure, this is the bounds of the parent and not the bounds of
this, right?

It's the paintable bounds of this layer, in the local space of this layer.

[sky, 2015-11-23 17:58:46]

Ok

[chrishtr, 2015-11-23 18:01:02]

On 2015/11/23 at 17:58:46, sky wrote:
> Ok

Turns out bounds() was incorrect, since it is relative to the parent. I changed
it to return gfx::Rect(size()).

[chrishtr, 2015-11-23 18:01:05]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-23 18:01:05]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1452353002/#ps200001
(title: " ")

[sky, 2015-11-23 18:01:33]

I saw that in your latest patch, LGTM

On Mon, Nov 23, 2015 at 10:01 AM,  <chrishtr@chromium.org> wrote:
> On 2015/11/23 at 17:58:46, sky wrote:
>>
>> Ok
>
>
> Turns out bounds() was incorrect, since it is relative to the parent. I
> changed
> it to return gfx::Rect(size()).
>
> https://codereview.chromium.org/1452353002/

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

[sky, 2015-11-23 18:01:33]

I saw that in your latest patch, LGTM

On Mon, Nov 23, 2015 at 10:01 AM,  <chrishtr@chromium.org> wrote:
> On 2015/11/23 at 17:58:46, sky wrote:
>>
>> Ok
>
>
> Turns out bounds() was incorrect, since it is relative to the parent. I
> changed
> it to return gfx::Rect(size()).
>
> https://codereview.chromium.org/1452353002/

-- 
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.

[commit-bot: I haz the power, 2015-11-23 18:03:38]

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

[commit-bot: I haz the power, 2015-11-23 18:47:16]

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

[commit-bot: I haz the power, 2015-11-23 18:47:18]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7...)

[chrishtr, 2015-11-23 21:12:37]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-23 21:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1452353002/#ps220001
(title: " ")

[commit-bot: I haz the power, 2015-11-23 21:15:39]

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

[commit-bot: I haz the power, 2015-11-23 22:36:18]

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

[commit-bot: I haz the power, 2015-11-23 22:36:19]

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

[chrishtr, 2015-11-24 00:11:43]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-24 00:11:44]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1452353002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-11-24 00:14:25]

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

[commit-bot: I haz the power, 2015-11-24 01:18:34]

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

[commit-bot: I haz the power, 2015-11-24 01:18:35]

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

[chrishtr, 2015-11-24 01:34:00]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-11-24 01:34:01]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
enne@chromium.org
Link to the patchset: https://codereview.chromium.org/1452353002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2015-11-24 01:40:14]

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

[commit-bot: I haz the power, 2015-11-24 03:57:32]

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

[commit-bot: I haz the power, 2015-11-24 03:57:33]

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

[chrishtr, 2015-11-24 06:42:50]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-11-24 06:44:14]

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

[commit-bot: I haz the power, 2015-11-24 08:11:41]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-24 08:12:26]

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