Bug 48962

Summary: Broken WebGL tests due to r71274
Product: WebKit Reporter: Adrienne Walker <enne>
Component: WebGLAssignee: Adrienne Walker <enne>
Status: RESOLVED FIXED    
Severity: Normal CC: dumi, enne, kbr, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch kbr: review+

Description Adrienne Walker 2010-11-03 16:56:24 PDT
In LayoutTests/fast/canvas/webgl/, gl-object-get-calls and bad-arguments-test.html were broken because several cases of returning empty string or undefined were changed to null.
Comment 1 Adrienne Walker 2010-11-03 16:59:18 PDT
Created attachment 72885 [details]
Patch
Comment 2 Kenneth Russell 2010-11-03 17:05:40 PDT
Comment on attachment 72885 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72885&action=review

> LayoutTests/fast/canvas/webgl/gl-object-get-calls.html:43
> +shouldBeTrue('gl.getAttachedShaders(null) == undefined');

I don't think this should ever return undefined. See https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.14.3 . Why do we not use shouldBeNull here?

> WebCore/html/canvas/WebGLRenderingContext.cpp:1633
> +        return "";

How are these return values different? Does one translate to null at the JS level and one to an empty string?
Comment 3 Kenneth Russell 2010-11-03 17:07:10 PDT
Comment on attachment 72885 [details]
Patch

In the interest of un-breaking the tree I'm going to r+ this patch because it looks like the author is out for the rest of the day. However I'm leaving the bug open because I think a revision is necessary.
Comment 4 Adrienne Walker 2010-11-03 17:10:16 PDT
(In reply to comment #2)
> (From update of attachment 72885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72885&action=review
> 
> > LayoutTests/fast/canvas/webgl/gl-object-get-calls.html:43
> > +shouldBeTrue('gl.getAttachedShaders(null) == undefined');
> 
> I don't think this should ever return undefined. See https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.14.3 . Why do we not use shouldBeNull here?

I'm not sure what you're linking to there.  Isn't that about lost contexts?

In my reading of the spec, both null and undefined are valid return values in the case of an invalid program.

> > WebCore/html/canvas/WebGLRenderingContext.cpp:1633
> > +        return "";
> 
> How are these return values different? Does one translate to null at the JS level and one to an empty string?

That's exactly it.  I could add a comment to this effect.
Comment 5 Kenneth Russell 2010-11-03 17:14:42 PDT
(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 72885 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=72885&action=review
> > 
> > > LayoutTests/fast/canvas/webgl/gl-object-get-calls.html:43
> > > +shouldBeTrue('gl.getAttachedShaders(null) == undefined');
> > 
> > I don't think this should ever return undefined. See https://cvs.khronos.org/svn/repos/registry/trunk/public/webgl/doc/spec/WebGL-spec.html#5.14.3 . Why do we not use shouldBeNull here?
> 
> I'm not sure what you're linking to there.  Isn't that about lost contexts?

"All methods returning nullable values or any return null."

> In my reading of the spec, both null and undefined are valid return values in the case of an invalid program.

Implementations should not return undefined for WebGLShader[ ] under any circumstances. Where does the spec indicate that this might happen?

> > > WebCore/html/canvas/WebGLRenderingContext.cpp:1633
> > > +        return "";
> > 
> > How are these return values different? Does one translate to null at the JS level and one to an empty string?
> 
> That's exactly it.  I could add a comment to this effect.

Might be helpful.
Comment 6 Kenneth Russell 2010-11-03 17:26:29 PDT
Committed r71289: <http://trac.webkit.org/changeset/71289>
Comment 7 Kenneth Russell 2010-11-03 17:27:16 PDT
Accidentally closed this bug when necessarily landing the patch above to un-break the tree. Reopening.
Comment 8 Adrienne Walker 2010-11-03 18:10:56 PDT
(In reply to comment #7)
> Accidentally closed this bug when necessarily landing the patch above to un-break the tree. Reopening.

gl-object-get-calls.html is still showing up with an error because of one extra blank line at the end.  It ran cleanly, but my git settings stripped that line when I committed locally and uploaded.  I will rebaseline this test.