Add NativeViewGLSurfaceEGL::CommitOverlayPlanes support for Android

Add NativeViewGLSurfaceEGL::CommitOverlayPlanes support for Android

Now NativeViewGLSurfaceEGL keeps a vector of pending overlays to be
displayed on the next swap or CommitOverlayPlanes. This is Android
only for now because it's the motivating case, but other platforms
could be added later.

BUG=560592
Committed: https://crrev.com/e6e68b1e1b3d6d8158f1cd4c194b74688df56346
Cr-Commit-Position: refs/heads/master@{#362341}

[watk, 2015-11-30 22:42:54]

Patchset #2 (id:20001) has been deleted

[watk, 2015-12-01 00:43:37]

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

[watk, 2015-12-01 00:43:37]

Hi piman, PTAL. I considered splitting the android specific parts into a new
class (GLSurfaceEGLAndroid) as we discussed, but I didn't for two reasons: 1) I
thought that this might make sense for other platforms, and 2) being unfamiliar
with this code, it felt like there was a good chance I would break something.

[piman, 2015-12-01 01:06:45]

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:686: if (SupportsCommitOverlayPlanes() &&
!CommitAndClearPendingOverlays())
SupportsCommitOverlayPlanes() seems orthogonal to
CommitAndClearPendingOverlays(), I think we should just always
CommitAndClearPendingOverlays() (keeping in mind that the overlay list is likely
empty). If anything just not do it if the list is empty.

It might make sense to move this before the eglSwapBuffers, in case there's an
interaction between it and the set of enabled overlays.

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:758: if (SupportsCommitOverlayPlanes() &&
!CommitAndClearPendingOverlays())
(same here)

[watk, 2015-12-01 01:55:55]

I appreciate the quick review!

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:686: if (SupportsCommitOverlayPlanes() &&
!CommitAndClearPendingOverlays())
On 2015/12/01 01:06:45, piman (slow to review) wrote:
> SupportsCommitOverlayPlanes() seems orthogonal to
> CommitAndClearPendingOverlays(), I think we should just always
> CommitAndClearPendingOverlays() (keeping in mind that the overlay list is
likely
> empty). If anything just not do it if the list is empty.
> 
> It might make sense to move this before the eglSwapBuffers, in case there's an
> interaction between it and the set of enabled overlays.

That makes a lot of sense. Done.

https://codereview.chromium.org/1486773002/diff/60001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:758: if (SupportsCommitOverlayPlanes() &&
!CommitAndClearPendingOverlays())
On 2015/12/01 01:06:45, piman (slow to review) wrote:
> (same here)

Done.

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

https://codereview.chromium.org/1486773002/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:822: // away.
How do you feel about this? It's not obvious how to make this more consistent
and less subtle.

[piman, 2015-12-01 02:03:02]

lgtm

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

https://codereview.chromium.org/1486773002/diff/80001/ui/gl/gl_surface_egl.cc...
ui/gl/gl_surface_egl.cc:822: // away.
On 2015/12/01 01:55:55, watk wrote:
> How do you feel about this? It's not obvious how to make this more consistent
> and less subtle.

I'd move this comment to the CommitOverlayPlanes implementation. That's the
place where it assumes that the overlays will magically appear, and the place
where possibly platform-specific bits would have to "kick" them for truly atomic
changes.

[watk, 2015-12-01 03:21:21]

On 2015/12/01 02:03:02, piman (slow to review) wrote:
> lgtm
> 
> https://codereview.chromium.org/1486773002/diff/80001/ui/gl/gl_surface_egl.cc
> File ui/gl/gl_surface_egl.cc (right):
> 
>
https://codereview.chromium.org/1486773002/diff/80001/ui/gl/gl_surface_egl.cc...
> ui/gl/gl_surface_egl.cc:822: // away.
> On 2015/12/01 01:55:55, watk wrote:
> > How do you feel about this? It's not obvious how to make this more
consistent
> > and less subtle.
> 
> I'd move this comment to the CommitOverlayPlanes implementation. That's the
> place where it assumes that the overlays will magically appear, and the place
> where possibly platform-specific bits would have to "kick" them for truly
atomic
> changes.

SGTM, done. Checking the CQ

[watk, 2015-12-01 03:21:29]

The CQ bit was checked by watk@chromium.org

[watk, 2015-12-01 03:21:29]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1486773002/#ps100001
(title: "Move comment")

[commit-bot: I haz the power, 2015-12-01 03:22:58]

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

[commit-bot: I haz the power, 2015-12-01 04:04:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-12-01 04:05:42]

Description was changed from

==========
Add NativeViewGLSurfaceEGL::CommitOverlayPlanes support for Android

Now NativeViewGLSurfaceEGL keeps a vector of pending overlays to be
displayed on the next swap or CommitOverlayPlanes. This is Android
only for now because it's the motivating case, but other platforms
could be added later.

BUG=560592
==========

to

==========
Add NativeViewGLSurfaceEGL::CommitOverlayPlanes support for Android

Now NativeViewGLSurfaceEGL keeps a vector of pending overlays to be
displayed on the next swap or CommitOverlayPlanes. This is Android
only for now because it's the motivating case, but other platforms
could be added later.

BUG=560592

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

[commit-bot: I haz the power, 2015-12-01 04:05:43]

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