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?

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.

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.

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

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

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