Add cc::SurfaceInfo::is_valid()

Add cc::SurfaceInfo::is_valid()

SurfaceInfo::is_valid() returns true iff the SurfaceId is valid AND the device
scale factor is non-zero AND the frame size is not empty.

SurfaceInfo::is_valid() replaces obtaining and checking if the SurfaceId of a
SurfaceInfo object is valid. While that is a necessary condition of validity of
a SurfaceInfo, it is technically insufficient.

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

Review-Url: https://codereview.chromium.org/2720503008
Cr-Commit-Position: refs/heads/master@{#453998}
Committed: https://chromium.googlesource.com/chromium/src/+/9afc576bd3e06d83d6abc870000293b4be9d6132

[Alex Z., 2017-02-28 01:38:20]

Description was changed from

==========
Add cc::SurfaceInfo::is_valid()

SurfaceInfo::is_valid() returns true iff the SurfaceId is valid AND the device
scale factor is non-zero AND the frame size is not empty.

SurfaceInfo::is_valid() replaces obtaining and checking if the SurfaceId of a
SurfaceInfo object is valid. While that is a necessary condition of validity of
a SurfaceInfo, it is technically insufficient.

BUG=695420
==========

to

==========
Add cc::SurfaceInfo::is_valid()

SurfaceInfo::is_valid() returns true iff the SurfaceId is valid AND the device
scale factor is non-zero AND the frame size is not empty.

SurfaceInfo::is_valid() replaces obtaining and checking if the SurfaceId of a
SurfaceInfo object is valid. While that is a necessary condition of validity of
a SurfaceInfo, it is technically insufficient.

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

[Alex Z., 2017-02-28 01:40:11]

staraz@chromium.org changed reviewers:
+ danakj@chromium.org, sky@chromium.org

[Alex Z., 2017-02-28 01:40:12]

sky@chromium.org: Please review changes in services/ui, third_party and ui/aura

danakj@chromium.org: Please review changes in cc and ui/compositor

[Fady Samuel, 2017-02-28 01:43:54]

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

[commit-bot: I haz the power, 2017-02-28 01:44:21]

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

[Fady Samuel, 2017-02-28 01:44:41]

lgtm

[Alex Z., 2017-02-28 01:52:45]

staraz@chromium.org changed reviewers:
+ enne@chromium.org
- danakj@chromium.org

[Alex Z., 2017-02-28 01:52:45]

- danakj@ since she's OOO.

enne@: Please review changes in cc.

sky@: Please also review changes in ui/compositor

[Fady Samuel, 2017-02-28 01:56:10]

Actually, could you please update the StructTraits and ParamTraits too?

[Fady Samuel, 2017-02-28 01:57:03]

https://cs.chromium.org/chromium/src/cc/ipc/surface_info_struct_traits.h?q=su...

Deserialization should fail if SurfaceInfo is invalid. We should not be sending
invalid SurfaceInfos over IPC.

[Alex Z., 2017-02-28 02:30:58]

On 2017/02/28 01:57:03, Fady Samuel wrote:
>
https://cs.chromium.org/chromium/src/cc/ipc/surface_info_struct_traits.h?q=su...
> 
> Deserialization should fail if SurfaceInfo is invalid. We should not be
sending
> invalid SurfaceInfos over IPC.

I only found surface_info_struct_traits.h. Is there another file for
ParamTraits?

[Alex Z., 2017-02-28 02:31:32]

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

[Alex Z., 2017-02-28 02:31:33]

tsepez@: Please review changes in struct traits.

[Fady Samuel, 2017-02-28 02:54:32]

For SurfaceInfo ParamTraits, we currently use macros:

https://cs.chromium.org/chromium/src/cc/ipc/cc_param_traits_macros.h?l=202

You'll have to write custom paramtraits similar to this:

https://cs.chromium.org/chromium/src/cc/ipc/cc_param_traits.cc?l=629

https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
File cc/ipc/surface_info_struct_traits.h (right):

https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
cc/ipc/surface_info_struct_traits.h:30:
data.ReadSizeInPixels(&out->size_in_pixels_) && out->is_valid();
I'm not sure if compilers are guaranteed to process these conditions in order.

Do this:

out->device_scale_factor_ = data.device_scale_factor();
if (!data.ReadSurfaceId(&out->id_))
  return false;

if (!data.ReadSizeInPixels(&out->size_in_pixels_))
  return false;

return out->is_valid();

[sky, 2017-02-28 05:04:44]

LGTM

[Tom Sepez, 2017-02-28 17:52:02]

lgtm

https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
File cc/ipc/surface_info_struct_traits.h (right):

https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
cc/ipc/surface_info_struct_traits.h:30:
data.ReadSizeInPixels(&out->size_in_pixels_) && out->is_valid();
On 2017/02/28 02:54:32, Fady Samuel wrote:
> I'm not sure if compilers are guaranteed to process these conditions in order.
> 
> Do this:
> 
> out->device_scale_factor_ = data.device_scale_factor();
> if (!data.ReadSurfaceId(&out->id_))
>   return false;
> 
> if (!data.ReadSizeInPixels(&out->size_in_pixels_))
>   return false;
> 
> return out->is_valid();

This is fine; sequence points in C occur at the end of the evaluation of the
right operand of a && expression.

[enne (OOO), 2017-02-28 18:05:56]

lgtm

[Fady Samuel, 2017-02-28 18:12:08]

On 2017/02/28 17:52:02, Tom Sepez wrote:
> lgtm
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> File cc/ipc/surface_info_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> cc/ipc/surface_info_struct_traits.h:30:
> data.ReadSizeInPixels(&out->size_in_pixels_) && out->is_valid();
> On 2017/02/28 02:54:32, Fady Samuel wrote:
> > I'm not sure if compilers are guaranteed to process these conditions in
order.
> > 
> > Do this:
> > 
> > out->device_scale_factor_ = data.device_scale_factor();
> > if (!data.ReadSurfaceId(&out->id_))
> >   return false;
> > 
> > if (!data.ReadSizeInPixels(&out->size_in_pixels_))
> >   return false;
> > 
> > return out->is_valid();
> 
> This is fine; sequence points in C occur at the end of the evaluation of the
> right operand of a && expression.

Ohh you're right, heh, I don't know what I was thinking...thanks!

[Alex Z., 2017-02-28 21:26:57]

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

[commit-bot: I haz the power, 2017-02-28 21:27:33]

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

[Alex Z., 2017-02-28 21:27:50]

On 2017/02/28 02:54:32, Fady Samuel wrote:
> For SurfaceInfo ParamTraits, we currently use macros:
> 
> https://cs.chromium.org/chromium/src/cc/ipc/cc_param_traits_macros.h?l=202
> 
> You'll have to write custom paramtraits similar to this:
> 
> https://cs.chromium.org/chromium/src/cc/ipc/cc_param_traits.cc?l=629
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> File cc/ipc/surface_info_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> cc/ipc/surface_info_struct_traits.h:30:
> data.ReadSizeInPixels(&out->size_in_pixels_) && out->is_valid();
> I'm not sure if compilers are guaranteed to process these conditions in order.
> 
> Do this:
> 
> out->device_scale_factor_ = data.device_scale_factor();
> if (!data.ReadSurfaceId(&out->id_))
>   return false;
> 
> if (!data.ReadSizeInPixels(&out->size_in_pixels_))
>   return false;
> 
> return out->is_valid();

Done.

[Alex Z., 2017-02-28 21:28:28]

On 2017/02/28 17:52:02, Tom Sepez wrote:
> lgtm
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> File cc/ipc/surface_info_struct_traits.h (right):
> 
>
https://codereview.chromium.org/2720503008/diff/20001/cc/ipc/surface_info_str...
> cc/ipc/surface_info_struct_traits.h:30:
> data.ReadSizeInPixels(&out->size_in_pixels_) && out->is_valid();
> On 2017/02/28 02:54:32, Fady Samuel wrote:
> > I'm not sure if compilers are guaranteed to process these conditions in
order.
> > 
> > Do this:
> > 
> > out->device_scale_factor_ = data.device_scale_factor();
> > if (!data.ReadSurfaceId(&out->id_))
> >   return false;
> > 
> > if (!data.ReadSizeInPixels(&out->size_in_pixels_))
> >   return false;
> > 
> > return out->is_valid();
> 
> This is fine; sequence points in C occur at the end of the evaluation of the
> right operand of a && expression.

Please review changes in param traits.

[commit-bot: I haz the power, 2017-02-28 21:31:37]

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

[commit-bot: I haz the power, 2017-02-28 21:31:38]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Alex Z., 2017-02-28 21:38:21]

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

