Closed Bug 534735 Opened 13 years ago Closed 13 years ago

Use some custom quickstubs for style

Categories

(Core :: Graphics: Canvas2D, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: vlad)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch v0 (obsolete) — Splinter Review
(Patch might need updating a little)
Assignee: nobody → vladimir
Blocks: 536564
Attached patch v1 (obsolete) — Splinter Review
Attachment #420763 - Attachment is obsolete: true
Attached patch v2 (obsolete) — Splinter Review
Updated and ready to go; all dependencies are in place.
Attachment #420845 - Attachment is obsolete: true
Attachment #421666 - Flags: review?(pavlov)
Comment on attachment 421666 [details] [diff] [review]
v2

Moving review over to bz -- custom quickstubs for canvas fillStyle/strokeStyle properties.
Attachment #421666 - Flags: review?(pavlov) → review?(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?
Attachment #421666 - Flags: review?(bzbarsky) → review-
(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.
Attached patch v3Splinter Review
Updated, also used member function pointers to simplify the .h file.
Attachment #421666 - Attachment is obsolete: true
Attachment #426092 - Flags: review?(bzbarsky)
Comment on attachment 426092 [details] [diff] [review]
v3

r=bzbarsky
Attachment #426092 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/a3e23b42b410
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.