Bug 149984 – Support createPattern(HTMLVideoElement...

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Dean Jackson]

createPattern can accept an HTMLVideoElement
https://html.spec.whatwg.org/multipage/scripting.html#dom-context-2d-createpattern

[Radar WebKit Bug Importer]

<rdar://problem/23058823>

[George Ruan]

Created attachment 279917 [details]
Patch

[Dean Jackson]

Comment on attachment 279917 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1809
> +RefPtr<CanvasPattern> CanvasRenderingContext2D::createPattern(HTMLVideoElement* videoElement,
> +    const String& repetitionType, ExceptionCode& ec)

Style nit: Don't break the line here.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1833
> +    RefPtr<Image> currentFrame = imageBuffer->copyImage();
> +    
> +    return CanvasPattern::create(currentFrame, repeatX, repeatY, canvas()->originClean());

No need for the local variable.

> LayoutTests/media/video-canvas-createPattern.html:55
> +                expectedResults.forEach(function(element) {
> +                  checkColorAtLocation(ctx, element[0], element[1], element[2], element[3], element[4], 2);
> +                });

Nit: Four space indentation.

[Eric Carlson]

Comment on attachment 279917 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1829
> +    GraphicsContext& ctx = imageBuffer->context();

No need to declare this as a local variable either.

> LayoutTests/media/video-canvas-createPattern.html:60
> +                  video.currentTime += 0.24;
> +                  return;

Nit: please use four space indentation here as well.

[George Ruan]

Created attachment 279937 [details]
In Progress

[George Ruan]

Created attachment 279938 [details]
In Progress - Fixed Comments from Dean and Eric

[Tim Horton]

Comment on attachment 279917 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1828
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(*videoElement), Unaccelerated);

Why explicitly Unaccelerated? It looks like paintCurrentFrameInContext sometimes uses a CVPixelBuffer-backed CGImage, and it may (should?) be more efficient to paint that into an accelerated ImageBuffer (and is *certainly* more efficient to paint from an accelerated ImageBuffer into the likely-accelerated canvas).

Seems like all of the surrounding code is a little wishy-washy about this, not just your new code, so this might be a future adjustment instead of something to fix in your patch.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1831
> +    RefPtr<Image> currentFrame = imageBuffer->copyImage();

If you decide that it's right to make the ImageBuffer sometimes accelerated, and since this is a short-lived ImageBuffer, you should use ImageBuffer::sinkIntoImage() here so that we can use the more efficient CreateImageReference.

[George Ruan]

Created attachment 280205 [details]
Patch

[George Ruan]

(In reply to comment #8)
> Created attachment 280205 [details]
> Patch

All of the security tests are literally the same except that the src and crossOrigin property differ.

[Build Bot]

Comment on attachment 280205 [details]
Patch

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

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html

[Build Bot]

Created attachment 280206 [details]
Archive of layout-test-results from ews100 for mac-yosemite

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

[Build Bot]

Comment on attachment 280205 [details]
Patch

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

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 280205 [details]
Patch

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

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
canvas/philip/tests/2d.pattern.image.null.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 280205 [details]
Patch

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

New failing tests:
fast/canvas/canvas-createPattern-video-invalid.html
media/video-canvas-createPattern.html
canvas/philip/tests/2d.pattern.image.null.html

[Build Bot]

Created attachment 280209 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Myles C. Maxfield]

Comment on attachment 280205 [details]
Patch

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

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:7
> +<script src="script-tests/canvas-createPattern-video-invalid.js"></script>

Why not put the source of this script inline?

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:7
> +<script src="script-tests/canvas-createPattern-video-loading.js"></script>

Ditto.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:7
> +<script src="script-tests/canvas-createPattern-video-modify.js"></script>

Ditto.

> LayoutTests/fast/canvas/script-tests/canvas-createPattern-video-invalid.js:5
> +shouldThrow("ctx.createPattern(undefined, undefined)", "'TypeError: Type error'");

This doesn't look right. Are you sure the second argument to shouldThrow should eval() to a string?

