Modified old wait sync point functions to also accept new sync tokens.

Modified old wait sync point functions to also accept new sync tokens.

In order to help with refactoring old sync points into new sync points,
glWaitSyncPointCHROMIUM() has been changed to accept both the old and
new sync points.

This CL only refactors all the ways we pass around sync points so in
theory shouldn't change any behavior. Once this lands we can then
incrementally change the sync point insertions to the new sync points.

R=piman@chromium.org
BUG=514815
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

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

[David Yen, 2015-10-26 22:05:20]

dyen@chromium.org changed reviewers:
+ bbudge@chromium.org, dalecurtis@chromium.org, dcheng@chromium.org,
jochen@chromium.org, sky@chromium.org

[David Yen, 2015-10-26 22:05:21]

Sorry for the huge CL, this was as minimal as I could make it.

I'm refactoring all our old sync points to use the new sync points, as an
intermediate step I'm only changing all the wait call sites to support both
types of sync points. Internally this means all transfer methods (mailboxes,
IPCs) and callbacks must also support both.

+jochen, sky for components/pdf_viewer, components_test_runner, blink, mojo,
aura changes

+dcheng for IPC message files (video_capture_messages, ppapi_messages)

+dalecurtis for media changes

+bbudge for ppapi changes

[sky, 2015-10-26 23:55:00]

LGTM

[DaleCurtis, 2015-10-26 23:59:41]

media/ lgtm

[bbudge, 2015-10-27 00:17:01]

pepper / ppapi LGTM

[jochen (gone - plz use gerrit), 2015-10-27 12:17:31]

deferring to sky

[dcheng, 2015-10-27 19:09:28]

