Adding status to swap complete

Adding status to swap buffers completion

This will give us more options than completing all swaps successfully, or losing context

BUG=476966
TBR=torne - approved interface change

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

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

[achaulk, 2015-04-17 19:13:58]

achaulk@chromium.org changed reviewers:
+ alexst@chromium.org, spang@chromium.org

[achaulk, 2015-04-17 19:13:58]

Alternately, https://codereview.chromium.org/1091323002/

I think this one is better, as we can just fail the swap itself, it's more
direct

[alexst (slow to review), 2015-04-28 16:00:48]

Agreed, I like this one as well. Allows us to lose the context on swap failure
as well.

[alexst (slow to review), 2015-04-28 18:35:02]

Can we split this up into addition of the status to callback and nothing done
with it and [2/2] actual buffer re-creation?

https://codereview.chromium.org/1084173004/diff/1/ui/ozone/platform/drm/gpu/g...
File ui/ozone/platform/drm/gpu/gbm_surface.h (right):

https://codereview.chromium.org/1084173004/diff/1/ui/ozone/platform/drm/gpu/g...
ui/ozone/platform/drm/gpu/gbm_surface.h:43: int result);
Can we make this an explicit enum type? You can macro enums for IPC.

[achaulk, 2015-04-28 18:51:19]

Yeah I'll split it up

https://codereview.chromium.org/1084173004/diff/1/ui/ozone/platform/drm/gpu/g...
File ui/ozone/platform/drm/gpu/gbm_surface.h (right):

https://codereview.chromium.org/1084173004/diff/1/ui/ozone/platform/drm/gpu/g...
ui/ozone/platform/drm/gpu/gbm_surface.h:43: int result);
On 2015/04/28 18:35:01, alexst wrote:
> Can we make this an explicit enum type? You can macro enums for IPC.

Sure

[achaulk, 2015-04-28 18:55:49]

Where is a good place to put the enum? I just stick it in some random common
header for this patch

[dnicoara, 2015-04-28 19:02:17]

On 2015/04/28 18:55:49, achaulk wrote:
> Where is a good place to put the enum? I just stick it in some random common
> header for this patch

You'll likely need to put it outside the platform (ui/ozone/common sounds like a
good place) since it needs to be included in
ui/ozone/common/gpu/ozone_gpu_messages.h.

[alexst (slow to review), 2015-04-28 19:13:18]

On 2015/04/28 19:02:17, dnicoara wrote:
> On 2015/04/28 18:55:49, achaulk wrote:
> > Where is a good place to put the enum? I just stick it in some random common
> > header for this patch
> 
> You'll likely need to put it outside the platform (ui/ozone/common sounds like
a
> good place) since it needs to be included in
> ui/ozone/common/gpu/ozone_gpu_messages.h.

ui/gfx is right, I think. this is not going through the ozone channel, but the
command buffer.

We stuck a similar one of these into
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/overlay_tra...,
so maybe just make a new file for it.

[achaulk, 2015-04-29 17:16:14]

Updated

[alexst (slow to review), 2015-04-29 17:53:37]