[Myles C. Maxfield]

Comment on attachment 280205 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1753
> +        ec = TypeError;

Looks like this is causing tests to fail. Not sure if we should update the test or revert this change. We should look in the spec.

[Myles C. Maxfield]

Comment on attachment 280205 [details]
Patch

r- until you make the bots green

[George Ruan]

Created attachment 280423 [details]
Patch

[Chris Dumez]

(In reply to comment #19)
> Comment on attachment 280205 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280205&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1753
> > +        ec = TypeError;
> 
> Looks like this is causing tests to fail. Not sure if we should update the
> test or revert this change. We should look in the spec.

We should throw a TypeError, I have a patch to fix this at:
https://bugs.webkit.org/show_bug.cgi?id=158322

[Chris Dumez]

Comment on attachment 280423 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Support createPattern(HTMLVideoElement...

Why the ...?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1825
> +    if (NativeImagePtr nativeImage = videoElement.nativeImageForCurrentTime()) {

Could use auto

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1826
> +        if (RefPtr<Image> image = BitmapImage::create(WTFMove(nativeImage)))

Could use auto. Pretty sure BitmapImage::create() returns a Ref<> not a RefPtr<>. Therefore, this null check is unnecessary as well.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1831
> +    std::unique_ptr<ImageBuffer> imageBuffer = ImageBuffer::create(size(videoElement), drawingContext() ? drawingContext()->renderingMode() : Accelerated);

Could use auto.

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:8
> +    description("Test the handling of invalid arguments in canvas createPattern() with video.")

Not really related to video so this title and file name are a bit misleading.

> LayoutTests/fast/canvas/canvas-createPattern-video-invalid.html:10
> +    var ctx = document.createElement('canvas').getContext('2d');

ctx -> context
We prefer not to use abbreviations in WebKit.

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:36
> +    shouldBe("document.createElement('canvas').getContext('2d').createPattern(video, 'repeat')", "null");

shouldBeNull()

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:53
> +        var ctx = canvas.getContext("2d");

ctx -> context

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Old Doctype.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:11
> +    var canvas, ctx;

Ctx -> context

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:40
> +        else if (modified) {

Since we are in a else, the if(modified) is redundant.
Also, else should be on the same line as the closing curly bracket.

If (!modified) {
   ...
} else {
   ...
}

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous-expected.txt:1
> +Untainted canvas:

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:9
> +function log(msg)

Would be nice to write this using js-test as well.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:55
> +    log("\n");

Log("") would probably suffice unless you need 2 blank lines for some reason.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials-expected.txt:1
> +Untainted canvas:

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:9
> +function log(msg)

Would be nice to write this using js-test as well.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:55
> +    log("\n");

No need for the \n I believe.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin-expected.txt:1
> +CONSOLE MESSAGE: line 18: Unable to get image data from canvas because the canvas has been tainted by cross-origin data.

Missing a description.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:2
> +<script src="../../media-resources/media-file.js"></script>

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:9
> +function log(msg)

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:9
> +function log(msg)

Same comments as for the other tests.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:75
> +video.src = "/resources/redirect.php?url=http://localhost:8000/security/resources/test.mp4";

There are a lot of mp4 files in the layout tests already. Is there any specific reason we need a new one?

[George Ruan]

Created attachment 280461 [details]
Patch

[Dean Jackson]

Comment on attachment 280461 [details]
Patch

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

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:21
> +        // Control tests

Nit: WebKit coding style requires a full stop at the end of this comment.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:45
> +    function testVideo()
> +    {
> +        // Control tests
> +        log("Testing data retrieval on an untainted canvas:");
> +        canvas = document.createElement("canvas");
> +        canvas.width = 100;
> +        canvas.height = 100;
> +
> +        shouldNotThrow("canvas.getContext('2d').getImageData(0, 0, 100, 100)");
> +        shouldNotThrow("canvas.toDataURL()");
> +
> +        // Test CORS
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> +        canvas = document.createElement("canvas");
> +        canvas.width = 100;
> +        canvas.height = 100;
> +        context = canvas.getContext("2d");
> +        context.fillStyle = context.createPattern(video, "repeat");
> +        context.fillRect(0, 0, 100, 100);
> +
> +        shouldNotThrow("context.getImageData(0, 0, 100, 100)");
> +        shouldNotThrow("canvas.toDataURL()");
> +
> +        finishJSTest();
> +    }

It seems this function is the same in a few tests. Maybe you should split it out to a separate file?

> LayoutTests/media/video-canvas-createPattern.html:41
> +                    logResult(false, "Expected (" + r + ", " + g + ", " + b + ") at (" + x + ", " + y + ") but saw (" + buffer[0] + ", " + buffer[1] + ", " + buffer[2] + ")");

This line, and similar lines, can be made more readable by using ES6 string templates (or whatever they are called). `Expected (${r}, ${g}, ${b}) at (${x}, ${y}....`

[Eric Carlson]

Comment on attachment 280461 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1812
> +    if (videoElement.readyState() == HTMLMediaElement::HAVE_NOTHING || videoElement.readyState() == HTMLMediaElement::HAVE_METADATA)

This can be "if (videoElement.readyState() < HAVE_CURRENT_DATA)"

> LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:38
> +    video.src = "../../media/content/test.mp4";

This should use "findMediaFile" because not all ports support MPEG-4.

> LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:23
> +    video.src = "../../media/content/test.mp4";

This should use "findMediaFile" because not all ports support MPEG-4.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:33
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:33
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:17
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:32
> +        log("");
> +        

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:16
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:32
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a pattern generated by a localhost video resource:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:48
> +    video.src = "http://localhost:8080/resources/test.mp4";

This should use findMediaFile() as not all ports support MPEG-4.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:16
> +    function log(msg)
> +    {
> +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> +    }

Is there any reason to not use "debug" from media-file.js?

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:32
> +        log("");
> +        
> +        log("Testing data retrieval on a canvas tainted by a pattern generated by a redirected video resource:");

You can get rid of the first log by adding a "\n" (or "<br />" if you use debug) to the beginning of the string logged by the second.

> LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:48
> +    video.src = "/resources/redirect.php?url=http://localhost:8000/resources/test.mp4";

This should use findMediaFile(), e.g. something like:

    video.src = `/resources/redirect.php?url=http://localhost:8000/${findMediaFile('video', 'resources/test')}`;

> LayoutTests/http/tests/security/resources/video-cross-origin-allow-credentials.php:10
> +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> +header("Access-Control-Allow-Credentials: true");
> +
> +if ($_SERVER['REQUEST_METHOD'] == "OPTIONS") {
> +    header("Access-Control-Allow-Methods: GET");
> +    header("Access-Control-Allow-Headers: origin, accept-encoding, referer, range");
> +    exit;
> +}

It would be nice to add CORS support to serve-video.php with another url parameter so it can be reused in other tests. For example, look at the way that "content-length" or "Content-Type" are handled. 

If you would rather land this as-is and fix it in a follow-up, please file a bug for the change.

[George Ruan]

Created attachment 280541 [details]
Patch

[George Ruan]

(In reply to comment #26)
> Comment on attachment 280461 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280461&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1812
> > +    if (videoElement.readyState() == HTMLMediaElement::HAVE_NOTHING || videoElement.readyState() == HTMLMediaElement::HAVE_METADATA)
> 
> This can be "if (videoElement.readyState() < HAVE_CURRENT_DATA)"
> 
> > LayoutTests/fast/canvas/canvas-createPattern-video-loading.html:38
> > +    video.src = "../../media/content/test.mp4";
> 
> This should use "findMediaFile" because not all ports support MPEG-4.
> 
> > LayoutTests/fast/canvas/canvas-createPattern-video-modify.html:23
> > +    video.src = "../../media/content/test.mp4";
> 
> This should use "findMediaFile" because not all ports support MPEG-4.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-anonymous.html:33
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-allowed-with-credentials.html:33
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a remote video pattern:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:17
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html:32
> > +        log("");
> > +        
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:16
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:32
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a pattern generated by a localhost video resource:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-localhost.html:48
> > +    video.src = "http://localhost:8080/resources/test.mp4";
> 
> This should use findMediaFile() as not all ports support MPEG-4.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:16
> > +    function log(msg)
> > +    {
> > +        document.getElementById('console').appendChild(document.createTextNode(msg + "\n"));
> > +    }
> 
> Is there any reason to not use "debug" from media-file.js?
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:32
> > +        log("");
> > +        
> > +        log("Testing data retrieval on a canvas tainted by a pattern generated by a redirected video resource:");
> 
> You can get rid of the first log by adding a "\n" (or "<br />" if you use
> debug) to the beginning of the string logged by the second.
> 
> > LayoutTests/http/tests/security/canvas-remote-read-remote-video-redirect.html:48
> > +    video.src = "/resources/redirect.php?url=http://localhost:8000/resources/test.mp4";
> 
> This should use findMediaFile(), e.g. something like:
> 
>     video.src =
> `/resources/redirect.php?url=http://localhost:8000/${findMediaFile('video',
> 'resources/test')}`;
> 
> > LayoutTests/http/tests/security/resources/video-cross-origin-allow-credentials.php:10
> > +header("Access-Control-Allow-Origin: http://127.0.0.1:8000");
> > +header("Access-Control-Allow-Credentials: true");
> > +
> > +if ($_SERVER['REQUEST_METHOD'] == "OPTIONS") {
> > +    header("Access-Control-Allow-Methods: GET");
> > +    header("Access-Control-Allow-Headers: origin, accept-encoding, referer, range");
> > +    exit;
> > +}
> 
> It would be nice to add CORS support to serve-video.php with another url
> parameter so it can be reused in other tests. For example, look at the way
> that "content-length" or "Content-Type" are handled. 
> 
> If you would rather land this as-is and fix it in a follow-up, please file a
> bug for the change.

New bug regarding CORS support for serve-video.php: 26639207.

[Darin Adler]

Comment on attachment 280541 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1805
> +    if (videoElement.readyState() < HTMLMediaElement::HAVE_CURRENT_DATA)
> +        return nullptr;

No exception in this case?

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:180
> +    [Conditional=VIDEO, RaisesException] CanvasPattern createPattern(HTMLVideoElement video, [TreatNullAs=EmptyString] DOMString repetitionType);

Given the fact that this returns null when the video doesn’t have data, the return type should be nullable: "CanvasPattern?" instead of "CanvasPattern".

Or maybe we should be raising an exception in that case. In which case the return type would remain, but we would have to set ec above.

[Dean Jackson]

Comment on attachment 280541 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1805
>> +        return nullptr;
> 
> No exception in this case?

No. The specification says:

  If the image argument is an HTMLOrSVGImageElement object that is not fully decodable, or if the image argument is an HTMLVideoElement object whose readyState attribute is either HAVE_NOTHING or HAVE_METADATA, then return bad and abort these steps.

Since HAVE_NOTHING and HAVE_METADATA are the only two values less than HAVE_CURRENT_DATA this is correct.

>> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:180
>> +    [Conditional=VIDEO, RaisesException] CanvasPattern createPattern(HTMLVideoElement video, [TreatNullAs=EmptyString] DOMString repetitionType);
> 
> Given the fact that this returns null when the video doesn’t have data, the return type should be nullable: "CanvasPattern?" instead of "CanvasPattern".
> 
> Or maybe we should be raising an exception in that case. In which case the return type would remain, but we would have to set ec above.

Indeed! Great catch.

In fact, all the createPattern calls are defined to return  CanvasPattern?

So George, please do this for the createPattern you are adding, and we'll fix the rest in a followup bug.

[George Ruan]

Created attachment 280621 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 280621 [details]
Patch

Clearing flags on attachment: 280621

Committed r201724: <http://trac.webkit.org/changeset/201724>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.