936968 - GLContextProviderEGL::RenewSurface doesn't do anything if it already has a surface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Really renew mSurface

The name RenewSurface implies that it will renew any surface that it already has. But in fact, RenewSurface doesn't do anything if it already has a surface. The right name for what it currently does would be CreateSurfaceIfDontAlreadyHaveOne; but instead, we really need it to renew the surface even if it already has a surface, because that's what we need to do on e.g. screen orientation change. See discussion on bug 925608.

R? jgilbert for the GLContext change here.
F? kats to confirm that the intent here (actually renew surfaces on e.g. orientation change) is correct.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Probably worth a comment to this effect?  As in "please don't optimize this to stop releasing the surface and getting a new one because that is not what we want" or something like that?

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Sure, good idea, I'll add a comment when I land.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 829948 [details] [diff] [review]
Really renew mSurface

Review of attachment 829948 [details] [diff] [review]:
-----------------------------------------------------------------

Fine, though this call looks redundant.

::: gfx/gl/GLContextProviderEGL.cpp
@@ +472,2 @@
>          sEGLLibrary.fMakeCurrent(EGL_DISPLAY(), EGL_NO_SURFACE, EGL_NO_SURFACE,
>                                   EGL_NO_CONTEXT);

This call seems redundant now.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)
> ::: gfx/gl/GLContextProviderEGL.cpp
> @@ +472,2 @@
> >          sEGLLibrary.fMakeCurrent(EGL_DISPLAY(), EGL_NO_SURFACE, EGL_NO_SURFACE,
> >                                   EGL_NO_CONTEXT);
> 
> This call seems redundant now.

It's going away in bug 925608 part 4/8.

Actually, I folded the present patch into bug 925608 part 4/8 because without that, the present patch leaves us in a half-way state that doesn't actually successfully renew surfaces.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

As said above, this landed, but folded into a patch on bug 925608.