Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[xlai (Olivia), 2016-06-06 17:08:36]

Description was changed from

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(third_party/WebKit/public/platform/modules/offscreencanvas/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

BUG=None
==========

to

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(third_party/WebKit/public/platform/modules/offscreencanvas/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

[xlai (Olivia), 2016-06-06 17:23:34]

xlai@chromium.org changed reviewers:
+ fsamuel@chromium.org

[xlai (Olivia), 2016-06-06 17:23:34]

Please take a look at this. Thanks!

[Fady Samuel, 2016-06-06 17:38:39]

Could you please write a test?

[xlai (Olivia), 2016-06-06 18:14:25]

Adding unit test to check whether SurfaceSequence type conversion is done
properly.

[Fady Samuel, 2016-06-06 18:34:30]

lgtm once comments addressed. Thanks. Please send to tsepez@ for security
review.

https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
File cc/ipc/surface_sequence.typemap (right):

https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
cc/ipc/surface_sequence.typemap:8: deps = [ "//cc/surfaces:surface_sequence", ]
Maybe just depend on cc/surfaces?

https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn
File cc/surfaces/BUILD.gn (right):

https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn#ne...
cc/surfaces/BUILD.gn:15: source_set("surface_sequence") {
Do we need this? Can't you just dep on all of surfaces?

[xlai (Olivia), 2016-06-06 19:04:32]

https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
File cc/ipc/surface_sequence.typemap (right):

https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
cc/ipc/surface_sequence.typemap:8: deps = [ "//cc/surfaces:surface_sequence", ]
On 2016/06/06 18:34:30, Fady Samuel wrote:
> Maybe just depend on cc/surfaces?

This is to be consistent as surface_id.typemap. Also, I think it's better to
have minimal deps.

https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn
File cc/surfaces/BUILD.gn (right):

https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn#ne...
cc/surfaces/BUILD.gn:15: source_set("surface_sequence") {
On 2016/06/06 18:34:30, Fady Samuel wrote:
> Do we need this? Can't you just dep on all of surfaces?

Same rationale as above. I think it's better to isolate surface_id and
surface_sequence, because that's all I need. Adding others may again introduce a
whole bunch of other dependent targets unnecessarily.

[Fady Samuel, 2016-06-06 22:16:28]

On 2016/06/06 19:04:32, xlai (Olivia) wrote:
>
https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
> File cc/ipc/surface_sequence.typemap (right):
> 
>
https://codereview.chromium.org/2043833002/diff/40001/cc/ipc/surface_sequence...
> cc/ipc/surface_sequence.typemap:8: deps = [ "//cc/surfaces:surface_sequence",
]
> On 2016/06/06 18:34:30, Fady Samuel wrote:
> > Maybe just depend on cc/surfaces?
> 
> This is to be consistent as surface_id.typemap. Also, I think it's better to
> have minimal deps.
> 
> https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn
> File cc/surfaces/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2043833002/diff/40001/cc/surfaces/BUILD.gn#ne...
> cc/surfaces/BUILD.gn:15: source_set("surface_sequence") {
> On 2016/06/06 18:34:30, Fady Samuel wrote:
> > Do we need this? Can't you just dep on all of surfaces?
> 
> Same rationale as above. I think it's better to isolate surface_id and
> surface_sequence, because that's all I need. Adding others may again introduce
a
> whole bunch of other dependent targets unnecessarily.

sigh OK. All this still will have to be refactored eventually. surfaces needs to
be diced into client/service/common pieces. I'll deal with it.

[xlai (Olivia), 2016-06-06 22:28:24]

xlai@chromium.org changed reviewers:
+ enne@chromium.org, tsepez@chromium.org

[xlai (Olivia), 2016-06-06 22:28:25]

Adding tsepez@ for security review and enne@ for edits to cc/ folder.

To fsamuel@: I remove the additional source_set I created as suggested and just
merge the surface_sequence into existing source_set with surface_id.

[Tom Sepez, 2016-06-06 22:34:39]

The mojom itself is fine, but I've one question.

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/surface_sequence...
File cc/ipc/surface_sequence.mojom (right):

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/surface_sequence...
cc/ipc/surface_sequence.mojom:9: // may be depended on once.
What are the consequences if an untrusted client doesn't obey these rules? 
Thanks.

[xlai (Olivia), 2016-06-07 15:17:04]

I replied in-line.

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/surface_sequence...
File cc/ipc/surface_sequence.mojom (right):

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/surface_sequence...
cc/ipc/surface_sequence.mojom:9: // may be depended on once.
On 2016/06/06 22:34:39, Tom Sepez wrote:
> What are the consequences if an untrusted client doesn't obey these rules? 
> Thanks.

Do you mean the consequences when someone mishandles cc::SurfaceSequence (which
is the type getting automatically converted from this mojom struct)? I don't
think that's an issue. 

First, the sole purpose of cc::SurfaceSequence is to ensure correct destruction
order in Surfaces on browser side and they are handled only in these two
functions (Surface::SatisfyDestructionDependencies and
Surface::SatisfyDestructionDependencies). Anyone who intends to modify existing
codes on Surface on browser would need to undergo scrutiny from owners.
Second, in fact, we already have two traditional IPCs
(FrameHostMsg_RequireSequence, FrameHostMsg_SatisfySequence) that are passing
SurfaceSequence across processes to invoke the above-mentioned functions. So far
nothing bad happens with these existing IPCs.

[Tom Sepez, 2016-06-07 15:27:26]

Ok LGTM.

[xlai (Olivia), 2016-06-07 18:09:50]

Description was changed from

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(third_party/WebKit/public/platform/modules/offscreencanvas/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

to

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

[xlai (Olivia), 2016-06-07 19:04:44]

xlai@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2016-06-07 19:16:15]

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/cc_ipc.gyp
File cc/ipc/cc_ipc.gyp (right):

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/cc_ipc.gyp#newco...
cc/ipc/cc_ipc.gyp:45: #        'for_blink': 'true',
Should this line be un-commented (do you need this to work with blink types)? Or
can it be removed?

[xlai (Olivia), 2016-06-07 19:45:25]

Patchset #5 (id:80001) has been deleted

[xlai (Olivia), 2016-06-07 20:45:22]

I did some modifications to cc/ipc/cc_ipc.gyp.

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/cc_ipc.gyp
File cc/ipc/cc_ipc.gyp (right):

https://codereview.chromium.org/2043833002/diff/60001/cc/ipc/cc_ipc.gyp#newco...
cc/ipc/cc_ipc.gyp:45: #        'for_blink': 'true',
On 2016/06/07 19:16:15, ajuma wrote:
> Should this line be un-commented (do you need this to work with blink types)?
Or
> can it be removed?

Just now I did an experiment locally by trying to use cc surface_id in
WebKit/public/ and find that I need both versions of targets: one with
"for_blink=true" and one without "for_blink=true". Mojom generates two sets of
headers, one for blink and one for non-blink. Somehow GN manages to resolve this
automatically, but many GYP build files keep both versions of targets to cover
the difference.

[ajuma, 2016-06-07 20:51:39]

Thanks, lgtm

[xlai (Olivia), 2016-06-07 20:53:37]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2016-06-07 20:53:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2043833002/#ps100001
(title: "Edit")

[commit-bot: I haz the power, 2016-06-07 21:03:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043833002/100001

[commit-bot: I haz the power, 2016-06-07 21:52:20]

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

[commit-bot: I haz the power, 2016-06-07 21:52:21]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xlai (Olivia), 2016-06-08 02:20:41]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-06-08 02:20:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043833002/100001

[commit-bot: I haz the power, 2016-06-08 03:22:29]

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

[commit-bot: I haz the power, 2016-06-08 03:22:30]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xlai (Olivia), 2016-06-08 11:27:39]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-06-08 11:27:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043833002/100001

[commit-bot: I haz the power, 2016-06-08 12:34:53]

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

[commit-bot: I haz the power, 2016-06-08 12:34:54]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xlai (Olivia), 2016-06-08 14:45:06]

The CQ bit was checked by xlai@chromium.org

[commit-bot: I haz the power, 2016-06-08 14:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043833002/100001

[commit-bot: I haz the power, 2016-06-08 16:25:43]

Description was changed from

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

to

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

[commit-bot: I haz the power, 2016-06-08 16:25:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-08 16:27:19]

Description was changed from

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

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

to

==========
Add SurfaceSequence mojom file to cc/ipc and edit corresponding GYP file

This patch is to facilitate landing a new mojo service in another patch
(https://codereview.chromium.org/2036663003/) which requires
to import both surface_id and surface_sequence in mojom files. It adds
surface_sequence.mojom and include both it and surface_id.mojom into GYP
compilation (Currently it's only included in GN build).

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[commit-bot: I haz the power, 2016-06-08 16:27:21]

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