https://codereview.chromium.org/1427543002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:200: const gpu::SyncToken
last_waited_sync_token() const {
Just curious: did you intentionally not return a const ref?

https://codereview.chromium.org/1427543002/diff/60001/cc/test/test_web_graphi...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/1427543002/diff/60001/cc/test/test_web_graphi...
cc/test/test_web_graphics_context_3d.h:279: const gpu::SyncToken&
last_waited_sync_token() const {
Is this supposed to be called somewhere?

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:337: gpu::SyncToken*
release_sync_token_storage = new gpu::SyncToken;
It doesn't have to be this CL, but can we make ownership of these objects
clearer in a followup patch? Can we use scoped_ptr<> to manage the storage, for
instance?

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:110: BufferFinishedCB;
Generally, abbreviations are discouraged:
http://google.github.io/styleguide/cppguide.html#General_Naming_Rules

This should probably be BufferFinishedCallback.

https://codereview.chromium.org/1427543002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/sync_token.h (right):

https://codereview.chromium.org/1427543002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/sync_token.h:85: return verified_flush_ !=
other.verified_flush() &&
Shouldn't this be || instead of &&?

Otherwise, there are SyncTokens which are neither == or !=.

https://codereview.chromium.org/1427543002/diff/60001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1427543002/diff/60001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:284: IPC_STRUCT_TRAITS_MEMBER(sync_token)
It appears that the ParamTraits for gpu::SyncToken did not go through an IPC
review. We should probably do that explicitly.
https://codereview.chromium.org/1399173002 is the issue where this was landed,
right?

[Justin Novosad, 2015-10-27 19:24:14]

junov@chromium.org changed reviewers:
+ junov@chromium.org

[Justin Novosad, 2015-10-27 19:24:15]

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp
(right):

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:164:
WGC3Dbyte m_mostRecentlyWaitedSyncToken[24];
24 -> GL_SYNC_TOKEN_SIZE_CHROMIUM

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebExternalTextureMailbox.h:38: signed char
syncToken[24];
24 -> GL_SYNC_TOKEN_SIZE_CHROMIUM

[David Yen, 2015-10-28 22:03:42]

dyen@chromium.org changed reviewers:
+ halliwell@chromium.org

[David Yen, 2015-10-28 22:03:44]

Per piman@'s suggestion, I've folded sync_points into sync_tokens so
mailboxes/callbacks/IPCs no longer take both arguments but only sync tokens.
Transitionally, there is a constructor that converts old sync points to sync
tokens.

Since the sync_point variable is removed in many circumstances, this change also
caught some more locations I didn't catch before.

+halliwell@ for chromecast changes

https://codereview.chromium.org/1427543002/diff/60001/cc/resources/resource_p...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/60001/cc/resources/resource_p...
cc/resources/resource_provider_unittest.cc:200: const gpu::SyncToken
last_waited_sync_token() const {
On 2015/10/27 19:09:28, dcheng wrote:
> Just curious: did you intentionally not return a const ref?

Good catch, was a typo.

https://codereview.chromium.org/1427543002/diff/60001/cc/test/test_web_graphi...
File cc/test/test_web_graphics_context_3d.h (right):

https://codereview.chromium.org/1427543002/diff/60001/cc/test/test_web_graphi...
cc/test/test_web_graphics_context_3d.h:279: const gpu::SyncToken&
last_waited_sync_token() const {
On 2015/10/27 19:09:28, dcheng wrote:
> Is this supposed to be called somewhere?

I originally had them both live side by side until the next CL which would make
the swap, but piman@ recommended that I collapse the 2 so now it is being used
and last_waited_sync_point() has been deleted.

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
File content/renderer/media/video_capture_impl.cc (right):

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
content/renderer/media/video_capture_impl.cc:337: gpu::SyncToken*
release_sync_token_storage = new gpu::SyncToken;
On 2015/10/27 19:09:28, dcheng wrote:
> It doesn't have to be this CL, but can we make ownership of these objects
> clearer in a followup patch? Can we use scoped_ptr<> to manage the storage,
for
> instance?

I will create a follow up CL for this.

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
File content/renderer/media/video_capture_impl.h (right):

https://codereview.chromium.org/1427543002/diff/60001/content/renderer/media/...
content/renderer/media/video_capture_impl.h:110: BufferFinishedCB;
On 2015/10/27 19:09:28, dcheng wrote:
> Generally, abbreviations are discouraged:
> http://google.github.io/styleguide/cppguide.html#General_Naming_Rules
> 
> This should probably be BufferFinishedCallback.

Done.

https://codereview.chromium.org/1427543002/diff/60001/gpu/command_buffer/comm...
File gpu/command_buffer/common/sync_token.h (right):

https://codereview.chromium.org/1427543002/diff/60001/gpu/command_buffer/comm...
gpu/command_buffer/common/sync_token.h:85: return verified_flush_ !=
other.verified_flush() &&
On 2015/10/27 19:09:28, dcheng wrote:
> Shouldn't this be || instead of &&?
> 
> Otherwise, there are SyncTokens which are neither == or !=.

Yes this had broken some unittests. I updated it to just use !(*this == other)
instead.

https://codereview.chromium.org/1427543002/diff/60001/ppapi/proxy/ppapi_messa...
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/1427543002/diff/60001/ppapi/proxy/ppapi_messa...
ppapi/proxy/ppapi_messages.h:284: IPC_STRUCT_TRAITS_MEMBER(sync_token)
On 2015/10/27 19:09:28, dcheng wrote:
> It appears that the ParamTraits for gpu::SyncToken did not go through an IPC
> review. We should probably do that explicitly.
> https://codereview.chromium.org/1399173002 is the issue where this was landed,
> right?

Yes, should the owners be changed?

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp
(right):

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBufferTest.cpp:164:
WGC3Dbyte m_mostRecentlyWaitedSyncToken[24];
On 2015/10/27 19:24:15, Justin Novosad wrote:
> 24 -> GL_SYNC_TOKEN_SIZE_CHROMIUM

Changed so this is no longer relevant.

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebExternalTextureMailbox.h (right):

https://codereview.chromium.org/1427543002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebExternalTextureMailbox.h:38: signed char
syncToken[24];
On 2015/10/27 19:24:15, Justin Novosad wrote:
> 24 -> GL_SYNC_TOKEN_SIZE_CHROMIUM

I thought blink does not include the chromium gles2 headers, isn't that why name
is [64] rather than [GL_MAILBOX_SIZE_CHROMIUM]?

[piman, 2015-10-29 00:08:20]

A few nits/typos, and one slightly bigger thing, but otherwise looks really
close. Thanks for doing this!

https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:2144: MOCK_METHOD1(waitSyncToken, void(const
GLbyte* sync_Token));
nit: sync_token

https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5371: CHECK(false);
nit: NOTREACHED()

https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:133: const
gpu::SyncTokne sync_token = frame->mailbox_holder(0).sync_token;
typo: SyncToken

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:549:
sync_token.SetVerifyFlush();
Actually, thinking about this.... if untrusted, the client can definitely lie
about verified_flush. But the system is designed to handle that.

If the SyncToken gets converted, that means we're about to send it over Mojo
IPC. When that's the case, we should expect it to be verified, before sending,
otherwise the synchronization will not work reliably anyway. So my suggestion is
instead to DCHECK(input.verified_flush()) on l.533, and when we get here simply
always sync_token.SetVerifyFlush() (and remove verified_flush from the mojom).
Checking on the sender side rather than the receiving side is better because
it's closer to the source that failed to verify the token (hopefully in a
related stack trace).

(The same argument applies to chrome IPC, and we could add a DCHECK in
ParamTraits<gpu::SyncToken>::Write).

[halliwell, 2015-10-29 04:24:54]

On 2015/10/29 00:08:20, piman (slow to review) wrote:
> A few nits/typos, and one slightly bigger thing, but otherwise looks really
> close. Thanks for doing this!
> 
>
https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
> File cc/output/gl_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
> cc/output/gl_renderer_unittest.cc:2144: MOCK_METHOD1(waitSyncToken, void(const
> GLbyte* sync_Token));
> nit: sync_token
> 
>
https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
> File gpu/command_buffer/client/gles2_implementation.cc (right):
> 
>
https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
> gpu/command_buffer/client/gles2_implementation.cc:5371: CHECK(false);
> nit: NOTREACHED()
> 
>
https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
> File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):
> 
>
https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
> media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:133: const
> gpu::SyncTokne sync_token = frame->mailbox_holder(0).sync_token;
> typo: SyncToken
> 
>
https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
> File mojo/converters/surfaces/surfaces_type_converters.cc (right):
> 
>
https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
> mojo/converters/surfaces/surfaces_type_converters.cc:549:
> sync_token.SetVerifyFlush();
> Actually, thinking about this.... if untrusted, the client can definitely lie
> about verified_flush. But the system is designed to handle that.
> 
> If the SyncToken gets converted, that means we're about to send it over Mojo
> IPC. When that's the case, we should expect it to be verified, before sending,
> otherwise the synchronization will not work reliably anyway. So my suggestion
is
> instead to DCHECK(input.verified_flush()) on l.533, and when we get here
simply
> always sync_token.SetVerifyFlush() (and remove verified_flush from the mojom).
> Checking on the sender side rather than the receiving side is better because
> it's closer to the source that failed to verify the token (hopefully in a
> related stack trace).
> 
> (The same argument applies to chrome IPC, and we could add a DCHECK in
> ParamTraits<gpu::SyncToken>::Write).

chromecast/ lgtm

[David Yen, 2015-10-29 17:08:08]

https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
File cc/output/gl_renderer_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/cc/output/gl_renderer_u...
cc/output/gl_renderer_unittest.cc:2144: MOCK_METHOD1(waitSyncToken, void(const
GLbyte* sync_Token));
On 2015/10/29 00:08:20, piman (slow to review) wrote:
> nit: sync_token

Done.

https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation.cc:5371: CHECK(false);
On 2015/10/29 00:08:20, piman (slow to review) wrote:
> nit: NOTREACHED()

Done.

https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
File media/video/gpu_memory_buffer_video_frame_pool_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/media/video/gpu_memory_...
media/video/gpu_memory_buffer_video_frame_pool_unittest.cc:133: const
gpu::SyncTokne sync_token = frame->mailbox_holder(0).sync_token;
On 2015/10/29 00:08:20, piman (slow to review) wrote:
> typo: SyncToken

Done.

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:549:
sync_token.SetVerifyFlush();
On 2015/10/29 00:08:20, piman (slow to review) wrote:
> Actually, thinking about this.... if untrusted, the client can definitely lie
> about verified_flush. But the system is designed to handle that.
> 
> If the SyncToken gets converted, that means we're about to send it over Mojo
> IPC. When that's the case, we should expect it to be verified, before sending,
> otherwise the synchronization will not work reliably anyway. So my suggestion
is
> instead to DCHECK(input.verified_flush()) on l.533, and when we get here
simply
> always sync_token.SetVerifyFlush() (and remove verified_flush from the mojom).
> Checking on the sender side rather than the receiving side is better because
> it's closer to the source that failed to verify the token (hopefully in a
> related stack trace).
> 
> (The same argument applies to chrome IPC, and we could add a DCHECK in
> ParamTraits<gpu::SyncToken>::Write).

