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

RESOLVED FIXED in mozilla28

Status

()

Core
Graphics
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla28
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Created attachment 829948 [details] [diff] [review]
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.
Attachment #829948 - Flags: review?(jgilbert)
Attachment #829948 - Flags: feedback?(bugmail.mozilla)
Attachment #829948 - Flags: feedback?(bugmail.mozilla) → feedback+
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?
(Assignee)

Comment 2

4 years ago
Sure, good idea, I'll add a comment when I land.
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.
Attachment #829948 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 4

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
As said above, this landed, but folded into a patch on bug 925608.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Assignee: nobody → bjacob
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.