Implemented StructTraits for cc::CopyOutputResult

Implemented StructTraits for cc::CopyOutputResult

MojoCompositorFrameSink needs to support copy requests, which means we need
mojo definition and struct traits for cc::CopyOutputResult in order to send it
over mojo.


BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2659703002
Cr-Commit-Position: refs/heads/master@{#447589}
Committed: https://chromium.googlesource.com/chromium/src/+/455475e2b6c5e7dffb2cc0ad8f86e37819deede7

[Saman Sami, 2017-01-26 20:38:05]

Description was changed from

==========
Implemented StructTraits for cc::CopyOutputResult

BUG=672071
==========

to

==========
Implemented StructTraits for cc::CopyOutputResult

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-01-26 20:39:00]

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

[commit-bot: I haz the power, 2017-01-26 20:39:42]

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-01-26 20:47:00]

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

[commit-bot: I haz the power, 2017-01-26 20:47:01]

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

[Saman Sami, 2017-01-31 16:53:36]

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

[commit-bot: I haz the power, 2017-01-31 16:53:59]

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-01-31 17:58:18]

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

[commit-bot: I haz the power, 2017-01-31 17:58:19]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-01-31 19:17:41]

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

[commit-bot: I haz the power, 2017-01-31 19:18:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-01-31 19:37:39]

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

[commit-bot: I haz the power, 2017-01-31 19:37:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-01-31 19:40:14]

Description was changed from

==========
Implemented StructTraits for cc::CopyOutputResult

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implemented StructTraits for cc::CopyOutputResult

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-01-31 19:40:14]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org, rockot@chromium.org, vmpstr@chromium.org

[Saman Sami, 2017-01-31 19:40:54]

Description was changed from

==========
Implemented StructTraits for cc::CopyOutputResult

BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implemented StructTraits for cc::CopyOutputResult

MojoCompositorFrameSink needs to support copy requests, which means we need
mojo definition and struct traits for cc::CopyOutputResult in order to send it
over mojo.


BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-01-31 19:42:43]

rockot@: Please review mojo
vmpstr@: Please review cc/

[Fady Samuel, 2017-01-31 19:44:57]

lgtm

[Ken Rockot(use gerrit already), 2017-01-31 20:16:35]

Mojo LGTM

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.mojom (right):

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.mojom:7: import "cc/ipc/texture_mailbox.mojom";
Please add //cc/ipc:interfaces to this target's public_deps

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.mojom:12: // Note: release_callback_ is not included.
nit: I don't think this detail is helpful or necessary to point out

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.typemap (right):

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.typemap:6: public_headers = [
"//cc/output/copy_output_result.h" ]
I think you also want to add public_deps = [ "//cc" ] ?

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.h (right):

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap_;
Non-POD static variables are strictly forbidden by the style guide (see
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables).

Please use a base::LazyInstance instead, i.e.

  static base::LazyInstance<SkBitmap> null_bitmap;
  if (!result.bitmap_)
    return null_bitmap.Get();
  return *result.bitmap_;

also naming nit: no trailing underscore, it's not a member variable.

[Ken Rockot(use gerrit already), 2017-01-31 20:17:16]

On 2017/01/31 at 20:16:35, Ken Rockot wrote:
> Mojo LGTM
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> File cc/ipc/copy_output_result.mojom (right):
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> cc/ipc/copy_output_result.mojom:7: import "cc/ipc/texture_mailbox.mojom";
> Please add //cc/ipc:interfaces to this target's public_deps
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> cc/ipc/copy_output_result.mojom:12: // Note: release_callback_ is not
included.
> nit: I don't think this detail is helpful or necessary to point out
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> File cc/ipc/copy_output_result.typemap (right):
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> cc/ipc/copy_output_result.typemap:6: public_headers = [
"//cc/output/copy_output_result.h" ]
> I think you also want to add public_deps = [ "//cc" ] ?
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> File cc/ipc/copy_output_result_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
> cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap_;
> Non-POD static variables are strictly forbidden by the style guide (see
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables).
> 
> Please use a base::LazyInstance instead, i.e.
> 
>   static base::LazyInstance<SkBitmap> null_bitmap;