Is it possible to have an empty sync token too which would be unverified? For
example, a mailbox holder which has no sync token associated with it

[piman, 2015-10-29 23:22:15]

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:549:
sync_token.SetVerifyFlush();
On 2015/10/29 17:08:08, David Yen wrote:
> On 2015/10/29 00:08:20, piman (slow to review) wrote:
> > Actually, thinking about this.... if untrusted, the client can definitely
lie
> > about verified_flush. But the system is designed to handle that.
> > 
> > If the SyncToken gets converted, that means we're about to send it over Mojo
> > IPC. When that's the case, we should expect it to be verified, before
sending,
> > otherwise the synchronization will not work reliably anyway. So my
suggestion
> is
> > instead to DCHECK(input.verified_flush()) on l.533, and when we get here
> simply
> > always sync_token.SetVerifyFlush() (and remove verified_flush from the
mojom).
> > Checking on the sender side rather than the receiving side is better because
> > it's closer to the source that failed to verify the token (hopefully in a
> > related stack trace).
> > 
> > (The same argument applies to chrome IPC, and we could add a DCHECK in
> > ParamTraits<gpu::SyncToken>::Write).
> 
> Is it possible to have an empty sync token too which would be unverified? For
> example, a mailbox holder which has no sync token associated with it

That's fair, but so the check should be DCHECK(input.verified_flush() ||
!input.HasData()).

