Closed Bug 948221 Opened 6 years ago Closed 6 years ago

Convert SurfaceFromElement to use Moz2D

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

Details

(Whiteboard: [qa-])

Attachments

(11 files)

4.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
943 bytes, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.01 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.27 KB, patch
roc
: review+
Details | Diff | Splinter Review
16.78 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.37 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
9.27 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
3.69 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → matt.woodrow
All the subpieces of Part 7 need to be landed together, I'll fold the patches before landing.
Attachment #8345030 - Flags: review?(roc)
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?
Attachment #8345033 - Flags: review?(bjacob) → review+
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 :-(
Attachment #8345023 - Flags: review?(roc) → review+
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
Attachment #8345030 - Flags: review?(roc) → review+
Attachment #8345025 - Flags: review?(bas) → review+
Attachment #8345027 - Flags: review?(bas) → review+
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!
Attachment #8345032 - Flags: review?(bas) → review+
(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!
(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!
Depends on: 973264
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.