ui/gl: Initialize the ANGLE Platform on all configurations

ui/gl: Initialize the ANGLE Platform on all EGL configurations

BUG=angleproject:1621
Committed: https://crrev.com/b2223ac7d08f5aa62e1d0c6268507142e86ef098
Cr-Commit-Position: refs/heads/master@{#436334}

[Corentin Wallez, 2016-11-10 19:32:05]

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

[commit-bot: I haz the power, 2016-11-10 19:32:27]

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

[commit-bot: I haz the power, 2016-11-10 19:45:49]

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

[commit-bot: I haz the power, 2016-11-10 19:45:50]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Corentin Wallez, 2016-11-11 13:51:19]

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

[commit-bot: I haz the power, 2016-11-11 13:51:30]

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

[commit-bot: I haz the power, 2016-11-11 14:02:06]

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

[commit-bot: I haz the power, 2016-11-11 14:02:07]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Corentin Wallez, 2016-11-11 14:13:49]

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

[commit-bot: I haz the power, 2016-11-11 14:14:02]

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

[commit-bot: I haz the power, 2016-11-11 14:49:13]

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

[commit-bot: I haz the power, 2016-11-11 14:49:13]

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

[Corentin Wallez, 2016-11-11 20:04:24]

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

[commit-bot: I haz the power, 2016-11-11 20:04:46]

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

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

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

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

Dry run: This issue passed the CQ dry run.

[Corentin Wallez, 2016-11-14 18:31:18]

Description was changed from

==========
ui/gl: Initialize the ANGLE Platform on all configurations

BUG=
==========

to

==========
ui/gl: Initialize the ANGLE Platform on all EGL configurations

BUG=angleproject:1621
==========

[Corentin Wallez, 2016-11-14 18:35:35]

cwallez@chromium.org changed reviewers:
+ kbr@chromium.org, thakis@chromium.org, zmo@chromium.org

[Corentin Wallez, 2016-11-14 18:35:36]

kbr, zmo PTAL, this will enable the ANGLE platform on Linux X11 so we can tell
Chrome which workarounds are being used (plus other goodies like traces
histograms etc.) This depends on an ANGLE-side change:
https://chromium-review.googlesource.com/c/410066/

thakis: a function rename touched gl_ozone_egl.cc PTAL

[Zhenyao Mo, 2016-11-14 19:18:22]

On 2016/11/14 18:35:36, Corentin Wallez wrote:
> kbr, zmo PTAL, this will enable the ANGLE platform on Linux X11 so we can tell
> Chrome which workarounds are being used (plus other goodies like traces
> histograms etc.) This depends on an ANGLE-side change:
> https://chromium-review.googlesource.com/c/410066/
> 
> thakis: a function rename touched gl_ozone_egl.cc PTAL

non owner lgtm

[Ken Russell (switch to Gerrit), 2016-11-14 20:02:24]

A few questions.

https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
File ui/gl/init/gl_initializer_android.cc (right):

https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
ui/gl/init/gl_initializer_android.cc:100: GLSurfaceEGL::ShutdownOneOff();
It seems asymmetric that a GLSurface ShutdownOneOff would be done in
ClearGLBindingsPlatform, when InitializeOneOff is called inside
InitializeGLOneOffPlatform -- especially given that this calls eglTerminate.

Is it worth adding a new entry point to do this work and modifying all the call
sites?

Is this code correct on all platforms which use EGL, or has it only been tested
with ANGLE? (Does ClearGLBindingsEGL do any EGL-related work?)

[Ken Russell (switch to Gerrit), 2016-11-29 22:06:51]

What's the plan for this CL? Going to move forward with it?

[Corentin Wallez, 2016-11-29 22:08:10]

On 2016/11/29 at 22:06:51, kbr wrote:
> What's the plan for this CL? Going to move forward with it?

Thanks for the reminder, I was waiting to know what the workarounds story was
going to be. I will get back to it and address your comments.

[Corentin Wallez, 2016-11-30 16:07:09]

https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
File ui/gl/init/gl_initializer_android.cc (right):

https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
ui/gl/init/gl_initializer_android.cc:100: GLSurfaceEGL::ShutdownOneOff();
On 2016/11/14 at 20:02:24, Ken Russell wrote:
> It seems asymmetric that a GLSurface ShutdownOneOff would be done in
ClearGLBindingsPlatform, when InitializeOneOff is called inside
InitializeGLOneOffPlatform -- especially given that this calls eglTerminate.
> 
> Is it worth adding a new entry point to do this work and modifying all the
call sites?
> 
> Is this code correct on all platforms which use EGL, or has it only been
tested with ANGLE? (Does ClearGLBindingsEGL do any EGL-related work?)

Thanks for pointing this out. Looking at the code, ResetForTesting is used in
OzoneEGL::ClearGLBindings and platform ClearGLBindings is used in tests and the
vr_shell. Also at initialization the gl bindings is initialized, then the
platform one off initialization is done, ClearGLBindings is currently clearing
the gl bindings but the way it is used is to deinit the GL completely.

