Closed Bug 861938 Opened 7 years ago Closed 7 years ago

JS demo does not run. http://js1k.com/2010-xmas/demo/856

Categories

(Core :: Canvas: 2D, defect)

23 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: mayankleoboy1, Assigned: bzbarsky)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130415 Firefox/23.0
Build ID: 20130415030812

Steps to reproduce:

1. Create fresh profile
2. Go to http://js1k.com/2010-xmas/demo/856



Actual results:

Nothing happened.


Expected results:

A christmas tree should have appeared, with snow and some red balls.

Works in FF19. So a regression.
In fact, there are some other demos on this site that dont run on the Nightly, but run on older versions.
Attachment #737988 - Attachment mime type: text/plain → text/html
Regression range is

Last good nightly: 2013-01-16
First bad nightly: 2013-01-17

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d8be4bc4fba8&tochange=712eca11a04e
The problem is caused by the page script calling this function:

CanvasRenderingContext2D::Fill()

with |undefined| as a parameter. That now throws an exception, whereas previously it didn't.

The function is documented on MDN as having no parameters[1], but looking at the code it does take one parameter of type const CanvasWindingRule&.

[1] https://developer.mozilla.org/en-US/docs/DOM/CanvasRenderingContext2D#fill%28%29
Boris, this looks like a change in how undefined values are treated when passed to methods expecting enums. The only possibility I see in the regression range is bug 824864; do you have a better suspect?
I suspect it's this one...

changeset:   119115:64450d6fee96
user:        Rik Cabanier <cabanier@adobe.com>
date:        Wed Jan 16 21:55:43 2013 -0500
summary:     Bug 827053 - Add support for winding in fill + clip + isPointInPath + tests the feature. r=bas
Yep, comment 6 is spot on.  The way the IDL (and the spec so far) is written passing undefined is NOT the same as not passing the argument: the latter uses the default value while the latter tries to stringify to "undefined" and then that's not a valid enum value so it throws an exception.

This demo does not run in a current Chrome dev build, for presumably the same reason.

I suppose we could change the IDL to [TreatUndefinedAs=Missing], since that's what the ES folks want all WebIDL to move to by default anyway...  Rik?
Flags: needinfo?(cabanier)
Probably there was no need to confirm this one, but:
Confirmed on Windows 7, 64 bits, on Firefox 21.0, on Aurora 23.0 and on latest Nightly 24.0:

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0, Build ID: 20130511120803

- User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130523 Firefox/23.0, Build ID: 20130523004551

- User agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:24.0) Gecko/20130523 Firefox/24.0, Build ID: 20130523030935

Reporter, you have done an excellent work with this bug, thank you for your contribution!
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout: View Rendering
Ever confirmed: true
Product: Firefox → Core
But not a layout bug at all.  ;)
Component: Layout: View Rendering → Canvas: 2D
(In reply to Boris Zbarsky (:bz) from comment #7)
> 
> I suppose we could change the IDL to [TreatUndefinedAs=Missing], since
> that's what the ES folks want all WebIDL to move to by default anyway... 
> Rik?

Yes, this change should have been backward compatible. I will try to submit a fix for this tomorrow.
(In reply to mayankleoboy1 from comment #1)
> In fact, there are some other demos on this site that dont run on the
> Nightly, but run on older versions.

Could you also file a bug against WebKit and Chrome?
(In reply to Rik Cabanier from comment #10)
> (In reply to Boris Zbarsky (:bz) from comment #7)
> > 
> > I suppose we could change the IDL to [TreatUndefinedAs=Missing], since
> > that's what the ES folks want all WebIDL to move to by default anyway... 
> > Rik?
> 
> Yes, this change should have been backward compatible. I will try to submit
> a fix for this tomorrow.

I can't submit a fix because [TreatUndefinedAs=Missing] is not implemented yet.
(https://bugzilla.mozilla.org/show_bug.cgi?id=829248)

I could work around this by having 2 signatures for 'fill'.
Does that sound OK?
Flags: needinfo?(cabanier)
How would two signatures help?
(In reply to Boris Zbarsky (:bz) from comment #13)
> How would two signatures help?

yes. I just tried it and it makes no difference. It still takes the 'enum' route which then fails.
I'm unsure how I can cleanly fix this.
Assignee: nobody → bzbarsky
Depends on: 829248
Whiteboard: [need review]
Comment on attachment 755018 [details] [diff] [review]
Make the CanvasWindingRule arguments be treated as missing if undefined is passed in.

Don't we need to change the spec too?
Attachment #755018 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> Comment on attachment 755018 [details] [diff] [review]
> Make the CanvasWindingRule arguments be treated as missing if undefined is
> passed in.
> 
> Don't we need to change the spec too?

It seems this would apply to every function that currently doesn't take arguments.
"I suppose we could change the IDL to [TreatUndefinedAs=Missing], since that's what the ES folks want all WebIDL to move to by default anyway"

If that's the case, why don't we change the code that generates the C++ binding so undefined parameters for optional arguments are allowed?
> If that's the case, why don't we change the code that generates the C++ binding so
> undefined parameters for optional arguments are allowed?

Because I haven't been able to get a clear commitment out of the other WebIDL implementors that they plan to do that behavior.

So for now, the spec needs [TreatUndefinedAs=Missing] to work like we want it to.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25c0e2489f2
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
(In reply to Boris Zbarsky (:bz) from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a25c0e2489f2

Does this mean that "[TreatUndefinedAs=Missing]" is implemented?
https://hg.mozilla.org/mozilla-central/rev/a25c0e2489f2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.