Remove the clip parameter from ContentLayerClient.

Remove the clip parameter from ContentLayerClient.

The API now specifies that ContentLayerClient decides its own PaintableRegion().

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[chrishtr, 2015-12-01 21:23:50]

Description was changed from

==========
none


none

BUG=
==========

to

==========
none


none

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

[chrishtr, 2015-12-01 21:24:40]

Description was changed from

==========
none


none

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

to

==========
Remove the clip parameter from ContentLayerClient.

The API now specifies that ContentLayerClient decides its own PaintableRegion().

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

[chrishtr, 2015-12-01 21:25:31]

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

[enne (OOO), 2015-12-01 22:43:17]

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
cc/test/fake_content_layer_client.cc:58: item->SetNew(PaintableRegion(),
std::vector<SkRRect>());
I think you should either not append a clip item at all, or you should append a
clip to the valid painted bounds (or maybe Blink should?)

[enne (OOO), 2015-12-01 22:46:25]

https://codereview.chromium.org/1489713004/diff/1/cc/blink/web_content_layer_...
File cc/blink/web_content_layer_impl.cc (right):

https://codereview.chromium.org/1489713004/diff/1/cc/blink/web_content_layer_...
cc/blink/web_content_layer_impl.cc:72:
cc::DisplayItemList::Create(PaintableRegion(), settings);
Should this be set to the rect?

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
cc/test/fake_content_layer_client.cc:58: item->SetNew(PaintableRegion(),
std::vector<SkRRect>());
Oops, I guess this is in a test.  So, probably it should go away?

https://codereview.chromium.org/1489713004/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1489713004/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:767: cc::DisplayItemList::Create(PaintableRegion(),
settings);
Should this as well be the valid painted rect? Or, if it shouldn't be here,
should we remove this parameter?

[chrishtr, 2015-12-01 23:01:20]

https://codereview.chromium.org/1489713004/diff/1/cc/blink/web_content_layer_...
File cc/blink/web_content_layer_impl.cc (right):

https://codereview.chromium.org/1489713004/diff/1/cc/blink/web_content_layer_...
cc/blink/web_content_layer_impl.cc:72:
cc::DisplayItemList::Create(PaintableRegion(), settings);
On 2015/12/01 at 22:46:25, enne wrote:
> Should this be set to the rect?

Not sure what you mean here.

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
File cc/test/fake_content_layer_client.cc (right):

https://codereview.chromium.org/1489713004/diff/1/cc/test/fake_content_layer_...
cc/test/fake_content_layer_client.cc:58: item->SetNew(PaintableRegion(),
std::vector<SkRRect>());
On 2015/12/01 at 22:46:25, enne wrote:
> Oops, I guess this is in a test.  So, probably it should go away?

Removed.

https://codereview.chromium.org/1489713004/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/1489713004/diff/1/ui/compositor/layer.cc#newc...
ui/compositor/layer.cc:767: cc::DisplayItemList::Create(PaintableRegion(),
settings);
On 2015/12/01 at 22:46:25, enne wrote:
> Should this as well be the valid painted rect? Or, if it shouldn't be here,
should we remove this parameter?

Valid painted rect? Not sure what you mean.

Also, DisplayItemList has code to replay itself into an SkCanvas, which requires
bounds.

[enne (OOO), 2015-12-01 23:11:34]

In both of those cases the rect passed to DisplayItemLIst::Create, which
currently also adds a clip rect of that size and sets the canvas to that size. 
It's weird to pass an empty rect in both major callers of
DisplayItemList::Create.

It seems like you should pass the rect over which the display list has valid
painted contents.  There's some intersest rect that Blink has painted that's
valid to raster from that may be a subset of the size of the layer.  That's the
rect I mean.

[chrishtr, 2015-12-02 00:55:07]

On 2015/12/01 at 23:11:34, enne wrote:
> In both of those cases the rect passed to DisplayItemLIst::Create, which
currently also adds a clip rect of that size and sets the canvas to that size. 
It's weird to pass an empty rect in both major callers of
DisplayItemList::Create.
> 
> It seems like you should pass the rect over which the display list has valid
painted contents.  There's some intersest rect that Blink has painted that's
valid to raster from that may be a subset of the size of the layer.  That's the
rect I mean.

I am passing that rect, right? I'm passing PaintableRegion().

[enne (OOO), 2015-12-02 18:38:31]

OOPS.  I was misunderstanding what was going on here.  Ignore me!

lgtm

<_<

[chrishtr, 2015-12-02 18:42:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-12-02 18:43:08]

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

[commit-bot: I haz the power, 2015-12-02 18:54:51]

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

[commit-bot: I haz the power, 2015-12-02 18:54:52]

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-12-02 19:10:47]

sky@, need your LGTM.

[sky, 2015-12-02 20:43:58]

LGTM

[chrishtr, 2015-12-02 21:41:27]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-12-02 21:41:27]

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

[commit-bot: I haz the power, 2015-12-02 21:42:01]

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

[commit-bot: I haz the power, 2015-12-02 22:22:11]

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

[commit-bot: I haz the power, 2015-12-02 22:22:11]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2015-12-02 23:30:35]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-12-02 23:31:11]

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

[commit-bot: I haz the power, 2015-12-03 00:26:45]

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

[commit-bot: I haz the power, 2015-12-03 00:26:46]

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

[chrishtr, 2015-12-03 16:49:55]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2015-12-03 16:50:37]

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

[commit-bot: I haz the power, 2015-12-03 17:43:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-03 17:44:33]

Description was changed from

==========
Remove the clip parameter from ContentLayerClient.

The API now specifies that ContentLayerClient decides its own PaintableRegion().

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

to

==========
Remove the clip parameter from ContentLayerClient.

The API now specifies that ContentLayerClient decides its own PaintableRegion().

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/e8384fae6b671dc545561a2734873755e807b27d
Cr-Commit-Position: refs/heads/master@{#363005}
==========

[commit-bot: I haz the power, 2015-12-03 17:44:34]

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