How about renaming ClearGLBindings to ShutdownGL and keep the approach here?

[Ken Russell (switch to Gerrit), 2016-12-02 01:46:56]

On 2016/11/30 16:07:09, Corentin Wallez wrote:
>
https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
> File ui/gl/init/gl_initializer_android.cc (right):
> 
>
https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
> ui/gl/init/gl_initializer_android.cc:100: GLSurfaceEGL::ShutdownOneOff();
> On 2016/11/14 at 20:02:24, Ken Russell wrote:
> > It seems asymmetric that a GLSurface ShutdownOneOff would be done in
> ClearGLBindingsPlatform, when InitializeOneOff is called inside
> InitializeGLOneOffPlatform -- especially given that this calls eglTerminate.
> > 
> > Is it worth adding a new entry point to do this work and modifying all the
> call sites?
> > 
> > Is this code correct on all platforms which use EGL, or has it only been
> tested with ANGLE? (Does ClearGLBindingsEGL do any EGL-related work?)
> 
> Thanks for pointing this out. Looking at the code, ResetForTesting is used in
> OzoneEGL::ClearGLBindings and platform ClearGLBindings is used in tests and
the
> vr_shell. Also at initialization the gl bindings is initialized, then the
> platform one off initialization is done, ClearGLBindings is currently clearing
> the gl bindings but the way it is used is to deinit the GL completely.
> 
> How about renaming ClearGLBindings to ShutdownGL and keep the approach here?

That sounds OK. ClearGLBindings -> ShutdownGL, ClearGLBindingsPlatform ->
ShutdownGLPlatform, or similar.

[Ken Russell (switch to Gerrit), 2016-12-02 01:47:11]

On 2016/12/02 01:46:56, Ken Russell wrote:
> On 2016/11/30 16:07:09, Corentin Wallez wrote:
> >
>
https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
> > File ui/gl/init/gl_initializer_android.cc (right):
> > 
> >
>
https://codereview.chromium.org/2491993002/diff/60001/ui/gl/init/gl_initializ...
> > ui/gl/init/gl_initializer_android.cc:100: GLSurfaceEGL::ShutdownOneOff();
> > On 2016/11/14 at 20:02:24, Ken Russell wrote:
> > > It seems asymmetric that a GLSurface ShutdownOneOff would be done in
> > ClearGLBindingsPlatform, when InitializeOneOff is called inside
> > InitializeGLOneOffPlatform -- especially given that this calls eglTerminate.
> > > 
> > > Is it worth adding a new entry point to do this work and modifying all the
> > call sites?
> > > 
> > > Is this code correct on all platforms which use EGL, or has it only been
> > tested with ANGLE? (Does ClearGLBindingsEGL do any EGL-related work?)
> > 
> > Thanks for pointing this out. Looking at the code, ResetForTesting is used
in
> > OzoneEGL::ClearGLBindings and platform ClearGLBindings is used in tests and
> the
> > vr_shell. Also at initialization the gl bindings is initialized, then the
> > platform one off initialization is done, ClearGLBindings is currently
clearing
> > the gl bindings but the way it is used is to deinit the GL completely.
> > 
> > How about renaming ClearGLBindings to ShutdownGL and keep the approach here?
> 
> That sounds OK. ClearGLBindings -> ShutdownGL, ClearGLBindingsPlatform ->
> ShutdownGLPlatform, or similar.

LGTM if that change is made.

[Nico, 2016-12-02 01:49:04]

my one file lgtm

[Corentin Wallez, 2016-12-05 16:10:06]

The CQ bit was checked by cwallez@chromium.org

[Corentin Wallez, 2016-12-05 16:10:06]

Rebased on top of https://codereview.chromium.org/2543313002/ which addresses
kbr's comments. Thanks everyone for the review.

[Corentin Wallez, 2016-12-05 16:10:06]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org,
zmo@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2491993002/#ps80001
(title: "Rebase on top of the renaming of ClearGLBindings")

[commit-bot: I haz the power, 2016-12-05 16:10:26]

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

[commit-bot: I haz the power, 2016-12-05 17:40:30]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480954206138200,
"parent_rev": "a0983f25b52177f5e99b6878b728131067480813", "commit_rev":
"384363e2ed66f18c8750146c3c156c6f852f1c19"}

[commit-bot: I haz the power, 2016-12-05 17:41:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-05 17:42:54]

Description was changed from

==========
ui/gl: Initialize the ANGLE Platform on all EGL configurations

BUG=angleproject:1621
==========

to

==========
ui/gl: Initialize the ANGLE Platform on all EGL configurations

BUG=angleproject:1621

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

[commit-bot: I haz the power, 2016-12-05 17:42:55]

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

[Corentin Wallez, 2016-12-05 21:52:00]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2548253003/ by cwallez@chromium.org.

The reason for reverting is: Unlikely suspect for flaky failure of contrast-hw
WebKit test see
https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu....