Closed
Bug 861938
Opened 11 years ago
Closed 11 years ago
JS demo does not run. http://js1k.com/2010-xmas/demo/856
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: mayankleoboy1, Assigned: bzbarsky)
References
()
Details
(Keywords: regression)
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Keywords: regression
Reporter | ||
Comment 1•11 years ago
|
||
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
Comment 5•11 years ago
|
||
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
![]() |
Assignee | |
Comment 7•11 years ago
|
||
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)
Comment 8•11 years ago
|
||
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
![]() |
Assignee | |
Comment 9•11 years ago
|
||
But not a layout bug at all. ;)
Component: Layout: View Rendering → Canvas: 2D
Comment 10•11 years ago
|
||
(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.
Comment 11•11 years ago
|
||
(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?
Comment 12•11 years ago
|
||
(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)
![]() |
Assignee | |
Comment 13•11 years ago
|
||
How would two signatures help?
Comment 14•11 years ago
|
||
(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 | |
Comment 15•11 years ago
|
||
The clean way to fix this is to fix bug 829248.
Attachment #755018 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•11 years ago
|
Assignee: nobody → bzbarsky
Comment 16•11 years ago
|
||
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+
Comment 17•11 years ago
|
||
(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?
![]() |
Assignee | |
Comment 18•11 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 19•11 years ago
|
||
I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=22218 on the spec.
![]() |
Assignee | |
Comment 20•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25c0e2489f2
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla24
Comment 21•11 years ago
|
||
(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?
![]() |
Assignee | |
Comment 22•11 years ago
|
||
Yep. See bug 829248.
Comment 23•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a25c0e2489f2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•