Peter Van der Beken [:peterv] Assignee
	Description • 12 years ago

      No description provided.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

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?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 12 years ago

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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 12 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.

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 12 years ago

A 2D canvas can have a null mCanvasElement if it has a non-null docshell instead.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

Oh, also, why do you need the .get() in GetParentObject?

	Peter Van der Beken [:peterv] Assignee
	Comment 7 • 12 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.

	Peter Van der Beken [:peterv] Assignee
	Comment 8 • 12 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).

	Peter Van der Beken [:peterv] Assignee
	Comment 9 • 12 years ago

This allows a null canvas element (also see comment 8).

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 12 years ago

> 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)...

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 12 years ago

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.

	Peter Van der Beken [:peterv] Assignee
	Comment 12 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f38152fc9f6

	Ed Morley [:emorley]
	Comment 13 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/1f38152fc9f6