Add wrappercache to CanvasRenderingContext2D

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Blocks: 762654
(Assignee)

Comment 1

5 years ago
Created attachment 631109 [details] [diff] [review]
v1
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 4

5 years ago
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.
Blocks: 750308
Oh, also, why do you need the .get() in GetParentObject?
Blocks: 750297
(Assignee)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
(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).
(Assignee)

Comment 9

5 years ago
Created attachment 633140 [details] [diff] [review]
v2

This allows a null canvas element (also see comment 8).
Attachment #631109 - Attachment is obsolete: true
Attachment #633140 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f38152fc9f6
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/1f38152fc9f6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 778128
You need to log in before you can comment on or make changes to this bug.