Comment on attachment 8517729 [details] [diff] [review] 0005-Implement-RenderbufferStorageMultisample.patch Review of attachment 8517729 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLContext.h @@ +212,5 @@ > + void ErrorInvalidFramebufferOperation(const char* fmt = 0, ...); > + void ErrorInvalidEnumInfo(const char* info, GLenum enumValue); > + void ErrorInvalidEnumInfo(const char* info, const char* funcName, > + GLenum enumValue); > + void ErrorOutOfMemory(const char* fmt = 0, ...); While we're here, can these 0s be nullptrs?

Let's just reuse this bug.

Comment on attachment 8709647 [details] [diff] [review] 0004-Handle-max-samples-per-format-and-normalize-meaning-.patch Review of attachment 8709647 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/WebGLFramebuffer.cpp @@ +822,5 @@ > + return true; > + } > + > + return (curWidth == width && > + curHeight == height); This would be nicer if it used a type like IntSize. @@ +874,3 @@ > } > > + return matches; It seems like the following would be simpler: uint32_t samples = mColorAttachment0.Samples(); bool matches = true; matches &= (mDepthAttachement.Samples() == samples); matches &= (mStencilAttachement.Samples() == samples); matches &= (mDepthStencilAttachement.Samples() == samples); for (const auto& cur : mMoreColorAttachments) { matches &= (cur.Samples() == samples; } This avoids the awkward needsInit business. (I'd prefer just initializing to the first one in AllImageRectsMatch too)

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8) > @@ +874,3 @@ > > } > > > > + return matches; > > It seems like the following would be simpler: I see that you modify this function in a later patch which makes my simplification no longer practical. My comments about needsInit still apply though.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8) > Comment on attachment 8709647 [details] [diff] [review] > 0004-Handle-max-samples-per-format-and-normalize-meaning-.patch > > Review of attachment 8709647 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/canvas/WebGLFramebuffer.cpp > @@ +822,5 @@ > > + return true; > > + } > > + > > + return (curWidth == width && > > + curHeight == height); > > This would be nicer if it used a type like IntSize. It would be real nice to go back through and do this for these classes. > > @@ +874,3 @@ > > } > > > > + return matches; > > It seems like the following would be simpler: > > uint32_t samples = mColorAttachment0.Samples(); > > bool matches = true; > matches &= (mDepthAttachement.Samples() == samples); > matches &= (mStencilAttachement.Samples() == samples); > matches &= (mDepthStencilAttachement.Samples() == samples); > > for (const auto& cur : mMoreColorAttachments) { > matches &= (cur.Samples() == samples; > } > > This avoids the awkward needsInit business. (I'd prefer just initializing to > the first one in AllImageRectsMatch too) We can't do that. * Attachments may not have an image. (including mColorAttachment0) * Samples() may return anything {0,1,...}, so none of those is a safe sentinel.