Closed Bug 911575 Opened 6 years ago Closed 6 years ago

Convert canvas callbacks to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(2 files)

No description provided.
Attached patch PatchSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #798296 - Flags: review?(Ms2ger)
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.)
Attachment #798296 - Flags: review?(Ms2ger)
Attachment #798296 - Flags: review?(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?
Attachment #798296 - Flags: review?(bzbarsky) → review-
Attached patch InterdiffSplinter Review
Not sure why the classinfo bit is there, it's not actually in the patch.
Attachment #803039 - Flags: review?(bzbarsky)
Comment on attachment 803039 [details] [diff] [review]
Interdiff

r=me
Attachment #803039 - Flags: review?(bzbarsky) → review+
Oops, that was the try changeset. inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8be6a45beadb
https://hg.mozilla.org/mozilla-central/rev/8be6a45beadb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.