Add support for converting cc::Region to and from protobuf.

Add support for converting cc::Region to and from protobuf.

For the (de)serialization of property trees and layers, we need to be
able to serialize several data types and this CL adds conversions and
a unit test for the cc::Region type.

The conversion of cc::Region happens by serializing a list of
gfx::Rect objects, and then on the deserialization side the union
of the gfx::Rect objects are created to recreate the cc::Region.

The initial version of this CL used writeToMemory and readFromMemory
in Skia, and a suggestion was made to fix using uint8_t instead of
char[], and this change still keeps this fix, even after those methods
are not in use anymore.

This is done in a separate CL to keep the logic-changing CLs clean
and focused.

BUG=538710
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/04b8bd2ea68f22f8051d9988e4872e51c4ab4409
Cr-Commit-Position: refs/heads/master@{#361302}

[nyquist, 2015-11-19 02:05:10]

nyquist@chromium.org changed reviewers:
+ dtrainor@chromium.org, vmpstr@chromium.org

[nyquist, 2015-11-19 02:05:11]

vmpstr, dtrainor: PTAL

This ended up being a little bit weird DEPS-wise sadly.

[nyquist, 2015-11-19 02:05:19]

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

[commit-bot: I haz the power, 2015-11-19 02:08:05]

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

[commit-bot: I haz the power, 2015-11-19 02:13:02]

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

[commit-bot: I haz the power, 2015-11-19 02:13:02]

Dry run: 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...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[David Trainor- moved to gerrit, 2015-11-19 19:14:01]

nits

https://chromiumcodereview.appspot.com/1460503004/diff/1/cc/base/DEPS
File cc/base/DEPS (right):

https://chromiumcodereview.appspot.com/1460503004/diff/1/cc/base/DEPS#newcode1
cc/base/DEPS:1: # Things within cc/base should not depend on things in cc/
outside of cc/base except protocol buffers.
Should this wrap?

https://chromiumcodereview.appspot.com/1460503004/diff/1/cc/base/region.cc
File cc/base/region.cc (right):

https://chromiumcodereview.appspot.com/1460503004/diff/1/cc/base/region.cc#ne...
cc/base/region.cc:147: scoped_ptr<char[]> buffer(new char[region_size]);
uint8_t?  Again same for the DisplayItem protos :(.  Thanks!

https://chromiumcodereview.appspot.com/1460503004/diff/1/cc/base/region.cc#ne...
cc/base/region.cc:156: size_t bytes_read =
region.readFromMemory(proto.skregion().c_str(),
.data?  Could you also do this for the DisplayItem* protos?  Thanks!

[nyquist, 2015-11-20 01:11:43]

dtrainor: PTAL

https://codereview.chromium.org/1460503004/diff/1/cc/base/DEPS
File cc/base/DEPS (right):

https://codereview.chromium.org/1460503004/diff/1/cc/base/DEPS#newcode1
cc/base/DEPS:1: # Things within cc/base should not depend on things in cc/
outside of cc/base except protocol buffers.
On 2015/11/19 19:14:01, David Trainor wrote:
> Should this wrap?

Done.

https://codereview.chromium.org/1460503004/diff/1/cc/base/region.cc
File cc/base/region.cc (right):

https://codereview.chromium.org/1460503004/diff/1/cc/base/region.cc#newcode147
cc/base/region.cc:147: scoped_ptr<char[]> buffer(new char[region_size]);
On 2015/11/19 19:14:01, David Trainor wrote:
> uint8_t?  Again same for the DisplayItem protos :(.  Thanks!

Done.

https://codereview.chromium.org/1460503004/diff/1/cc/base/region.cc#newcode156
cc/base/region.cc:156: size_t bytes_read =
region.readFromMemory(proto.skregion().c_str(),
On 2015/11/19 19:14:01, David Trainor wrote:
> .data?  Could you also do this for the DisplayItem* protos?  Thanks!

Done.

[David Trainor- moved to gerrit, 2015-11-20 21:46:50]

lgtm with a few small checked_cast nits.

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.cc
File cc/base/region.cc (right):

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.c...
cc/base/region.cc:149: proto->set_skregion(buffer.get(), region_size);
base::checked_cast<int>?  IIRC set_skregion() will take an int for the size.

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/playback/clip...
File cc/playback/clip_path_display_item.cc (right):

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/playback/clip...
cc/playback/clip_path_display_item.cc:46: details->set_clip_path(buffer.get(),
path_size);
base::checked_cast<int>?

[danakj, 2015-11-20 21:48:08]

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
File cc/base/region.h (right):

https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
I think we should treat cc::Region the same way we treat gfx::Rect (ie we didn't
add methods directly to those classes yet), because we probably will move this
to ui/gfx/geometry to share with blink, and I'd prefer not to race with fixing
proto support there.

[David Trainor- moved to gerrit, 2015-11-20 21:52:32]

On 2015/11/20 21:48:08, danakj (behind on reviews) wrote:
> https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
> File cc/base/region.h (right):
> 
>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
> cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
> I think we should treat cc::Region the same way we treat gfx::Rect (ie we
didn't
> add methods directly to those classes yet), because we probably will move this
> to ui/gfx/geometry to share with blink, and I'd prefer not to race with fixing
> proto support there.

That should be doable.  Would it be possible to expose the SkRegion from Region
in order to serialize it?

[David Trainor- moved to gerrit, 2015-11-20 21:52:53]

On 2015/11/20 21:52:32, David Trainor wrote:
> On 2015/11/20 21:48:08, danakj (behind on reviews) wrote:
> >
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
> > File cc/base/region.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
> > cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
> > I think we should treat cc::Region the same way we treat gfx::Rect (ie we
> didn't
> > add methods directly to those classes yet), because we probably will move
this
> > to ui/gfx/geometry to share with blink, and I'd prefer not to race with
fixing
> > proto support there.
> 
> That should be doable.  Would it be possible to expose the SkRegion from
Region
> in order to serialize it?

Err, obviously possible, but would you mind if we exposed it? :)

[danakj, 2015-11-20 21:56:42]

On Fri, Nov 20, 2015 at 1:52 PM, <dtrainor@chromium.org> wrote:

> On 2015/11/20 21:48:08, danakj (behind on reviews) wrote:
>
>>
>>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
>> File cc/base/region.h (right):
>>
>
>
>
>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
>
>> cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
>> I think we should treat cc::Region the same way we treat gfx::Rect (ie we
>>
> didn't
>
>> add methods directly to those classes yet), because we probably will move
>> this
>> to ui/gfx/geometry to share with blink, and I'd prefer not to race with
>> fixing
>> proto support there.
>>
>
> That should be doable.  Would it be possible to expose the SkRegion from
> Region
> in order to serialize it?
>

I'd prefer if you used the iterator interface.

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

[nyquist, 2015-11-20 22:45:31]

On 2015/11/20 21:56:42, danakj (behind on reviews) wrote:
> On Fri, Nov 20, 2015 at 1:52 PM, <mailto:dtrainor@chromium.org> wrote:
> 
> > On 2015/11/20 21:48:08, danakj (behind on reviews) wrote:
> >
> >>
> >>
> https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
> >> File cc/base/region.h (right):
> >>
> >
> >
> >
> >
>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
> >
> >> cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
> >> I think we should treat cc::Region the same way we treat gfx::Rect (ie we
> >>
> > didn't
> >
> >> add methods directly to those classes yet), because we probably will move
> >> this
> >> to ui/gfx/geometry to share with blink, and I'd prefer not to race with
> >> fixing
> >> proto support there.
> >>
> >
> > That should be doable.  Would it be possible to expose the SkRegion from
> > Region
> > in order to serialize it?
> >
> 
> I'd prefer if you used the iterator interface.

Happy to use that for serialization.

I'm a little bit unclear on how to put it all back together again though.
Specifically, from SkRegion to cc::Region?

The list of rects contained in the SkRegion are serializable, and we can even
till use the writeToMemory and readFromMemory methods, but I can't find any
constructors or setters that would help me go from SkRegion to cc:Region. I was
thinking about using Union, but I'm not 100% certain that Union is always the
right thing to do when I deserialize.

[vmpstr, 2015-11-20 23:10:52]

On 2015/11/20 22:45:31, nyquist wrote:
> On 2015/11/20 21:56:42, danakj (behind on reviews) wrote:
> > On Fri, Nov 20, 2015 at 1:52 PM, <mailto:dtrainor@chromium.org> wrote:
> > 
> > > On 2015/11/20 21:48:08, danakj (behind on reviews) wrote:
> > >
> > >>
> > >>
> >
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h
> > >> File cc/base/region.h (right):
> > >>
> > >
> > >
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/1460503004/diff/20001/cc/base/region.h...
> > >
> > >> cc/base/region.h:72: void ToProtobuf(proto::Region* proto) const;
> > >> I think we should treat cc::Region the same way we treat gfx::Rect (ie we
> > >>
> > > didn't
> > >
> > >> add methods directly to those classes yet), because we probably will move
> > >> this
> > >> to ui/gfx/geometry to share with blink, and I'd prefer not to race with
> > >> fixing
> > >> proto support there.
> > >>
> > >
> > > That should be doable.  Would it be possible to expose the SkRegion from
> > > Region
> > > in order to serialize it?
> > >
> > 
> > I'd prefer if you used the iterator interface.
> 
> Happy to use that for serialization.
> 
> I'm a little bit unclear on how to put it all back together again though.
> Specifically, from SkRegion to cc::Region?
> 
> The list of rects contained in the SkRegion are serializable, and we can even
> till use the writeToMemory and readFromMemory methods, but I can't find any
> constructors or setters that would help me go from SkRegion to cc:Region. I
was
> thinking about using Union, but I'm not 100% certain that Union is always the
> right thing to do when I deserialize.

The list of rects that the iterator returns should be disjoint rects that when
unioned together would reproduce the same region. So I think you just want to
store a list of rects, and then to reproduce it just union all of those together
(with ample testing of course :) ).

[nyquist, 2015-11-21 01:30:24]

Description was changed from

==========
Add support for converting cc::Region to and from protobuf.

For the (de)serialization of property trees and layers, we need to be
able to serialize several data types and this CL adds conversions and
a unit test for the cc::Region type.

This type depends directly on the SkRegion serialization using the
writeToMemory(...) and readFromMemory(...) methods.

This is done in a separate CL to keep the logic-changing CLs clean
and focused.

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

to

==========
Add support for converting cc::Region to and from protobuf.

For the (de)serialization of property trees and layers, we need to be
able to serialize several data types and this CL adds conversions and
a unit test for the cc::Region type.

The conversion of cc::Region happens by serializing a list of
gfx::Rect objects, and then on the deserialization side the union
of the gfx::Rect objects are created to recreate the cc::Region.

The initial version of this CL used writeToMemory and readFromMemory
in Skia, and a suggestion was made to fix using uint8_t instead of
char[], and this change still keeps this fix, even after those methods
are not in use anymore.

This is done in a separate CL to keep the logic-changing CLs clean
and focused.

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

[nyquist, 2015-11-21 01:34:03]

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

[commit-bot: I haz the power, 2015-11-21 01:36:47]

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

[commit-bot: I haz the power, 2015-11-21 02:42:52]

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

[commit-bot: I haz the power, 2015-11-21 02:42:53]

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

[nyquist, 2015-11-23 17:29:12]

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

[commit-bot: I haz the power, 2015-11-23 17:30:30]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[nyquist, 2015-11-23 20:58:04]

vmpstr (danakj): PTAL

[vmpstr, 2015-11-23 21:10:04]

https://codereview.chromium.org/1460503004/diff/80001/cc/proto/cc_conversions...
File cc/proto/cc_conversions_unittest.cc (right):

https://codereview.chromium.org/1460503004/diff/80001/cc/proto/cc_conversions...
cc/proto/cc_conversions_unittest.cc:39: }
If you have a region that's constructed as a union of two overlapping rects,
does that pass the test?

[nyquist, 2015-11-23 22:31:29]

https://codereview.chromium.org/1460503004/diff/80001/cc/proto/cc_conversions...
File cc/proto/cc_conversions_unittest.cc (right):

https://codereview.chromium.org/1460503004/diff/80001/cc/proto/cc_conversions...
cc/proto/cc_conversions_unittest.cc:39: }
On 2015/11/23 21:10:04, vmpstr wrote:
> If you have a region that's constructed as a union of two overlapping rects,
> does that pass the test?

Yes, that works. Added test for this. Thanks!

How these rects are supposed to work is not totally clear to me, so please tell
me if there are more tests you think would be helpful.

[nyquist, 2015-11-23 22:31:44]

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

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

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

[vmpstr, 2015-11-23 23:09:21]

lgtm

https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
File cc/proto/cc_conversions_unittest.cc (right):

https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
cc/proto/cc_conversions_unittest.cc:41: TEST(RegionTest,
OverlappingRectUnionProtoConversion) {
I'd also add one

Region region(gfx::Rect(0, 0, 10, 10));
region.Subtract(gfx::Rect(5, 5, 1, 1));

[nyquist, 2015-11-23 23:37:35]

danakj: PTAL. I understand you're behind on reviews, so unless you disagree I
plan on sending this through the CQ tomorrow (Tuesday). If you haven't gotten to
this CL before that, feel free to still comment on this CL, and I'll do a
follow-up to address it.

https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
File cc/proto/cc_conversions_unittest.cc (right):

https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
cc/proto/cc_conversions_unittest.cc:41: TEST(RegionTest,
OverlappingRectUnionProtoConversion) {
On 2015/11/23 23:09:20, vmpstr wrote:
> I'd also add one
> 
> Region region(gfx::Rect(0, 0, 10, 10));
> region.Subtract(gfx::Rect(5, 5, 1, 1));

Done.

[nyquist, 2015-11-23 23:41:51]

The CQ bit was checked by nyquist@chromium.org

[nyquist, 2015-11-23 23:41:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1460503004/#ps120001
(title: "Added a test for subtraction of overlapping rects")

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

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

[danakj, 2015-11-23 23:46:50]

On Mon, Nov 23, 2015 at 3:37 PM, <nyquist@chromium.org> wrote:

> danakj: PTAL. I understand you're behind on reviews, so unless you
> disagree I
> plan on sending this through the CQ tomorrow (Tuesday). If you haven't
> gotten to
> this CL before that, feel free to still comment on this CL, and I'll do a
> follow-up to address it.


yup, i just didn't want to the API to Region, so it matches how gfx types
were done. I'm happy if Vlad's happy otherwise.


>
>
>
>
>
https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
> File cc/proto/cc_conversions_unittest.cc (right):
>
>
>
https://codereview.chromium.org/1460503004/diff/100001/cc/proto/cc_conversion...
> cc/proto/cc_conversions_unittest.cc:41: TEST(RegionTest,
> OverlappingRectUnionProtoConversion) {
> On 2015/11/23 23:09:20, vmpstr wrote:
>
>> I'd also add one
>>
>
> Region region(gfx::Rect(0, 0, 10, 10));
>> region.Subtract(gfx::Rect(5, 5, 1, 1));
>>
>
> Done.
>
> https://codereview.chromium.org/1460503004/
>

-- 
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-24 02:44:41]

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

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

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

[nyquist, 2015-11-24 03:50:01]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2015-11-24 03:50:44]

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

[commit-bot: I haz the power, 2015-11-24 05:17:34]

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

[commit-bot: I haz the power, 2015-11-24 05:17:35]

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

[nyquist, 2015-11-24 09:27:09]

The CQ bit was checked by nyquist@chromium.org

[commit-bot: I haz the power, 2015-11-24 09:27:57]

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

[commit-bot: I haz the power, 2015-11-24 10:04:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-24 10:05:23]

Patchset 7 (id:??) landed as
https://crrev.com/04b8bd2ea68f22f8051d9988e4872e51c4ab4409
Cr-Commit-Position: refs/heads/master@{#361302}