Convert canvas callbacks to WebIDL

RESOLVED FIXED in mozilla26

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 798296 [details] [diff] [review]
Patch
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)
(Assignee)

Updated

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

Comment 4

5 years ago
Created attachment 803039 [details] [diff] [review]
Interdiff

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

Comment 7

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.