Nick Cameron [:nrc] Reporter
	Description • 12 years ago

Canvas now uses the new dom bindings but its helper classes (TextMetrics, CavnasGradient, etc.) use the old ones. They should be converted.

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

This bug involves creating WebIDL interfaces (see [1]) to replace nsIDOMCanvasGradient, nsIDOMCanvasPattern, and nsIDOMTextMetrics (see [2-4]).

[1] https://developer.mozilla.org/en/Mozilla/WebIDL_bindings
[2] XPIDL: http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl#16
[3] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.h#38
[4] http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.cpp#159

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

I'm not sure how good-first-bug this one is, because it's not clear to me what to parent the objects to.  They're not tied to a particular canvas context, after all...

This is going to keep coming up a lot.  :(

	Sankha Narayan Guria [:sankha] Assignee
	Comment 3 • 12 years ago

I would like to work on this. But I am new to writing WebIDL interfaces, so I might need a bit help there.

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

Ms2ger and I are happy to help.  Please feel free to send mail or ping on IRC!

	Sankha Narayan Guria [:sankha] Assignee
	Comment 5 • 12 years ago

As I talked to Ms2ger on IRC, he said to implement CanvasPattern first from http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#canvaspattern

But I don't find any SVGMatrix type or class in http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl . So is there any other substitute to SVGMatrix?

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

Nope.  Just comment out setTransform in the IDL: we don't implement it at the moment, and implementing it is not part of this bug at all.  ;)

	Sankha Narayan Guria [:sankha] Assignee
	Comment 7 • 12 years ago

I just converted the CanvasPattern to WebIDL. Is there anything more to be done for CanvasPattern in the sources? Once the Canvaspattern class is done, I'll start off with the others.

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

You'll need to update the implementation class:

http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/CanvasRenderingContext2D.h#89

to inherit from nsWrapperCache and add "SetIsDOMBinding()" to the constructor, for starters. See also <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings>.

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

Comment on attachment 687673 [details] [diff] [review]
CanvasPattern WebIDL implementation

Indeed.  I'm a little surprised this compiles, actually...  It's a good start, but the wrapper caching bits need to happen.

You might also need changes to the WrapStyle function in CanvasRenderingContext2D.cpp, and the GetStyleAsStringOrInterface bits.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 10 • 12 years ago

This is a WIP patch. Sorry because of the weekend, I could not ask on IRC. I am getting build errors on this. The specific error can be found at http://pastebin.mozilla.org/1982236

Could you help me out with this?

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

See https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class step 6?

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

And adding the missing '; there might help too.  ;)

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

Comment on attachment 690058 [details] [diff] [review]
WIP patch

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

::: content/canvas/src/CanvasRenderingContext2D.h
@@ +122,5 @@
>  
> +  virtual JSObject* WrapObject(JSContext *cx, JSObject *scope,
> +                               bool *triedToWrap)
> +  {
> +    return mozilla::dom::CanvasPatternBinding::WrapObject(cx, scope, this, triedToWrap)

Wrap, not WrapObject; and a ';' as bz mentioned.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 14 • 12 years ago

I made the changes, but still getting the CanvasPatternBinding not declared error.

 0:56.03 In file included from /home/sankha/oss/nightly/mozilla-central/content/canvas/src/CanvasRenderingContext2D.cpp:7:0:
 0:56.03 /home/sankha/oss/nightly/mozilla-central/content/canvas/src/CanvasRenderingContext2D.h: In member function ‘virtual JSObject* mozilla::dom::CanvasPattern::WrapObject(JSContext*, JSObject*, bool*)’:
 0:56.03 /home/sankha/oss/nightly/mozilla-central/content/canvas/src/CanvasRenderingContext2D.h:119:26: error: ‘mozilla::dom::CanvasPatternBinding’ has not been declared
 0:56.61 ../../../../dist/include/mozilla/dom/CanvasRenderingContext2D.h:120:3: error: control reaches end of non-void function [-Werror=return-type]

==================
I tried the step 7 in https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Adding_WebIDL_bindings_to_a_class to generate CanvasPattern-example.cpp, which had CanvasPatternBinding declared. So I am unable to understand why does it say undeclared?

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

You lost the following hunk from http://pastebin.mozilla.org/1982557 somewhere:

diff --git a/content/canvas/src/CanvasRenderingContext2D.h b/content/canvas/src/CanvasRenderingContext2D.h
--- a/content/canvas/src/CanvasRenderingContext2D.h
+++ b/content/canvas/src/CanvasRenderingContext2D.h
@@ -12,16 +12,17 @@
 #include "nsColor.h"
 #include "nsHTMLCanvasElement.h"
 #include "nsHTMLVideoElement.h"
 #include "CanvasUtils.h"
 #include "gfxFont.h"
 #include "mozilla/ErrorResult.h"
 #include "mozilla/dom/ImageData.h"
 #include "mozilla/dom/UnionTypes.h"
+#include "mozilla/dom/CanvasPatternBinding.h"
 
 #define NS_CANVASGRADIENTAZURE_PRIVATE_IID \
     {0x28425a6a, 0x90e0, 0x4d42, {0x9c, 0x75, 0xff, 0x60, 0x09, 0xb3, 0x10, 0xa8}}
 #define NS_CANVASPATTERNAZURE_PRIVATE_IID \
     {0xc9bacc25, 0x28da, 0x421e, {0x9a, 0x4b, 0xbb, 0xd6, 0x93, 0x05, 0x12, 0xbc}}
 
 class nsIDOMXULElement;

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

Comment on attachment 690125 [details] [diff] [review]
Does not compile

What ms2ger said.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 17 • 12 years ago

This moves the CanvasPattern interface to WebIDL. Now I am beginning to work on TextMetrics.

	Sankha Narayan Guria [:sankha] Assignee
	Comment 18 • 12 years ago

Patch after addressing bz's comments on IRC.

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

Comment on attachment 691773 [details] [diff] [review]
Part 1: Moves CanvasPattern to WebIDL

That seems much more plausible.

Have you run the canvas tests?  Or at least manual fillStyle/strokeStyle tests?

	Sankha Narayan Guria [:sankha] Assignee
	Comment 20 • 12 years ago

This patch tries to move TextMetrics interface to WebIDL, but it doesn't compile. The errors that I'm getting are here: http://pastebin.mozilla.org/2012105

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

Comment on attachment 694914 [details] [diff] [review]
Part 2: Moves TextMetrics to WebIDL

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

I think you forgot to "hg add" the webidl file?

::: content/canvas/src/CanvasRenderingContext2D.h
@@ -263,5 @@
>    void StrokeText(const nsAString& text, double x, double y,
>                    const mozilla::dom::Optional<double>& maxWidth,
>                    mozilla::ErrorResult& error);
> -  already_AddRefed<nsIDOMTextMetrics>
> -    MeasureText(const nsAString& rawText, mozilla::ErrorResult& error);

This seems to have moved somewhere it shouldn't have

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

Comment on attachment 694914 [details] [diff] [review]
Part 2: Moves TextMetrics to WebIDL

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

Can you check if the issues I pointed out in comment 21 helps?