That should be

    static base::LazyInstance<SkBitmap> null_bitmap = LAZY_INSTANCE_INITIALIZER;

>   if (!result.bitmap_)
>     return null_bitmap.Get();
>   return *result.bitmap_;
> 
> also naming nit: no trailing underscore, it's not a member variable.

[Ken Rockot(use gerrit already), 2017-01-31 20:20:09]

Actually, somewhat serendipitously, this thread has been happening:
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

It sounds like as of an hour ago, it's safe to use local static variables.
So please feel free to ignore my suggestion about LazyInstance!

On Tue, Jan 31, 2017 at 12:17 PM, <rockot@chromium.org> wrote:

> On 2017/01/31 at 20:16:35, Ken Rockot wrote:
> > Mojo LGTM
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result.mojom
> > File cc/ipc/copy_output_result.mojom (right):
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result.mojom#newcode7
> > cc/ipc/copy_output_result.mojom:7: import "cc/ipc/texture_mailbox.mojom"
> ;
> > Please add //cc/ipc:interfaces to this target's public_deps
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result.mojom#newcode12
> > cc/ipc/copy_output_result.mojom:12: // Note: release_callback_ is not
> included.
> > nit: I don't think this detail is helpful or necessary to point out
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result.typemap
> > File cc/ipc/copy_output_result.typemap (right):
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result.typemap#newcode6
> > cc/ipc/copy_output_result.typemap:6: public_headers = [
> "//cc/output/copy_output_result.h" ]
> > I think you also want to add public_deps = [ "//cc" ] ?
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result_struct_traits.h
> > File cc/ipc/copy_output_result_struct_traits.h (right):
> >
> >
> https://codereview.chromium.org/2659703002/diff/120001/cc/ip
> c/copy_output_result_struct_traits.h#newcode20
> > cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap
> null_bitmap_;
> > Non-POD static variables are strictly forbidden by the style guide (see
> https://google.github.io/styleguide/cppguide.html#Static_
> and_Global_Variables).
> >
> > Please use a base::LazyInstance instead, i.e.
> >
> > static base::LazyInstance<SkBitmap> null_bitmap;
>
> That should be
>
> static base::LazyInstance<SkBitmap> null_bitmap =
> LAZY_INSTANCE_INITIALIZER;
>
> > if (!result.bitmap_)
> > return null_bitmap.Get();
> > return *result.bitmap_;
> >
> > also naming nit: no trailing underscore, it's not a member variable.
>
>
>
> https://codereview.chromium.org/2659703002/
>

-- 
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, 2017-01-31 20:34:08]

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

[commit-bot: I haz the power, 2017-01-31 20:34:09]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-01-31 20:47:57]

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.mojom (right):

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.mojom:12: // Note: release_callback_ is not included.
On 2017/01/31 20:16:35, Ken Rockot wrote:
> nit: I don't think this detail is helpful or necessary to point out

Done.

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.typemap (right):

https://codereview.chromium.org/2659703002/diff/120001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.typemap:6: public_headers = [
"//cc/output/copy_output_result.h" ]
On 2017/01/31 20:16:35, Ken Rockot wrote:
> I think you also want to add public_deps = [ "//cc" ] ?

Done.

[Saman Sami, 2017-01-31 20:48:05]

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

