Closed Bug 1081363 Opened 5 years ago Closed 5 years ago

Don't do readback internal to ShSurf_Basic

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

Attachments

(1 file)

Let's consider ShSurf_Basic as not having any destination form, thus requiring a readback, rather than having it be yet another readback path.

We already have readback paths for cases where we can't handle the type of the ShSurf. We should just have that code handle Basic surfaces.
Comment on attachment 8503454 [details] [diff] [review]
0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch

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

::: gfx/gl/SharedSurface.cpp
@@ +491,5 @@
> +    if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat))
> +        return false;
> +
> +    const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 ||
> +                           dstFormat == gfx::SurfaceFormat::R8G8B8X8;

I wouldn't mind some parens to separate the == || == part from the assignment.
Attachment #8503454 - Flags: review?(dglastonbury) → review+
(In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> Comment on attachment 8503454 [details] [diff] [review]
> 0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch
> 
> Review of attachment 8503454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/SharedSurface.cpp
> @@ +491,5 @@
> > +    if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat))
> > +        return false;
> > +
> > +    const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 ||
> > +                           dstFormat == gfx::SurfaceFormat::R8G8B8X8;
> 
> I wouldn't mind some parens to separate the == || == part from the
> assignment.

We generally don't do this elsewhere, but I wouldn't otherwise be adverse to it.
It's a fairly standard pattern, so we can be confident the order-of-operations should be obvious.
See Also: → 1081495
(In reply to Jeff Gilbert [:jgilbert] from comment #5)
> (In reply to Dan Glastonbury :djg :kamidphish from comment #4)
> > Comment on attachment 8503454 [details] [diff] [review]
> > 0001-Don-t-do-readback-eagerly-in-ShSurf_Basic.patch
> > 
> > Review of attachment 8503454 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/gl/SharedSurface.cpp
> > @@ +491,5 @@
> > > +    if (!dst->LockBits(&dstBytes, &dstSize, &dstStride, &dstFormat))
> > > +        return false;
> > > +
> > > +    const bool isDstRGBA = dstFormat == gfx::SurfaceFormat::R8G8B8A8 ||
> > > +                           dstFormat == gfx::SurfaceFormat::R8G8B8X8;
> > 
> > I wouldn't mind some parens to separate the == || == part from the
> > assignment.
> 
> We generally don't do this elsewhere, but I wouldn't otherwise be adverse to
> it.
> It's a fairly standard pattern, so we can be confident the
> order-of-operations should be obvious.

It just helps break up the a = b == c, which looks a bit odd to me. Also Emacs automatically gets the indentation correction when you put ()'s. I know it doesn't effect the way the code is compiled but helps my dinosaur brain parse what's on the screen. :-)
https://hg.mozilla.org/mozilla-central/rev/a0c17c73ba1d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.