All the subpieces of Part 7 need to be landed together, I'll fold the patches before landing.

Comment on attachment 8345033 [details] [diff] [review] Part 7.3: Convert WebGL callers of SurfaceFromElement use Moz2D Review of attachment 8345033 [details] [diff] [review]: ----------------------------------------------------------------- Only one important nit about setting imageOut=nullptr. r=me with that. ::: content/canvas/src/WebGLContext.h @@ +27,5 @@ > > #include "mozilla/LinkedList.h" > #include "mozilla/CheckedInt.h" > #include "mozilla/Scoped.h" > +#include "mozilla/gfx/2D.h" It's not great to have to include 2D.h in another header, but here indeed you don't have a choice as we have template functions implemented in the header, and templatedness makes them hard to move out of it. We'll need to fix that at some point... @@ +425,3 @@ > WebGLTexelFormat srcFormat; > nsLayoutUtils::SurfaceFromElementResult res = SurfaceFromElement(elt); > + rv = SurfaceFromElementResultToImageSurface(res, data, At first the removal of the getter_AddRefs scared me... until I saw that below you changed the way that SurfaceFromElementResultToImageSurface returns its result, so it's no longer a scary already-addreffed Foo**. Many thanks for this change! @@ +430,4 @@ > return; > > + gfx::IntSize size = data->GetSize(); > + uint32_t byteLength = data->Stride() * size.height; Funny how we don't check for integer overflow here. We need to fix that, even if it's hardly attainable in practice. @@ +470,4 @@ > return; > > + gfx::IntSize size = data->GetSize(); > + uint32_t byteLength = data->Stride() * size.height; same remark as above... (just note to self) @@ +993,5 @@ > > template<class ElementType> > nsLayoutUtils::SurfaceFromElementResult SurfaceFromElement(ElementType* aElement) { > MOZ_ASSERT(aElement); > + uint32_t flags = 0; I trust you about SFE_WANT_IMAGE_SURFACE not being useful anymore, I have no idea about that. ::: content/canvas/src/WebGLContextGL.cpp @@ +2646,1 @@ > *format = WebGLTexelFormat::None; Only one important nit: could you restore the first line in this function, i.e. make this function unconditionally set imageOut=nullptr at the beginning, so that we never exit leaving it with undefined value?

Comment on attachment 8345023 [details] [diff] [review] Part 1: Remove mPrintSurface from HTMLMediaElement since it's never used. Review of attachment 8345023 [details] [diff] [review]: ----------------------------------------------------------------- Printing of videos is busted but it should be fixed :-(

Comment on attachment 8345030 [details] [diff] [review] Part 7.1: Convert SurfaceFromElement to Moz2D. Review of attachment 8345030 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.h @@ +1584,5 @@ > }; > > static SurfaceFromElementResult SurfaceFromElement(mozilla::dom::Element *aElement, > + uint32_t aSurfaceFlags = 0, > + DrawTarget *aTarget = nullptr); aTarget needs to be documented somewhere

Comment on attachment 8345032 [details] [diff] [review] Part 7.2: Convert CanvasRenderingContext2D callers of SurfaceFromElement use Moz2D Review of attachment 8345032 [details] [diff] [review]: ----------------------------------------------------------------- This is full of awesome!

(In reply to Benoit Jacob [:bjacob] from comment #12) > ::: content/canvas/src/WebGLContextGL.cpp > @@ +2646,1 @@ > > *format = WebGLTexelFormat::None; > > Only one important nit: could you restore the first line in this function, > i.e. make this function unconditionally set imageOut=nullptr at the > beginning, so that we never exit leaving it with undefined value? RefPtr's are default initialized to nullptr already!

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a8de576daf https://hg.mozilla.org/integration/mozilla-inbound/rev/c13412051f52 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b0f7f3dfa8e https://hg.mozilla.org/integration/mozilla-inbound/rev/f81e94b06315 https://hg.mozilla.org/integration/mozilla-inbound/rev/d4cceaf55796 https://hg.mozilla.org/integration/mozilla-inbound/rev/524b83d6ff75 https://hg.mozilla.org/integration/mozilla-inbound/rev/491765fa039c https://tbpl.mozilla.org/?tree=Try&rev=83bb3289b5c0

(In reply to Matt Woodrow (:mattwoodrow) from comment #16) > (In reply to Benoit Jacob [:bjacob] from comment #12) > > > ::: content/canvas/src/WebGLContextGL.cpp > > @@ +2646,1 @@ > > > *format = WebGLTexelFormat::None; > > > > Only one important nit: could you restore the first line in this function, > > i.e. make this function unconditionally set imageOut=nullptr at the > > beginning, so that we never exit leaving it with undefined value? > > RefPtr's are default initialized to nullptr already! Which is good, but not as good as preserving the guarantee that this function leaves imageOut with the value nullptr in error cases. In other words, I was counting "leaving imageOut with whatever value it had before calling this function" as an "undefined value" case. Please still consider restoring this 1 line of code!

https://hg.mozilla.org/mozilla-central/rev/e2a8de576daf https://hg.mozilla.org/mozilla-central/rev/c13412051f52 https://hg.mozilla.org/mozilla-central/rev/7b0f7f3dfa8e https://hg.mozilla.org/mozilla-central/rev/f81e94b06315 https://hg.mozilla.org/mozilla-central/rev/d4cceaf55796 https://hg.mozilla.org/mozilla-central/rev/524b83d6ff75 https://hg.mozilla.org/mozilla-central/rev/491765fa039c