Going through bug 948002, I saw a number of uses of RectangleObject() that didn't really check if it was null, even if the function returning null has a very specific meaning. Generally, this means that things fail quietly. I want to make them more fragile so we can be more confident we're asking the right questions.

I only need one reviewer, really. I'm sure you guys are both jumping to review this sort of patch, so I'll offer it to both of you. :P There's a bunch of style normalizations in here. Splitting it out can probably be done, but will be a pain for me. Let me know if the inter-mingled style changes are too much. One change here is use of return-by-reference for non-nullable retvals. Another is a refactor of the CheckFramebufferStatus code, and condensing the two parallel implementations into Framebuffer::CheckFramebufferStatus. Another is a clarification (largely via asserts) of when an FB actually has a consistent valid RectObject. An FB basically has to be FB-complete in order to get anything useful for a RectObject. In line with the previous change is paring back the assumption that colorAttach[0] is always present. I don't think it's gone, but it should be closer to being gone.

Comment on attachment 8363400 [details] [diff] [review] sharp-fb-rects Review of attachment 8363400 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/canvas/src/WebGLFramebuffer.cpp @@ +402,5 @@ > bool > +WebGLFramebuffer::HasDefinedAttachments() const > +{ > + bool hasAttachments = false; > + I'm leaving a ton of empty whitespace here. My bad.

This is just stripping end-of-line whitespace.

Maybe I'll give bjacob a break for once. Feel free to reassign review, Dan.

Comment on attachment 8363408 [details] [diff] [review] wsfix-pre Double plus good. But I've already cleaned up the whitespace in a patch in 948002, so we have to decide in which order to land these patches.

Comment on attachment 8363409 [details] [diff] [review] patch Review of attachment 8363409 [details] [diff] [review]: ----------------------------------------------------------------- Only nits. ::: content/canvas/src/WebGLFramebuffer.cpp @@ +74,5 @@ > return false; > + > + if (Renderbuffer()) { > + return Renderbuffer()->HasUninitializedImageData(); > + } else if (Texture()) { No else after return. Occurs throughout this file. @@ +108,5 @@ > + return true; > + else if (Renderbuffer()) > + return true; > + > + return false; No else after return, but I don't prefer this style. bool WebGLFramebuffer::Attachment::HasImage() const { return (Texture() && Texture()->HasImageInfoAt(mTexImageTarget, mTexImageLevel)) || Renderbuffer(); } Is more concise and, IMHO, easier to read. @@ +123,5 @@ > + } else if (Renderbuffer()) { > + return *Renderbuffer(); > + } > + > + MOZ_CRASH("Should not get here."); Is this overly paranoid? (double check of the assert at the top of function) @@ +176,3 @@ > return IsValidAttachedTextureColorFormat(format); > } > + MOZ_ASSERT(false, "Invalid WebGL attachment point?"); What is the difference because MOZ_CRASH(...) and MOZ_ASSERT(false, ...)? @@ +365,5 @@ > > void > +WebGLFramebuffer::DetachTexture(const WebGLTexture* tex) > +{ > + for (size_t i = 0; i < mColorAttachments.Length(); i++) { I'm not a fan of calling container class "size" methods on every iteration. @@ +518,5 @@ > + if (HasIncompleteAttachments()) > + return LOCAL_GL_FRAMEBUFFER_INCOMPLETE_ATTACHMENT; > + > + if (!AllImageRectsMatch()) > + return LOCAL_GL_FRAMEBUFFER_INCOMPLETE_DIMENSIONS; // No consistent size // Inconsistent size ::: content/canvas/src/WebGLFramebuffer.h @@ +126,5 @@ > int(mStencilAttachment.IsDefined()) + > int(mDepthStencilAttachment.IsDefined()) >= 2; > } > > + size_t ColorAttachmentCount() const { This is only called in once in the code. To me appears to be a go candidate to remove.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc417a1f843 https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbdbd74e876

More fixups of nits.

Comment on attachment 8364148 [details] [diff] [review] fixups Review of attachment 8364148 [details] [diff] [review]: ----------------------------------------------------------------- M-x delete-trailing-whitespace

https://hg.mozilla.org/mozilla-central/rev/0cc417a1f843 https://hg.mozilla.org/mozilla-central/rev/9fbdbd74e876

Comment on attachment 8365327 [details] [diff] [review] fixups Review of attachment 8365327 [details] [diff] [review]: ----------------------------------------------------------------- GTG.

Comment on attachment 8365327 [details] [diff] [review] fixups Doesn't apply cleanly. Please rebase.

> What is the difference because MOZ_CRASH(...) and MOZ_ASSERT(false, ...)? MOZ_ASSERT is debug-only.

By using MOZ_ASSERT instead of MOZ_CRASH, bug 974744 doesn't bring down the browser, for example ;)

This is in central, but not marked FIXED. Inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/e8a7d2c8607c Central http://hg.mozilla.org/mozilla-central/rev/e8a7d2c8607c