Bug 163853

Summary: Move HTML canvas and tracks from ExceptionCode to Exception
Product: WebKit Reporter: Darin Adler <darin>
Component: BindingsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, buildbot, cdumez, ossy, rniwa, sam, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=163856
Bug Depends on: 168632    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Patch
none
Patch cdumez: review+

Description Darin Adler 2016-10-22 12:55:41 PDT
Move HTML canvas and tracks from ExceptionCode to Exception
Comment 1 Darin Adler 2016-10-22 13:53:16 PDT
Created attachment 292506 [details]
Patch
Comment 2 Darin Adler 2016-10-22 14:12:08 PDT
Created attachment 292507 [details]
Patch
Comment 3 Darin Adler 2016-10-22 14:54:46 PDT
I had switched to another branch. Switching back to see why the tests are failing.
Comment 4 Build Bot 2016-10-22 15:11:19 PDT
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
Comment 5 Build Bot 2016-10-22 15:11:23 PDT
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
Comment 6 Build Bot 2016-10-22 15:15:36 PDT
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
Comment 7 Build Bot 2016-10-22 15:15:40 PDT
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
Comment 8 Build Bot 2016-10-22 15:21:19 PDT
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
Comment 9 Build Bot 2016-10-22 15:21:23 PDT
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
Comment 10 Build Bot 2016-10-22 15:27:00 PDT
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
Comment 11 Build Bot 2016-10-22 15:27:04 PDT
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
Comment 12 Darin Adler 2016-10-22 16:29:09 PDT
Created attachment 292518 [details]
Patch
Comment 13 Darin Adler 2016-10-22 16:44:54 PDT
Created attachment 292519 [details]
Patch
Comment 14 Darin Adler 2016-10-22 16:45:17 PDT
New patch is merged with the changes from bug 163856.
Comment 15 Chris Dumez 2016-10-22 16:45:45 PDT
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.
Comment 16 Chris Dumez 2016-10-22 17:06:27 PDT
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.
Comment 17 Darin Adler 2016-10-22 18:06:04 PDT
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.
Comment 18 Darin Adler 2016-10-22 18:06:33 PDT
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.
Comment 19 Darin Adler 2016-10-22 18:07:56 PDT
Committed r207720: <http://trac.webkit.org/changeset/207720>
Comment 20 Csaba Osztrogonác 2016-10-24 11:41:08 PDT
(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.
Comment 21 Darin Adler 2016-10-24 12:33:17 PDT
(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.
Comment 22 Alex Christensen 2016-10-24 14:20:10 PDT
r207779