[commit-bot: I haz the power, 2017-01-31 20:48:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-01-31 21:32:19]

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.mojom (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.mojom:14: skia.mojom.Bitmap? bitmap;
is '?' a mojom for optional?

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap;
nit: empty_bitmap

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/struct_traits_u...
File cc/ipc/struct_traits_unittest.cc (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/struct_traits_u...
cc/ipc/struct_traits_unittest.cc:436: // Texture test
nit: Can you create a different TEST_F for this (even though there will be some
boilerplate code)

https://codereview.chromium.org/2659703002/diff/140001/cc/output/copy_output_...
File cc/output/copy_output_result.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/output/copy_output_...
cc/output/copy_output_result.h:48: CopyOutputResult&
operator=(CopyOutputResult&& other);
Should this come with a corresponding move ctor?

[Ken Rockot(use gerrit already), 2017-01-31 21:46:20]

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.mojom (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.mojom:14: skia.mojom.Bitmap? bitmap;
On 2017/01/31 at 21:32:19, vmpstr wrote:
> is '?' a mojom for optional?

Drive-by answer: Yes

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap;
On 2017/01/31 at 21:32:19, vmpstr wrote:
> nit: empty_bitmap

Per the ongoing discussion linked earlier, please also make this a SkBitmap* and
allocate it on the heap so it just leaks on shutdown. We don't want local
statics running destructors on shutdown.

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:31: static bool
Read(cc::mojom::CopyOutputResultDataView data,
nit: (sorry, overlooked this earlier) Please define this out-of-line by moving
it to a cc file. Same for bitmap() since it's non-trivial.

[commit-bot: I haz the power, 2017-02-01 00:05:43]

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

[commit-bot: I haz the power, 2017-02-01 00:05:44]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-02-01 00:44:41]

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

[commit-bot: I haz the power, 2017-02-01 00:45:30]

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-01 00:50:36]

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

[commit-bot: I haz the power, 2017-02-01 00:50:37]

Dry run: 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...)

[Saman Sami, 2017-02-01 01:33:59]

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

[commit-bot: I haz the power, 2017-02-01 01:34:46]

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-01 01:40:39]

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

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

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-02-01 15:38:58]

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

[commit-bot: I haz the power, 2017-02-01 15:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-01 15:44:43]

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

[commit-bot: I haz the power, 2017-02-01 15:44:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-01 15:49:57]

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

[commit-bot: I haz the power, 2017-02-01 15:50:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-01 15:51:58]

PTAL

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap;
On 2017/01/31 21:32:19, vmpstr wrote:
> nit: empty_bitmap

This is a null bitmap though. IsNull() returns true. Mojo also uses the term
"null".

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap;
On 2017/01/31 21:46:20, Ken Rockot wrote:
> On 2017/01/31 at 21:32:19, vmpstr wrote:
> > nit: empty_bitmap
> 
> Per the ongoing discussion linked earlier, please also make this a SkBitmap*
and
> allocate it on the heap so it just leaks on shutdown. We don't want local
> statics running destructors on shutdown.