[commit-bot: I haz the power, 2017-02-28 21:39:02]

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-28 22:04:17]

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

[commit-bot: I haz the power, 2017-02-28 22:04:18]

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

[Alex Z., 2017-02-28 22:54:45]

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

[commit-bot: I haz the power, 2017-02-28 22:55:11]

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

[Alex Z., 2017-02-28 22:55:24]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-01 00:32:15]

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

[commit-bot: I haz the power, 2017-03-01 00:32:16]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-03-01 18:44:09]

lgtm

[Fady Samuel, 2017-03-01 18:44:55]

lgtm

[Alex Z., 2017-03-01 18:58:51]

The CQ bit was checked by staraz@chromium.org

[Alex Z., 2017-03-01 18:58:52]

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/2720503008/#ps80001
(title: "Added Log()")

[commit-bot: I haz the power, 2017-03-01 18:59:16]

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-03-01 19:09:21]

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

[commit-bot: I haz the power, 2017-03-01 19:09:22]

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

[Alex Z., 2017-03-01 19:12:17]

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

[Alex Z., 2017-03-01 19:12:18]

pdr@: Please review changes in WebKit

[pdr., 2017-03-01 19:16:01]

On 2017/03/01 at 19:12:18, staraz wrote:
> pdr@: Please review changes in WebKit