Please give a bit more context in the description about how this will be used.
For example this also allows us to lose the context on swap callback as well in
the async case instead of waiting for the next swap buffers.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_window.cc (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_window.cc:126: const
base::Callback<void(gfx::SwapResult)>& callback) {
We should typdef this as well somewhere in the platform.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.cc (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:21: if (result != gfx::SwapAck)
If one of these fails, we should fail. If we get fail followed by recreate
surface, we will lose the failure signal.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: const
base::Callback<void(gfx::SwapResult)>& callback);
Let's typedef too.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:27: base::AtomicRefCount count_;
These all come on the same thread, so no need to use atomic.

[achaulk, 2015-05-06 18:29:48]

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_window.cc (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_window.cc:126: const
base::Callback<void(gfx::SwapResult)>& callback) {
On 2015/04/29 17:53:37, alexst wrote:
> We should typdef this as well somewhere in the platform.

Done.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.cc (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:21: if (result != gfx::SwapAck)
On 2015/04/29 17:53:37, alexst wrote:
> If one of these fails, we should fail. If we get fail followed by recreate
> surface, we will lose the failure signal.

Done.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: const
base::Callback<void(gfx::SwapResult)>& callback);
On 2015/04/29 17:53:37, alexst wrote:
> Let's typedef too.

Done.

https://codereview.chromium.org/1084173004/diff/20001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:27: base::AtomicRefCount count_;
On 2015/04/29 17:53:37, alexst wrote:
> These all come on the same thread, so no need to use atomic.

I just duplicated the implementation which used atomic

[alexst (slow to review), 2015-05-08 15:00:46]

lgtm with nits.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:11: const
base::Callback<void(gfx::SwapResult)>& callback)
SwapCompletionCallback

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:12: : callback_(callback),
refcount_(crtc_count), result_(gfx::SwapAck) {
crtc_count_ for consistency.

[dnicoara, 2015-05-08 15:15:41]

Drive-by styling comments.

https://codereview.chromium.org/1084173004/diff/40001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gfx/swap_result.h#ne...
ui/gfx/swap_result.h:11: SwapAck,
style guide says you should be using MACRO_STYLE for enum values.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_window.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_window.h:85: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
Style guide says typedefs and constants should be declared first in the class.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
nit: Would it make sense to typedef this outside a class scope and just re-use
it everywhere in the drm platform?

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/public/surface...
File ui/ozone/public/surface_ozone_egl.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/public/surface...
ui/ozone/public/surface_ozone_egl.h:43: typedef
base::Callback<void(gfx::SwapResult)> SwapCompletionCallback;
typedefs should be declared first in the class.

[achaulk, 2015-05-13 15:58:49]

achaulk@chromium.org changed reviewers:
+ piman@chromium.org

[achaulk, 2015-05-13 15:58:50]

+piman for content & ui/gl

[piman, 2015-05-13 16:53:17]

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:199: callback.Run(SwapAck);
SwapAck seems wrong if !success. Should SwapBuffers return a SwapAck instead?

[achaulk, 2015-05-14 18:15:46]

https://codereview.chromium.org/1084173004/diff/40001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gfx/swap_result.h#ne...
ui/gfx/swap_result.h:11: SwapAck,
On 2015/05/08 15:15:41, dnicoara wrote:
> style guide says you should be using MACRO_STYLE for enum values.

Done.

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:199: callback.Run(SwapAck);
On 2015/05/13 16:53:16, piman (Very slow to review) wrote:
> SwapAck seems wrong if !success. Should SwapBuffers return a SwapAck instead?

Ah, yeah I just changed it to switch off of the bool

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/drm_window.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/drm_window.h:85: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/08 15:15:41, dnicoara wrote:
> Style guide says typedefs and constants should be declared first in the class.

Done.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:11: const
base::Callback<void(gfx::SwapResult)>& callback)
On 2015/05/08 15:00:46, alexst wrote:
> SwapCompletionCallback

Done.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.cc:12: : callback_(callback),
refcount_(crtc_count), result_(gfx::SwapAck) {
On 2015/05/08 15:00:46, alexst wrote:
> crtc_count_ for consistency.

Done.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/08 15:15:41, dnicoara wrote:
> nit: Would it make sense to typedef this outside a class scope and just re-use
> it everywhere in the drm platform?

Sure, though it's either adding a typedef here or an include at the top

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/public/surface...
File ui/ozone/public/surface_ozone_egl.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/public/surface...
ui/ozone/public/surface_ozone_egl.h:43: typedef
base::Callback<void(gfx::SwapResult)> SwapCompletionCallback;
On 2015/05/08 15:15:41, dnicoara wrote:
> typedefs should be declared first in the class.

Done.

[dnicoara, 2015-05-14 18:44:54]

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/14 18:15:46, achaulk wrote:
> On 2015/05/08 15:15:41, dnicoara wrote:
> > nit: Would it make sense to typedef this outside a class scope and just
re-use
> > it everywhere in the drm platform?
> 
> Sure, though it's either adding a typedef here or an include at the top

I was hoping to have the typedef done in one place and just reuse it everywhere
else. You've typedef-ed it in 3-4 places and they are all the same. I just
thought it would make it easier to follow if one doesn't need to go back and
forth making sure it is the same typedef everywhere.

[achaulk, 2015-05-14 18:53:06]

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/14 18:44:54, dnicoara wrote:
> On 2015/05/14 18:15:46, achaulk wrote:
> > On 2015/05/08 15:15:41, dnicoara wrote:
> > > nit: Would it make sense to typedef this outside a class scope and just
> re-use
> > > it everywhere in the drm platform?
> > 
> > Sure, though it's either adding a typedef here or an include at the top
> 
> I was hoping to have the typedef done in one place and just reuse it
everywhere
> else. You've typedef-ed it in 3-4 places and they are all the same. I just
> thought it would make it easier to follow if one doesn't need to go back and
> forth making sure it is the same typedef everywhere.

One for the entire build or for ozone?

[dnicoara, 2015-05-14 18:58:58]

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

[dnicoara, 2015-05-14 18:59:00]

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/14 18:53:05, achaulk wrote:
> On 2015/05/14 18:44:54, dnicoara wrote:
> > On 2015/05/14 18:15:46, achaulk wrote:
> > > On 2015/05/08 15:15:41, dnicoara wrote:
> > > > nit: Would it make sense to typedef this outside a class scope and just
> > re-use
> > > > it everywhere in the drm platform?
> > > 
> > > Sure, though it's either adding a typedef here or an include at the top
> > 
> > I was hoping to have the typedef done in one place and just reuse it
> everywhere
> > else. You've typedef-ed it in 3-4 places and they are all the same. I just
> > thought it would make it easier to follow if one doesn't need to go back and
> > forth making sure it is the same typedef everywhere.
> 
> One for the entire build or for ozone?

Whichever is more reasonable. I think this is only used in Ozone at the moment,
so you could just typedef the callback outside SurfaceOzoneEGL. Then you
wouldn't need all the convenience typedefs sprinkled everywhere.

[piman, 2015-05-14 19:08:19]

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:199: callback.Run(SwapAck);
On 2015/05/14 18:15:46, achaulk wrote:
> On 2015/05/13 16:53:16, piman (Very slow to review) wrote:
> > SwapAck seems wrong if !success. Should SwapBuffers return a SwapAck
instead?
> 
> Ah, yeah I just changed it to switch off of the bool

Can you change the return value of SwapBuffers instead? Is there a fundamental
reason why the sync and async versions should differ?

[achaulk, 2015-05-14 20:38:27]

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc
File ui/gl/gl_surface.cc (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/gl/gl_surface.cc#new...
ui/gl/gl_surface.cc:199: callback.Run(SwapAck);
On 2015/05/14 19:08:19, piman (Very slow to review) wrote:
> On 2015/05/14 18:15:46, achaulk wrote:
> > On 2015/05/13 16:53:16, piman (Very slow to review) wrote:
> > > SwapAck seems wrong if !success. Should SwapBuffers return a SwapAck
> instead?
> > 
> > Ah, yeah I just changed it to switch off of the bool
> 
> Can you change the return value of SwapBuffers instead? Is there a fundamental
> reason why the sync and async versions should differ?

Done.

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
File ui/ozone/platform/drm/gpu/page_flip_request.h (right):

https://codereview.chromium.org/1084173004/diff/40001/ui/ozone/platform/drm/g...
ui/ozone/platform/drm/gpu/page_flip_request.h:18: typedef
SurfaceOzoneEGL::SwapCompletionCallback SwapCompletionCallback;
On 2015/05/14 18:58:59, dnicoara wrote:
> On 2015/05/14 18:53:05, achaulk wrote:
> > On 2015/05/14 18:44:54, dnicoara wrote:
> > > On 2015/05/14 18:15:46, achaulk wrote:
> > > > On 2015/05/08 15:15:41, dnicoara wrote:
> > > > > nit: Would it make sense to typedef this outside a class scope and
just
> > > re-use
> > > > > it everywhere in the drm platform?
> > > > 
> > > > Sure, though it's either adding a typedef here or an include at the top
> > > 
> > > I was hoping to have the typedef done in one place and just reuse it
> > everywhere
> > > else. You've typedef-ed it in 3-4 places and they are all the same. I just
> > > thought it would make it easier to follow if one doesn't need to go back
and
> > > forth making sure it is the same typedef everywhere.
> > 
> > One for the entire build or for ozone?
> 
> Whichever is more reasonable. I think this is only used in Ozone at the
moment,
> so you could just typedef the callback outside SurfaceOzoneEGL. Then you
> wouldn't need all the convenience typedefs sprinkled everywhere.

Done.

[piman, 2015-05-14 20:58:54]

at the very least GLES2DecoderImpl::DoSwapBuffers/HandlePostSubBufferCHROMIUM
should be updated, they check the result of
GLSurface::SwapBuffers/PostSubBuffer.
The test would become inverted (true becomes SWAP_ACK which is 0).

Maybe there are other call sites.

You could find those by using an enum class, which would remove implicit
conversions to int.

https://codereview.chromium.org/1084173004/diff/80001/ui/gl/gl_surface_win.cc
File ui/gl/gl_surface_win.cc (right):

https://codereview.chromium.org/1084173004/diff/80001/ui/gl/gl_surface_win.cc...
ui/gl/gl_surface_win.cc:269: return return gfx::SWAP_ACK;
nit: return return = typo

[achaulk, 2015-05-14 21:26:54]

On 2015/05/14 20:58:54, piman (Very slow to review) wrote:
> at the very least GLES2DecoderImpl::DoSwapBuffers/HandlePostSubBufferCHROMIUM
> should be updated, they check the result of
> GLSurface::SwapBuffers/PostSubBuffer.
> The test would become inverted (true becomes SWAP_ACK which is 0).
> 
> Maybe there are other call sites.
> 
> You could find those by using an enum class, which would remove implicit
> conversions to int.
> 
> https://codereview.chromium.org/1084173004/diff/80001/ui/gl/gl_surface_win.cc
> File ui/gl/gl_surface_win.cc (right):
> 
>
https://codereview.chromium.org/1084173004/diff/80001/ui/gl/gl_surface_win.cc...
> ui/gl/gl_surface_win.cc:269: return return gfx::SWAP_ACK;
> nit: return return = typo

Ah, yeah I had gone through the list of callsites in codesearch but missed the
most important one somehow. Fixed now

[piman, 2015-05-14 23:09:57]

LGTM, but 1 important thing left - sorry I missed it previously

https://codereview.chromium.org/1084173004/diff/120001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/120001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:14: SWAP_RESULT_LAST,
Oh, This needs to be SWAP_RESULT_LAST = SWAP_NAK_RECREATE_BUFFERS

[achaulk, 2015-05-15 18:13:39]

https://codereview.chromium.org/1084173004/diff/120001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/120001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:14: SWAP_RESULT_LAST,
On 2015/05/14 23:09:57, piman (Very slow to review) wrote:
> Oh, This needs to be SWAP_RESULT_LAST = SWAP_NAK_RECREATE_BUFFERS

Ah thanks, I wasn't sure if these were supposed to be last element or count of
elements. I'll fix that

[achaulk, 2015-05-15 20:51:28]

achaulk@chromium.org changed reviewers:
+ danakj@chromium.org, wfh@chromium.org

[achaulk, 2015-05-15 20:51:29]

+wfh for security review of ipc change
+dana for new ui/gfx file

[danakj, 2015-05-15 21:54:19]

I think of gfx as being a wrapper layer around skia, so maybe this is a bit
weird to me, but I'm not sure where all things like this should be living. It's
a bit like gfx::Display/Screen which also look like they're in the wrong place
to me. So RS-LGTM.

[achaulk, 2015-05-25 15:20:01]

achaulk@chromium.org changed reviewers:
+ nasko@chromium.org

[achaulk, 2015-05-25 15:20:02]

+nasko for security review of ipc

[nasko, 2015-05-26 18:20:16]

Couple of comments on IPC review.

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
Update the year and drop the "(c)".

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:10: enum class SwapResult {
Is there another gfx header file for this to be included in? Since it is about
swapping buffers, wouldn't ui/gfx/gpu_memory_buffer.h work?

[Will Harris, 2015-05-26 18:21:58]

wfh@chromium.org changed reviewers:
- wfh@chromium.org

[achaulk, 2015-05-26 21:09:36]

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:1: // Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2015/05/26 18:20:16, nasko wrote:
> Update the year and drop the "(c)".

Done.

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:10: enum class SwapResult {
On 2015/05/26 18:20:16, nasko wrote:
> Is there another gfx header file for this to be included in? Since it is about
> swapping buffers, wouldn't ui/gfx/gpu_memory_buffer.h work?

I'm not sure there's an appropriate one really. Gpu memory buffer is a loose fit
at best, as it doesn't have any relation to the SwapBuffers path. But I'll move
it if you want, it's no big deal

[nasko, 2015-05-26 21:18:47]

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:10: enum class SwapResult {
On 2015/05/26 21:09:36, achaulk wrote:
> On 2015/05/26 18:20:16, nasko wrote:
> > Is there another gfx header file for this to be included in? Since it is
about
> > swapping buffers, wouldn't ui/gfx/gpu_memory_buffer.h work?
> 
> I'm not sure there's an appropriate one really. Gpu memory buffer is a loose
fit
> at best, as it doesn't have any relation to the SwapBuffers path. But I'll
move
> it if you want, it's no big deal

If it is unrelated, that's fine. I just want to avoid adding a new file for the
sake of one enum. Usually, we have an *_enums.h version of the file that
contains the enums used in *_messages.h file, so I wanted to see if there is
such a thing for gfx.

You still need to add this file to the appropriate .gypi file.

[achaulk, 2015-05-27 17:57:09]

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h
File ui/gfx/swap_result.h (right):

https://codereview.chromium.org/1084173004/diff/140001/ui/gfx/swap_result.h#n...
ui/gfx/swap_result.h:10: enum class SwapResult {
On 2015/05/26 21:18:47, nasko wrote:
> On 2015/05/26 21:09:36, achaulk wrote:
> > On 2015/05/26 18:20:16, nasko wrote:
> > > Is there another gfx header file for this to be included in? Since it is
> about
> > > swapping buffers, wouldn't ui/gfx/gpu_memory_buffer.h work?
> > 
> > I'm not sure there's an appropriate one really. Gpu memory buffer is a loose
> fit
> > at best, as it doesn't have any relation to the SwapBuffers path. But I'll
> move
> > it if you want, it's no big deal
> 
> If it is unrelated, that's fine. I just want to avoid adding a new file for
the
> sake of one enum. Usually, we have an *_enums.h version of the file that
> contains the enums used in *_messages.h file, so I wanted to see if there is
> such a thing for gfx.
> 
> You still need to add this file to the appropriate .gypi file.

Added

[nasko, 2015-05-27 18:05:37]

IPC LGTM

[spang, 2015-05-27 19:05:46]

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

[spang, 2015-05-27 19:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org,
piman@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps160001
(title: "add to gyp")

[commit-bot: I haz the power, 2015-05-27 19:06:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1084173004/160001

[achaulk, 2015-05-27 19:07:25]

Definitely needs a rebase, doing that now

[spang, 2015-05-27 19:08:00]

The CQ bit was unchecked by spang@chromium.org

[achaulk, 2015-05-27 19:28:32]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-27 19:28:33]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-27 19:29:45]

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

[achaulk, 2015-05-27 19:32:25]

The CQ bit was unchecked by achaulk@chromium.org

[achaulk, 2015-05-27 19:33:03]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-27 19:33:04]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps200001
(title: "re-add new header")

[commit-bot: I haz the power, 2015-05-27 19:35:48]

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

[commit-bot: I haz the power, 2015-05-27 19:48:14]

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

[commit-bot: I haz the power, 2015-05-27 19:48:16]

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

[achaulk, 2015-05-27 20:20:07]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-27 20:20:07]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps220001
(title: "fix test file")

[commit-bot: I haz the power, 2015-05-27 20:20:31]

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

[commit-bot: I haz the power, 2015-05-27 20:43:09]

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

[commit-bot: I haz the power, 2015-05-27 20:43:11]

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

[achaulk, 2015-05-27 20:59:22]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-27 20:59:22]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps240001
(title: "fix win/mac/android builds")

[commit-bot: I haz the power, 2015-05-27 20:59:39]

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

[commit-bot: I haz the power, 2015-05-27 21:36:13]

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

[commit-bot: I haz the power, 2015-05-27 21:36:15]

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

[achaulk, 2015-05-28 14:28:00]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-28 14:28:01]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps260001
(title: "other ozone platforms")

[commit-bot: I haz the power, 2015-05-28 14:28:14]

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

[commit-bot: I haz the power, 2015-05-28 14:54:25]

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

[commit-bot: I haz the power, 2015-05-28 14:54:27]

Try jobs failed on following builders:
  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_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[achaulk, 2015-05-28 17:04:24]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-28 17:04:25]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps280001
(title: "fix mac")

[commit-bot: I haz the power, 2015-05-28 17:05:19]

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

[commit-bot: I haz the power, 2015-05-28 17:39:26]

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

[commit-bot: I haz the power, 2015-05-28 17:39:28]

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

[achaulk, 2015-05-28 18:17:58]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-28 18:17:59]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps300001
(title: "android fixes")

[commit-bot: I haz the power, 2015-05-28 18:18:30]

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

[commit-bot: I haz the power, 2015-05-28 18:27:39]

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

[commit-bot: I haz the power, 2015-05-28 18:27:42]

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

[achaulk, 2015-05-28 19:27:00]

achaulk@chromium.org changed reviewers:
+ torne@chromium.org

[achaulk, 2015-05-28 19:27:02]

+torne for android_webview owners

[achaulk, 2015-05-28 19:55:42]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-28 19:55:43]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps320001
(title: "hopefully last mac fix")