Done.

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:31: static bool
Read(cc::mojom::CopyOutputResultDataView data,
On 2017/01/31 21:46:20, Ken Rockot wrote:
> nit: (sorry, overlooked this earlier) Please define this out-of-line by moving
> it to a cc file. Same for bitmap() since it's non-trivial.

Done.

https://codereview.chromium.org/2659703002/diff/140001/cc/output/copy_output_...
File cc/output/copy_output_result.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/output/copy_output_...
cc/output/copy_output_result.h:48: CopyOutputResult&
operator=(CopyOutputResult&& other);
On 2017/01/31 21:32:19, vmpstr wrote:
> Should this come with a corresponding move ctor?

Done.

[Ken Rockot(use gerrit already), 2017-02-01 16:04:55]

Still LGTM, with some nits

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.h (right):

https://codereview.chromium.org/2659703002/diff/140001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.h:20: static SkBitmap null_bitmap;
On 2017/02/01 at 15:51:57, Saman Sami wrote:
> On 2017/01/31 21:32:19, vmpstr wrote:
> > nit: empty_bitmap
> 
> This is a null bitmap though. IsNull() returns true. Mojo also uses the term
"null".

And perhaps more importantly, IsNull() returns true iff SkBitmap::is_null
returns true. So the meaning of "null" is consistent here. Furthermore, SkBitmap
also defines empty() which does not mean the same thing.

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/BUILD.gn
File cc/ipc/BUILD.gn (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/BUILD.gn#newcod...
cc/ipc/BUILD.gn:134: "//skia/public/interfaces:interfaces",
nit: "//foo/bar" implies "//foo/bar:bar" so ":interfaces" here is redundant and
should be omitted

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.typemap (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.typemap:8: deps = [
nit: AFAICT public_deps would be more correct here since these dependencies are
exposed by the traits header

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.cc (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.cc:21: static SkBitmap* null_bitmap =
nullptr;
no need for this and conditional initialization. you can just

  static SkBitmap* null_bitmap = new SkBitmap;

and the right thing will happen (i.e. a new SkBitmap will be allocated exactly
once, on the first call to this function)

[commit-bot: I haz the power, 2017-02-01 16:43:53]

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

[commit-bot: I haz the power, 2017-02-01 16:43:54]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-02-01 17:16:25]

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

[commit-bot: I haz the power, 2017-02-01 17:16:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-02-01 17:19:41]

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/BUILD.gn
File cc/ipc/BUILD.gn (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/BUILD.gn#newcod...
cc/ipc/BUILD.gn:134: "//skia/public/interfaces:interfaces",
On 2017/02/01 16:04:55, Ken Rockot wrote:
> nit: "//foo/bar" implies "//foo/bar:bar" so ":interfaces" here is redundant
and
> should be omitted

Done.

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result.typemap (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result.typemap:8: deps = [
On 2017/02/01 16:04:55, Ken Rockot wrote:
> nit: AFAICT public_deps would be more correct here since these dependencies
are
> exposed by the traits header

Done.

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
File cc/ipc/copy_output_result_struct_traits.cc (right):

https://codereview.chromium.org/2659703002/diff/240001/cc/ipc/copy_output_res...
cc/ipc/copy_output_result_struct_traits.cc:21: static SkBitmap* null_bitmap =
nullptr;
On 2017/02/01 16:04:55, Ken Rockot wrote:
> no need for this and conditional initialization. you can just
> 
>   static SkBitmap* null_bitmap = new SkBitmap;
> 
> and the right thing will happen (i.e. a new SkBitmap will be allocated exactly
> once, on the first call to this function)

Done.

[commit-bot: I haz the power, 2017-02-01 18:21:40]

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

[commit-bot: I haz the power, 2017-02-01 18:21:42]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-02-01 18:49:55]

cc lgtm

[Saman Sami, 2017-02-01 18:50:44]

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

[commit-bot: I haz the power, 2017-02-01 18:51:55]

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-01 18:56:55]

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

[commit-bot: I haz the power, 2017-02-01 18:56:56]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-02-01 19:00:07]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-02-01 19:00:07]

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

[commit-bot: I haz the power, 2017-02-01 19:00:31]

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-01 19:10:17]

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

[commit-bot: I haz the power, 2017-02-01 19:10:19]

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

[Saman Sami, 2017-02-01 19:26:14]

samans@chromium.org changed reviewers:
+ tsepez@chromium.org

[Saman Sami, 2017-02-01 19:33:38]

tsepez@: Please review mojo for security.

[Tom Sepez, 2017-02-01 20:05:02]

mojom LGTM

[Saman Sami, 2017-02-01 20:07:28]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-02-01 20:07:46]

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-01 20:12:34]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1485979648352170,
"parent_rev": "61d41e654177c4891cdc344dcf03446c150c5695", "commit_rev":
"455475e2b6c5e7dffb2cc0ad8f86e37819deede7"}

[commit-bot: I haz the power, 2017-02-01 20:13:14]

Description was changed from

==========
Implemented StructTraits for cc::CopyOutputResult

MojoCompositorFrameSink needs to support copy requests, which means we need
mojo definition and struct traits for cc::CopyOutputResult in order to send it
over mojo.


BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Implemented StructTraits for cc::CopyOutputResult

MojoCompositorFrameSink needs to support copy requests, which means we need
mojo definition and struct traits for cc::CopyOutputResult in order to send it
over mojo.


BUG=672071
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2659703002
Cr-Commit-Position: refs/heads/master@{#447589}
Committed:
https://chromium.googlesource.com/chromium/src/+/455475e2b6c5e7dffb2cc0ad8f86...
==========

[commit-bot: I haz the power, 2017-02-01 20:13:15]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/455475e2b6c5e7dffb2cc0ad8f86...