LGTM

[Alex Z., 2017-03-01 19:33:45]

The CQ bit was checked by staraz@chromium.org

[commit-bot: I haz the power, 2017-03-01 19:34:27]

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-03-01 19:39:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488396825754490,
"parent_rev": "569d20972fa8cd57b3f6b666c0c4e596ada50640", "commit_rev":
"9afc576bd3e06d83d6abc870000293b4be9d6132"}

[commit-bot: I haz the power, 2017-03-01 19:41:14]

Description was changed from

==========
Add cc::SurfaceInfo::is_valid()

SurfaceInfo::is_valid() returns true iff the SurfaceId is valid AND the device
scale factor is non-zero AND the frame size is not empty.

SurfaceInfo::is_valid() replaces obtaining and checking if the SurfaceId of a
SurfaceInfo object is valid. While that is a necessary condition of validity of
a SurfaceInfo, it is technically insufficient.

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

to

==========
Add cc::SurfaceInfo::is_valid()

SurfaceInfo::is_valid() returns true iff the SurfaceId is valid AND the device
scale factor is non-zero AND the frame size is not empty.

SurfaceInfo::is_valid() replaces obtaining and checking if the SurfaceId of a
SurfaceInfo object is valid. While that is a necessary condition of validity of
a SurfaceInfo, it is technically insufficient.

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

Review-Url: https://codereview.chromium.org/2720503008
Cr-Commit-Position: refs/heads/master@{#453998}
Committed:
https://chromium.googlesource.com/chromium/src/+/9afc576bd3e06d83d6abc8700002...
==========

[commit-bot: I haz the power, 2017-03-01 19:41:16]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9afc576bd3e06d83d6abc8700002...