534735 - Use some custom quickstubs for style

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

strokeStyle/fillStyle currently use nsIVariant, which isn't quickstubbed and causes trace aborts.  Use the custom QS functionality from bug 534733 to quickstub them.

Note: get/putImageData can also be quickstubbed in the same way, but I'm waiting on bug 534467 and bug 532774 to do that.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v0

(Patch might need updating a little)

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v1

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v2

Updated and ready to go; all dependencies are in place.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 421666 [details] [diff] [review]
v2

Moving review over to bz -- custom quickstubs for canvas fillStyle/strokeStyle properties.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421666 [details] [diff] [review]
v2

>+++ b/content/canvas/src/CustomQS_Canvas2D.h

The fill and stroke code is identical except for s/Fill/Stroke/, right?  I almost wonder whether we should only have one copy of it in a macro (since I assume a template can't do the trick here)....

>+++ b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
>+  // These are dummy attribute, used to generate the quickstubs.
>+  attribute long strokeStyle;
>+  attribute long fillStyle;

This seems like it would break the Java and Python bindings, no?  Is there a reason those can't stay as nsIVariant attributes, along with the code that implements them right now?

>+  // should be specified, the one that's not null/void is used.

s/,/;/

>  For the getter,
>+  // ifaceType is 0 if it's a string, 1 if it's a pattern, or 2 if it's a gradient

Can we just put some named constants on this interface instead?

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> (From update of attachment 421666 [details] [diff] [review])
> >+++ b/content/canvas/src/CustomQS_Canvas2D.h
> 
> The fill and stroke code is identical except for s/Fill/Stroke/, right?  I
> almost wonder whether we should only have one copy of it in a macro (since I
> assume a template can't do the trick here)....

Maybe, though large macros are also kinda painful to maintain.  Might be possible to do it by passing in the function pointers for the specific setters.. I can try.

> >+++ b/dom/interfaces/canvas/nsIDOMCanvasRenderingContext2D.idl
> >+  // These are dummy attribute, used to generate the quickstubs.
> >+  attribute long strokeStyle;
> >+  attribute long fillStyle;
> 
> This seems like it would break the Java and Python bindings, no?  Is there a
> reason those can't stay as nsIVariant attributes, along with the code that
> implements them right now?

I'll be honest: I don't care about the Java and Python bindings.  I actually consider them a bug, and was going to bring that up at the platform work week.  But, as you say, these could probably become nsIVariant and just use the setter/getter functions after extracting whatever the thing is from the variant.

> >+  // should be specified, the one that's not null/void is used.
> 
> s/,/;/
> 
> >  For the getter,
> >+  // ifaceType is 0 if it's a string, 1 if it's a pattern, or 2 if it's a gradient
> 
> Can we just put some named constants on this interface instead?

Yeah, could.  Will fix.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: v3

Updated, also used member function pointers to simplify the .h file.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 426092 [details] [diff] [review]
v3

r=bzbarsky

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

http://hg.mozilla.org/mozilla-central/rev/a3e23b42b410