Closed Bug 762651 Opened 7 years ago Closed 7 years ago

Add wrappercache to CanvasRenderingContext2D

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: peterv, Assigned: peterv)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 762654
Attached patch v1 (obsolete) — Splinter Review
Attachment #631109 - Flags: review?(bzbarsky)
Comment on attachment 631109 [details] [diff] [review]
v1

smaug should probably review the skippability stuff.

This generally looks good, but I'm confused by the change in NS_DOMReadStructuredClone.  That gives the object a null parent, no?  How will that work?
Attachment #631109 - Flags: review?(bugs)
Comment on attachment 631109 [details] [diff] [review]
v1

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

::: content/canvas/public/nsICanvasRenderingContextInternal.h
@@ +49,5 @@
>    void SetCanvasElement(nsHTMLCanvasElement* aParentCanvas)
>    {
>      mCanvasElement = aParentCanvas;
>    }
> +  nsHTMLCanvasElement *GetParentObject() const {

* to the left

::: content/canvas/src/CustomQS_Canvas2D.h
@@ +129,5 @@
> +    nsIDOMCanvasRenderingContext2D *self;
> +    xpc_qsSelfRef selfref;
> +    JS::AutoValueRooter tvr(cx);
> +    if (!xpc_qsUnwrapThis(cx, obj, &self, &selfref.ptr, tvr.jsval_addr(), nsnull))
> +        return JS_FALSE;

false

@@ +136,5 @@
> +    self->GetCanvas(getter_AddRefs(canvas));
> +    nsCOMPtr<nsIContent> content = do_QueryInterface(canvas);
> +    nsHTMLCanvasElement* element = nsHTMLCanvasElement::FromContent(content);
> +    if (!element) {
> +      return NS_ERROR_FAILURE;

xpc_qsThrow(cx, NS_ERROR_FAILURE);

::: dom/base/nsDOMClassInfo.h
@@ +1603,5 @@
> +  NS_IMETHOD PostCreatePrototype(JSContext * cx, JSObject * proto) {
> +    nsresult rv = nsDOMGenericSH::PostCreatePrototype(cx, proto);
> +    if (NS_SUCCEEDED(rv)) {
> +      if (!::JS_DefineProperty(cx, proto, "VIEWPORT", INT_TO_JSVAL(0x0BA2),
> +                               nsnull, nsnull, JSPROP_ENUMERATE))

We needed this for bug 586938, but our WebIDL interface does list VIEWPORT. Can this go now?
Comment on attachment 631109 [details] [diff] [review]
v1


>+  nsHTMLCanvasElement *GetParentObject() const {
Nnit, { should be in the next line


>+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(nsCanvasRenderingContext2DAzure)
>+ if (nsCCUncollectableMarker::sGeneration && tmp->IsBlack()) {
>+    nsGenericElement* canvasElement = tmp->mCanvasElement;
>+    if (canvasElement->IsPurple()) {
>+      canvasElement->RemovePurple();
>+    }
>+    nsGenericElement::MarkNodeChildren(canvasElement);
>+    return true;
>+  }
>+NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
Is it guaranteed that mCanvasElement is never null here?
Perhaps add a null check. That would be at least consistent 
with the rest of the canvas code where mCanvasElement is 
null-checked.
Attachment #631109 - Flags: review?(bugs) → review+
A 2D canvas can have a null mCanvasElement if it has a non-null docshell instead.
Oh, also, why do you need the .get() in GetParentObject?
Comment on attachment 631109 [details] [diff] [review]
v1

Bz is working on a patch that will make us not need to add nsWrapperCache to ImageData.
Attachment #631109 - Flags: review?(bzbarsky)
Attachment #631109 - Flags: review+
(In reply to Boris Zbarsky (:bz) from comment #5)
> A 2D canvas can have a null mCanvasElement if it has a non-null docshell
> instead.

Which is never set in nsCanvasRenderingContext2DAzure (InitializeWithSurface is not implemented, like in WebGLContext).

I wonder if we should make nsCanvasRenderingContextSH::PreCreate/nsCanvasRenderingContext2DAzure::WrapObject fail if mCanvasElement is null? It would force script code that wants a canvas rendering context to use a canvas element to get one, where currently one can probably use createInstance to create one. Or we could leave it as is and in that case create the wrapper in the caller's compartment (like we do for XMLHttpRequest).
Attached patch v2Splinter Review
This allows a null canvas element (also see comment 8).
Attachment #631109 - Attachment is obsolete: true
Attachment #633140 - Flags: review?(bzbarsky)
Summary: Add wrappercache to CanvasRenderingContext2D and ImageData → Add wrappercache to CanvasRenderingContext2D
> Which is never set in nsCanvasRenderingContext2DAzure 

For now, yeah.  The only consumer of InitializeWithSurface is TaskbarPreview.cpp and it explicitly creates a thebes canvas by contract.

> I wonder if we should make
> nsCanvasRenderingContextSH::PreCreate/nsCanvasRenderingContext2DAzure::WrapObject fail if
> mCanvasElement is null?

Sounds great to me.  I don't see any hits on the thebes canvas contract in the addons mxr (out-of-date though it may be)...
Comment on attachment 633140 [details] [diff] [review]
v2

>+xpc_qsUnwrapArg<mozilla::dom::ImageData>(JSContext *cx, jsval v,
>+    nsIDOMImageData* arg = *ppArg;
>+    nsIDOMImageData* argRef = *ppArgRef;

You don't need to init those, since you don't write back to *ppArg and *ppArgRef unless NS_SUCCEEDED(rv).

r=me with that.
Attachment #633140 - Flags: review?(bzbarsky) → review+
Keywords: dev-doc-needed
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/1f38152fc9f6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.