[commit-bot: I haz the power, 2015-05-28 19:56:05]

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

[commit-bot: I haz the power, 2015-05-28 20:31:05]

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

[commit-bot: I haz the power, 2015-05-28 20:31:07]

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

[achaulk, 2015-05-28 21:48:13]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-28 21:48:14]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps340001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-28 21:49:11]

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

[commit-bot: I haz the power, 2015-05-29 01:36:39]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-05-29 01:37:23]

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

[dmurph, 2015-05-29 02:20:24]

On 2015/05/29 at 01:37:23, commit-bot wrote:
> Patchset 18 (id:??) landed as
https://crrev.com/c794eda78e9ba3c46b550b433e9fe5a248d40104
> Cr-Commit-Position: refs/heads/master@{#331906}

I'm suspecting this patch is causing all of the Linux MSAN errors:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/bu...

[dmurph, 2015-05-29 03:16:32]

On 2015/05/29 at 02:20:24, dmurph wrote:
> On 2015/05/29 at 01:37:23, commit-bot wrote:
> > Patchset 18 (id:??) landed as
https://crrev.com/c794eda78e9ba3c46b550b433e9fe5a248d40104
> > Cr-Commit-Position: refs/heads/master@{#331906}
> 
> I'm suspecting this patch is causing all of the Linux MSAN errors:
>
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20MSAN/bu...

Nvm, found culprit

[Nico, 2015-05-29 05:18:23]

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

[Nico, 2015-05-29 05:18:25]

looks like this caused
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...

[dcheng, 2015-05-29 05:19:18]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org
- thakis@chromium.org

[dcheng, 2015-05-29 05:19:20]

FYI, this appears to breaking the ozone bots
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...):

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/ui/ozone/demo/ozone_demo.gl_renderer.o.d -DV8_DEPRECATION_WARNINGS
-D_FILE_OFFSET_BITS=64 -DCHROMIUM_BUILD -DCR_CLANG_REVISION=238013-1
-DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1
-DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_OZONE=1 -DUSE_DEFAULT_RENDER_THEME=1
-DUSE_LIBJPEG_TURBO=1 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1
-DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS
-DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS
-DENABLE_HIDPI=1 -DENABLE_TOPCHROME_MD=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA
-DDCHECK_ALWAYS_ON=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1
-DCLD_VERSION=2 -DENABLE_PRINTING=1 -DENABLE_PRINT_PREVIEW=1
-DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DSAFE_BROWSING_SERVICE -DSK_SUPPORT_GPU=1
-DSK_SUPPORT_LEGACY_OPTIONLESS_GET_PIXELS
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DMESA_EGL_NO_X11_HEADERS
-DUSE_LIBPCI=1 -DUSE_NSS_CERTS=1 -DOS_CHROMEOS=1 -D__STDC_CONSTANT_MACROS
-D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0
-Igen -I../.. -I../../skia/config -I../../third_party/khronos -I../../gpu
-I../../third_party/skia/include/core -I../../third_party/skia/include/effects
-I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu
-I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops
-I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports
-I../../third_party/skia/include/utils -I../../skia/ext
-I../../third_party/mesa/src/include -Werror -pthread -fno-strict-aliasing -Wall
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -Wno-reserved-user-defined-literal -Xclang -load -Xclang
/b/build/slave/linux_chromeos/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-B/b/build/slave/linux_chromeos/build/src/third_party/binutils/Linux_x64/Release/bin
-Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override -m64 -march=x86-64
-O2 -fno-ident -fdata-sections -ffunction-sections -funwind-tables
-funique-section-names -fno-exceptions -fno-rtti -fno-threadsafe-statics
-fvisibility-inlines-hidden -Wsign-compare -std=gnu++11  -c
../../ui/ozone/demo/gl_renderer.cc -o obj/ui/ozone/demo/ozone_demo.gl_renderer.o
../../ui/ozone/demo/gl_renderer.cc:56:35: error: no viable conversion from
'Callback<typename internal::BindState<typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunnableType, typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunType, internal::TypeList<typename
internal::CallbackParamTraits<WeakPtr<GlRenderer> >::StorageType>
>::UnboundRunType>' to 'const Callback<void (gfx::SwapResult)>'
  if (!surface_->SwapBuffersAsync(base::Bind(&GlRenderer::PostRenderFrameTask,
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/callback.h:358:7: note: candidate constructor (the implicit move
constructor) not viable: no known conversion from 'base::Callback<typename
internal::BindState<typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunnableType, typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunType, internal::TypeList<typename
internal::CallbackParamTraits<WeakPtr<GlRenderer> >::StorageType>
>::UnboundRunType>' to 'base::Callback<void (gfx::SwapResult)> &&' for 1st
argument
class Callback;
      ^
../../base/callback.h:366:7: note: candidate constructor (the implicit copy
constructor) not viable: no known conversion from 'base::Callback<typename
internal::BindState<typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunnableType, typename internal::FunctorTraits<void
(GlRenderer::*)()>::RunType, internal::TypeList<typename
internal::CallbackParamTraits<WeakPtr<GlRenderer> >::StorageType>
>::UnboundRunType>' to 'const base::Callback<void (gfx::SwapResult)> &' for 1st
argument
class Callback<R(Args...)> : public internal::CallbackBase {
      ^
../../base/callback.h:375:3: note: candidate template ignored: could not match
'internal::BindState<Runnable, BindRunType, BoundArgsType> *' against
'base::Callback<typename internal::BindState<typename
internal::FunctorTraits<void (GlRenderer::*)()>::RunnableType, typename
internal::FunctorTraits<void (GlRenderer::*)()>::RunType,
internal::TypeList<typename internal::CallbackParamTraits<WeakPtr<GlRenderer>
>::StorageType> >::UnboundRunType>'
  Callback(internal::BindState<Runnable, BindRunType,
  ^
../../ui/gl/gl_surface.h:83:63: note: passing argument to parameter 'callback'
here
  virtual bool SwapBuffersAsync(const SwapCompletionCallback& callback);
                                                              ^
1 error generated.

[vabr (Chromium), 2015-05-29 10:39:10]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/1166463004/ by vabr@chromium.org.

The reason for reverting is: Looks like this broke compilation on
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

Speculatively reverting for now. If confirmed, will file a bug with details,
otherwise re-revert.

Cheers,
your's sheriff.

[vabr (Chromium), 2015-05-29 11:03:20]

On 2015/05/29 10:39:10, vabr (Chromium) wrote:
> A revert of this CL (patchset #18 id:340001) has been created in
> https://codereview.chromium.org/1166463004/ by mailto:vabr@chromium.org.
> 
> The reason for reverting is: Looks like this broke compilation on
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....
> 
> Speculatively reverting for now. If confirmed, will file a bug with details,
> otherwise re-revert.
Revert helped, bug here: http://crbug.com/493649.

> 
> Cheers,
> your's sheriff.
(Sorry for poor grammar, s/your's/your/ .)

[achaulk, 2015-05-29 15:26:22]

Looks like the ozone demo isn't built on the bots. Fixed now

[achaulk, 2015-05-29 15:26:31]

The CQ bit was checked by achaulk@chromium.org

[achaulk, 2015-05-29 15:26:32]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org,
danakj@chromium.org, nasko@chromium.org, alexst@chromium.org
Link to the patchset: https://codereview.chromium.org/1084173004/#ps360001
(title: "fix ozone demo")

[commit-bot: I haz the power, 2015-05-29 15:26:59]

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

[commit-bot: I haz the power, 2015-05-29 16:35:13]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2015-05-29 16:36:11]

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