[David Yen, 2015-10-29 23:27:38]

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
File mojo/converters/surfaces/surfaces_type_converters.cc (right):

https://codereview.chromium.org/1427543002/diff/80001/mojo/converters/surface...
mojo/converters/surfaces/surfaces_type_converters.cc:549:
sync_token.SetVerifyFlush();
On 2015/10/29 23:22:15, piman (slow to review) wrote:
> On 2015/10/29 17:08:08, David Yen wrote:
> > On 2015/10/29 00:08:20, piman (slow to review) wrote:
> > > Actually, thinking about this.... if untrusted, the client can definitely
> lie
> > > about verified_flush. But the system is designed to handle that.
> > > 
> > > If the SyncToken gets converted, that means we're about to send it over
Mojo
> > > IPC. When that's the case, we should expect it to be verified, before
> sending,
> > > otherwise the synchronization will not work reliably anyway. So my
> suggestion
> > is
> > > instead to DCHECK(input.verified_flush()) on l.533, and when we get here
> > simply
> > > always sync_token.SetVerifyFlush() (and remove verified_flush from the
> mojom).
> > > Checking on the sender side rather than the receiving side is better
because
> > > it's closer to the source that failed to verify the token (hopefully in a
> > > related stack trace).
> > > 
> > > (The same argument applies to chrome IPC, and we could add a DCHECK in
> > > ParamTraits<gpu::SyncToken>::Write).
> > 
> > Is it possible to have an empty sync token too which would be unverified?
For
> > example, a mailbox holder which has no sync token associated with it
> 
> That's fair, but so the check should be DCHECK(input.verified_flush() ||
> !input.HasData()).

Done.

[piman, 2015-10-29 23:45:17]

LGTM with one last thing.

https://codereview.chromium.org/1427543002/diff/290001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1427543002/diff/290001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:94: 
Can you add:
if (p->HasData() && !verified_flush)
  return false;


That way it ensures the browser never has to handle a SyncToken that isn't
verified (if otherwise valid). This is a common pattern in IPC traits: sender
DCHECKs a condition, receiver refuses the message if the condition is not true.

