Bug 163853 – Move HTML canvas and tracks from ExceptionCode to Exception

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Darin Adler]

Move HTML canvas and tracks from ExceptionCode to Exception

[Darin Adler]

Created attachment 292506 [details]
Patch

[Darin Adler]

Created attachment 292507 [details]
Patch

[Darin Adler]

I had switched to another branch. Switching back to see why the tests are failing.

[Build Bot]

Comment on attachment 292507 [details]
Patch

Attachment 292507 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2346828

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
media/track/regions-webvtt/vtt-region-constructor.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html

[Build Bot]

Created attachment 292510 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292507 [details]
Patch

Attachment 292507 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2346840

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
media/track/regions-webvtt/vtt-region-constructor.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 292512 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292507 [details]
Patch

Attachment 292507 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2346841

New failing tests:
http/tests/canvas/webgl/origin-clean-conformance.html
media/track/regions-webvtt/vtt-region-constructor.html
http/tests/webgl/1.0.2/texSubImage2DHTML.html

[Build Bot]

Created attachment 292514 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292507 [details]
Patch

Attachment 292507 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2346844

New failing tests:
media/track/regions-webvtt/vtt-region-constructor.html

[Build Bot]

Created attachment 292515 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 292518 [details]
Patch

[Darin Adler]

Created attachment 292519 [details]
Patch

[Darin Adler]

New patch is merged with the changes from bug 163856.

[Chris Dumez]

Comment on attachment 292518 [details]
Patch

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

Will continue reviewing on the new iteration.

> Source/WebCore/bindings/js/JSWebGL2RenderingContextCustom.cpp:101
> +    if (state.argumentCount() != 2)

This should really be:
if (state.argumentCount() < 2)

It does not make sense to throw if more parameters are provided.

> Source/WebCore/bindings/js/JSWebGLRenderingContextBaseCustom.cpp:333
> +    String name = state.uncheckedArgument(0).toString(&state)->value(&state);

toWTFString() ?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1948
> +ExceptionOr<RefPtr<ImageData>> CanvasRenderingContext2D::createImageData(float sw, float sh) const

Not a big deal but the spec seems to use double instead of float.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1966
> +ExceptionOr<RefPtr<ImageData>> CanvasRenderingContext2D::getImageData(float sx, float sy, float sw, float sh) const

Not a big deal but the spec seems to use double instead of float.

[Chris Dumez]

Comment on attachment 292519 [details]
Patch

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

r=me with comments

> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:84
> +    auto& context = static_cast<WebGLRenderingContext&>(*m_context);

downcast<>() would be safer.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1186
> +                appendChild(displayBox.get(), ASSERT_NO_EXCEPTION);

I believe the .get() is redundant.

[Darin Adler]

Comment on attachment 292518 [details]
Patch

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

>> Source/WebCore/bindings/js/JSWebGL2RenderingContextCustom.cpp:101
>> +    if (state.argumentCount() != 2)
> 
> This should really be:
> if (state.argumentCount() < 2)
> 
> It does not make sense to throw if more parameters are provided.

Fixed, but I did not add a test.

>> Source/WebCore/bindings/js/JSWebGLRenderingContextBaseCustom.cpp:333
>> +    String name = state.uncheckedArgument(0).toString(&state)->value(&state);
> 
> toWTFString() ?

Done.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1948
>> +ExceptionOr<RefPtr<ImageData>> CanvasRenderingContext2D::createImageData(float sw, float sh) const
> 
> Not a big deal but the spec seems to use double instead of float.

Moving to double would be a big project; not worth doing just the arguments to createImageData; I think we’d want to change many FloatSize and FloatRect sites to use double as well. At the time we originally wrote all of this, float happened to match Core Graphics coordinates. But now, on 64-bit systems, those are double as well. I am not going to change anything here at this time, but I think it may be worth doing this later.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1966
>> +ExceptionOr<RefPtr<ImageData>> CanvasRenderingContext2D::getImageData(float sx, float sy, float sw, float sh) const
> 
> Not a big deal but the spec seems to use double instead of float.

Ditto.

[Darin Adler]

Comment on attachment 292519 [details]
Patch

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

>> Source/WebCore/html/canvas/OESVertexArrayObject.cpp:84
>> +    auto& context = static_cast<WebGLRenderingContext&>(*m_context);
> 
> downcast<>() would be safer.

Done.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1186
>> +                appendChild(displayBox.get(), ASSERT_NO_EXCEPTION);
> 
> I believe the .get() is redundant.

Done.

[Darin Adler]

Committed r207720: <http://trac.webkit.org/changeset/207720>

[Csaba Osztrogonác]

(In reply to comment #19)
> Committed r207720: <http://trac.webkit.org/changeset/207720>

It broke the Apple Windows build, see build.webkit.org for details.

[Darin Adler]

(In reply to comment #20)
> (In reply to comment #19)
> > Committed r207720: <http://trac.webkit.org/changeset/207720>
> 
> It broke the Apple Windows build, see build.webkit.org for details.

I fixed one particular Windows build break I saw on a later EWS run in this check-in:

https://trac.webkit.org/changeset/207763

That might or might not have been enough. Looks like the Windows build is now failing because of CSSCharsetRule.h.

[Alex Christensen]

r207779

[Chris Dumez]

This seems to have regressed:
- https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/context/context-lost.html
- https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/more/conformance/quickCheckAPI-S_V.html

I am taking a look.