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: 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?
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+
(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.
As said above, this landed, but folded into a patch on bug 925608.
Status: NEW → RESOLVED
Last Resolved: 5 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.