(unfortunately, I don't think we have a way to do the same in mojo converters).

[David Yen, 2015-10-30 00:01:30]

https://codereview.chromium.org/1427543002/diff/290001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1427543002/diff/290001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:94: 
On 2015/10/29 23:45:17, piman (slow to review) wrote:
> Can you add:
> if (p->HasData() && !verified_flush)
>   return false;
> 
> 
> That way it ensures the browser never has to handle a SyncToken that isn't
> verified (if otherwise valid). This is a common pattern in IPC traits: sender
> DCHECKs a condition, receiver refuses the message if the condition is not
true.
> 
> (unfortunately, I don't think we have a way to do the same in mojo
converters).

Done.

[piman, 2015-10-30 00:17:47]

LGTM, thanks!

[jochen (gone - plz use gerrit), 2015-10-30 12:00:51]

lgtm

[Justin Novosad, 2015-11-02 16:08:29]

lgtm

[dcheng, 2015-11-03 07:39:29]

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/resource_...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/resource_...
cc/resources/resource_provider_unittest.cc:138: uint32_t sync_point =
static_cast<uint32_t>(sync_token.release_count());
Nit: just make this a uint64_t instead of casting?

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:74: explicit
SyncTokenClientImpl(gpu::gles2::GLES2Interface* gl,
Nit: no explicit, it takes multiple arguments.

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:80: DCHECK(sync_token_.namespace_id() ==
Nit: DCHECK_EQ(gpu::CommandBufferNamespace::OLD_SYNC_POINTS,
sync_token_.namespace_id());

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:82: return
static_cast<uint32>(sync_token_.release_count());
Out of curiosity, why isn't release_count() just a uint32_t to avoid these
casts?

https://codereview.chromium.org/1427543002/diff/390001/cc/test/layer_tree_pix...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/test/layer_tree_pix...
cc/test/layer_tree_pixel_test.cc:292: 
Revert these empty lines?

https://codereview.chromium.org/1427543002/diff/390001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:149:
ParamTraits<gpu::SyncToken>::Log(p.sync_token, l);
Nit: LogParam (here and previous line) for magical type-deduction.

[David Yen, 2015-11-03 18:09:28]

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/resource_...
File cc/resources/resource_provider_unittest.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/resource_...
cc/resources/resource_provider_unittest.cc:138: uint32_t sync_point =
static_cast<uint32_t>(sync_token.release_count());
On 2015/11/03 07:39:28, dcheng wrote:
> Nit: just make this a uint64_t instead of casting?

sync_point_for_mail_box takes a uint32, I'd rather minimize changes here. All of
this code is about to be deleted though once we switch to real sync tokens.

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:74: explicit
SyncTokenClientImpl(gpu::gles2::GLES2Interface* gl,
On 2015/11/03 07:39:29, dcheng wrote:
> Nit: no explicit, it takes multiple arguments.

Done.

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:80: DCHECK(sync_token_.namespace_id() ==
On 2015/11/03 07:39:29, dcheng wrote:
> Nit: DCHECK_EQ(gpu::CommandBufferNamespace::OLD_SYNC_POINTS,
> sync_token_.namespace_id());

Done.

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:82: return
static_cast<uint32>(sync_token_.release_count());
On 2015/11/03 07:39:29, dcheng wrote:
> Out of curiosity, why isn't release_count() just a uint32_t to avoid these
> casts?

The new sync tokens need 64 bit releases to avoid overflow. The old sync points
are 32 bit, this CL is temporarily folding the old sync points into the new sync
tokens before removing the old code so I am just using 32 bits of the release
counter.

https://codereview.chromium.org/1427543002/diff/390001/cc/test/layer_tree_pix...
File cc/test/layer_tree_pixel_test.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/test/layer_tree_pix...
cc/test/layer_tree_pixel_test.cc:292: 
On 2015/11/03 07:39:29, dcheng wrote:
> Revert these empty lines?

Done.

https://codereview.chromium.org/1427543002/diff/390001/gpu/ipc/gpu_command_bu...
File gpu/ipc/gpu_command_buffer_traits.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/gpu/ipc/gpu_command_bu...
gpu/ipc/gpu_command_buffer_traits.cc:149:
ParamTraits<gpu::SyncToken>::Log(p.sync_token, l);
On 2015/11/03 07:39:29, dcheng wrote:
> Nit: LogParam (here and previous line) for magical type-deduction.

Done.

[dcheng, 2015-11-03 18:16:22]

lgtm, under the condition that casting from uint64 to uint32 goes away soon =)

[David Yen, 2015-11-03 18:18:36]

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/1427543002/diff/390001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:82: return
static_cast<uint32>(sync_token_.release_count());
On 2015/11/03 18:09:27, David Yen wrote:
> On 2015/11/03 07:39:29, dcheng wrote:
> > Out of curiosity, why isn't release_count() just a uint32_t to avoid these
> > casts?
> 
> The new sync tokens need 64 bit releases to avoid overflow. The old sync
points
> are 32 bit, this CL is temporarily folding the old sync points into the new
sync
> tokens before removing the old code so I am just using 32 bits of the release
> counter.

In case it is unclear, this function will eventually be turned into something
like "void InsertSyncToken(gpu::SyncToken* sync_token)" instead of using the old
sync points. This can happen incrementally and we wouldn't need to change any
call sites which do the wait.

[David Yen, 2015-11-03 18:39:34]

The CQ bit was checked by dyen@chromium.org

[David Yen, 2015-11-03 18:39:35]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
sky@chromium.org, dalecurtis@chromium.org, halliwell@chromium.org,
jochen@chromium.org, piman@chromium.org, junov@chromium.org
Link to the patchset: https://codereview.chromium.org/1427543002/#ps430001
(title: "format")

[commit-bot: I haz the power, 2015-11-03 18:41:04]

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

[commit-bot: I haz the power, 2015-11-03 20:03:15]

Committed patchset #23 (id:430001)

[commit-bot: I haz the power, 2015-11-03 20:04:15]

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