911575 - Convert canvas callbacks to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 798296 [details] [diff] [review]
Patch

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

I'm not confident I can review this, sorry. (And you need a real peer for the test_interfaces.html change.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 798296 [details] [diff] [review]
Patch

Watch out for conflicts with bug 817700.

>+++ b/content/html/content/public/HTMLCanvasElement.h
>+              FileCallback aCallback,

Er, that compiles?  Gah.  Can you please nuke the copy ctor on CallbackObject so this won't compile?  ;)  You're _supposed_ to have a |FileCallback& aCallback| here.  And various other places in the patch.

>+  mozilla::dom::PrintCallback* GetMozPrintCallback() const;
>+  void SetMozPrintCallback(mozilla::dom::PrintCallback* aCallback);

This is already in the mozilla::dom namespace, no?

>+++ b/content/html/content/src/HTMLCanvasElement.cpp
>+    mCallback.Receive(&mCallback, mBlob, rv);

Why that first argument?  But also, why is FileCallback a callback interface instead of a callback function?  Seems like it should be a callback function.  Same for the print callback.


>+  mozilla::dom::FileCallback mCallback;

That needs to be an nsRefPtr.

> HTMLCanvasElement::HandlePrintCallback(nsPresContext::nsPresContextType aType)
>+  nsRefPtr<PrintCallback> printCallback;

Unused, right?

> HTMLCanvasElement::CallPrintCallback()
>+  ErrorResult rv;
>+  GetMozPrintCallback()->Render(this, *mPrintState, rv);

Again, why the first arg?

>+HTMLCanvasElement::ToBlob(JSContext* aCx,
>+                          FileCallback aCallback,

FileCallback&

>+++ b/dom/interfaces/html/nsIDOMHTMLCanvasElement.idl
>-  attribute nsIPrintCallback mozPrintCallback;

No webidl replacement for this?

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: Interdiff

Not sure why the classinfo bit is there, it's not actually in the patch.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 803039 [details] [diff] [review]
Interdiff

r=me

Comment 6

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/try/rev/8915a9df8a7e

Comment 7

[David Zbarsky (:dzbarsky)]

Oops, that was the try changeset. inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8be6a45beadb

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8be6a45beadb