[webkit-reviews] review granted: [Bug 75053] Fix semantics of WEBKIT_WEBGL_lose_context : [Attachment 121165] Patch

Wed Jan 4 15:20:06 PST 2012

James Robinson <jamesr at chromium.org> has granted Kenneth Russell
<kbr at google.com>'s request for review:
Bug 75053: Fix semantics of WEBKIT_WEBGL_lose_context
https://bugs.webkit.org/show_bug.cgi?id=75053

Attachment 121165: Patch
https://bugs.webkit.org/attachment.cgi?id=121165&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121165&action=review

R=me. there are a number of setTimeout(..., 1)s which worry me a bit, we
normally construct tests so the timeout value doesn't matter and pass 0 in just
to be clear. commented on them inline. otherwise looks great

> LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:40
> +	   }, 1);

the choice of '1' here is a little surprising, normally we use 0 delays for
tests to get a clean callstack. if the '1' is significant then i worry this is
flaky

> LayoutTests/fast/canvas/webgl/WebGLContextEvent.html:55
> +	   }, 1);

same here - should probably be 0

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:1
>  <html>

why doesn't this test have an HTML5 doctype "<!DOCTYPE html>" ? our tests
normally do unless they're specifically for quirks mode

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:3
> +<meta charset="utf-8">

i don't think we normally do this

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:70
> +	  }, 1);

0 here, please

> LayoutTests/fast/canvas/webgl/context-lost-restored.html:108
> +	 }, 1);

0

> LayoutTests/fast/canvas/webgl/context-lost.html:3
> +<meta charset="utf-8">

this is unusual. can this test have a <!DOCTYPE html> please?