102586 2012-11-17 05:16:53 -0800 [GTK] media/video-src-blob.html 2013-10-08 01:06:27 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk, LayoutTestFailure P2 Normal --- 1 zan webkit-unassigned cgarcia commit-queue gustavo menard mrobinson pnormand oldest_to_newest 769938 0 zan 2012-11-17 05:16:53 -0800 The test is failing since it has been introduced in r134802. http://trac.webkit.org/changeset/134802 Here's the diff: --- /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/media/video-src-blob-expected.txt +++ /home/slave/webkitgtk/gtk-linux-64-release/build/layout-test-results/media/video-src-blob-actual.txt @@ -1,5 +1,5 @@ This tests the ability of the <video> element to load blob URLs. In the browser, select a video file: EVENT(change) -EVENT(loadedmetadata) +EVENT(error) TEST(false) FAIL END OF TEST 909370 1 cgarcia 2013-07-17 10:24:46 -0700 Now that we use the SubresourceLoder to load videos this should be easy to fix. 909371 2 206897 cgarcia 2013-07-17 10:25:17 -0700 Created attachment 206897 Patch 909381 3 206897 mrobinson 2013-07-17 10:36:46 -0700 Comment on attachment 206897 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206897&action=review Looks good to me. Great work! Just a couple questions... > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:465 > + DataBufferingPolicy bufferingPolicy = url.protocolIs("blob") ? BufferData : DoNotBufferData; > + CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, DoNotAllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, DoSecurityCheck, UseDefaultOriginRestrictionsForType)); > priv->resource = loader->requestRawResource(cacheRequest); Why is it useful to buffer data that is already in memory on the client side? > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:605 > + if (!url.isValid() || (!url.protocolIsInHTTPFamily() && !url.protocolIs("blob"))) { I think this could be a helper. Something like urlIsValidAndHasSupportedProtocol or just urlHasSupportedProtocol. 909385 4 cgarcia 2013-07-17 10:46:06 -0700 (In reply to comment #3) > (From update of attachment 206897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206897&action=review > > Looks good to me. Great work! Just a couple questions... Thanks! > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:465 > > + DataBufferingPolicy bufferingPolicy = url.protocolIs("blob") ? BufferData : DoNotBufferData; > > + CachedResourceRequest cacheRequest(request, ResourceLoaderOptions(SendCallbacks, DoNotSniffContent, bufferingPolicy, DoNotAllowStoredCredentials, DoNotAskClientForCrossOriginCredentials, DoSecurityCheck, UseDefaultOriginRestrictionsForType)); > > priv->resource = loader->requestRawResource(cacheRequest); > > Why is it useful to buffer data that is already in memory on the client side? Because when buffered it can be cached, I think. I followed what apple does, TBH. > > Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:605 > > + if (!url.isValid() || (!url.protocolIsInHTTPFamily() && !url.protocolIs("blob"))) { > > I think this could be a helper. Something like urlIsValidAndHasSupportedProtocol or just urlHasSupportedProtocol. I did it indeed :-) but in the end it looked too much code for a simple check and moved back to the current patch, I'll add the helper function since it makes the code easier to read. 915438 5 pnormand 2013-08-08 07:55:23 -0700 LGTM but yeah that helper function would be good to have indeed. 926798 6 206897 mrobinson 2013-09-09 11:04:43 -0700 Comment on attachment 206897 Patch Looks good. Consider the helper when landing. :) 937443 7 cgarcia 2013-10-08 01:06:27 -0700 Committed r157091: <http://trac.webkit.org/changeset/157091> 206897 2013-07-17 10:25:17 -0700 2013-09-09 11:04:42 -0700 Patch wcore-gst-blob.diff text/plain 5039 cgarcia ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiNGVkNGQwLi4yMzZjMDA0IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTMtMDct MTcgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCisgICAgICAg IFtHVEtdIG1lZGlhL3ZpZGVvLXNyYy1ibG9iLmh0bWwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMjU4NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgICogcGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3RhdGlvbnM6 IFVuc2tpcAorICAgICAgICBtZWRpYS92aWRlby1zcmMtYmxvYi5odG1sLgorCiAyMDEzLTA3LTE3 ICBGcsOpZMOpcmljIFdhbmcgIDxmcmVkLndhbmdAZnJlZS5mcj4KIAogICAgICAgICBSZXBsYWNl IE1hdGhNTCBwaXhlbCB0ZXN0cyBieSByZWZ0ZXN0cy4KZGlmZiAtLWdpdCBhL0xheW91dFRlc3Rz L3BsYXRmb3JtL2d0ay9UZXN0RXhwZWN0YXRpb25zIGIvTGF5b3V0VGVzdHMvcGxhdGZvcm0vZ3Rr L1Rlc3RFeHBlY3RhdGlvbnMKaW5kZXggODZkODhiZS4uYjJiYmQ1MSAxMDA2NDQKLS0tIGEvTGF5 b3V0VGVzdHMvcGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3RhdGlvbnMKKysrIGIvTGF5b3V0VGVzdHMv cGxhdGZvcm0vZ3RrL1Rlc3RFeHBlY3RhdGlvbnMKQEAgLTEyNDYsOCArMTI0Niw2IEBAIHdlYmtp dC5vcmcvYi8xMDE1MzkgZWRpdGluZy9leGVjQ29tbWFuZC9zd2l0Y2gtbGlzdC10eXBlLXdpdGgt b3JwaGFuZWQtbGkuaHRtbCBbCiB3ZWJraXQub3JnL2IvMTAyMzQ5IGZhc3QvZG9tL3NoYWRvdy9w c2V1ZG9jbGFzcy11cGRhdGUtZW5hYmxlZC1hbmNob3IuaHRtbCBbIEltYWdlT25seUZhaWx1cmUg XQogd2Via2l0Lm9yZy9iLzEwMjM0OSBmYXN0L2RvbS9zaGFkb3cvcHNldWRvY2xhc3MtdXBkYXRl LWVuYWJsZWQtYXJlYS5odG1sIFsgSW1hZ2VPbmx5RmFpbHVyZSBdCiAKLXdlYmtpdC5vcmcvYi8x MDI1ODYgbWVkaWEvdmlkZW8tc3JjLWJsb2IuaHRtbCBbIEZhaWx1cmUgXQotCiAjIEZpeCBmb3Ig aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk3MTkyIGludHJvZHVjZXMg dGhlc2UgcmVncmVzc2lvbnMKIHdlYmtpdC5vcmcvYi8xMDI3NzYgd2Via2l0Lm9yZy9iLzEwMjk5 MyBtZWRpYS9tZWRpYS1kb2N1bWVudC1hdWRpby1yZXBhaW50Lmh0bWwgWyBGYWlsdXJlIFRpbWVv dXQgXQogd2Via2l0Lm9yZy9iLzEwMjc3NiBtZWRpYS90cmFjay90cmFjay1jdWUtcmVuZGVyaW5n LWhvcml6b250YWwuaHRtbCBbIEZhaWx1cmUgXQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv Q2hhbmdlTG9nIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IDczMzdjMDAuLjdhNzE1 ZDYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsNSArMSwyNCBAQAogMjAxMy0wNy0xNyAgQ2FybG9zIEdhcmNp YSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKKworICAgICAgICBbR1RLXSBtZWRpYS92 aWRlby1zcmMtYmxvYi5odG1sCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3df YnVnLmNnaT9pZD0xMDI1ODYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KworICAgICAgICBBZGQgc3VwcG9ydCBmb3IgbG9hZGluZyB2aWRlb3MgdXNpbmcgYmxvYiBVUkxz LgorCisgICAgICAgIEZpeGVzIG1lZGlhL3ZpZGVvLXNyYy1ibG9iLmh0bWwuCisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvV2ViS2l0V2ViU291cmNlR1N0cmVhbWVyLmNw cDoKKyAgICAgICAgKHdlYktpdFdlYlNyY1N0YXJ0KTogVXNlIEJ1ZmZlckRhdGEgYnVmZmVyaW5n IHBvbGljeSBmb3IgYmxvYgorICAgICAgICBVUkxzLgorICAgICAgICAod2ViS2l0V2ViU3JjR2V0 UHJvdG9jb2xzKTogQWRkIGJsb2IgdG8gdGhlIGxpc3Qgb2Ygc3VwcG9ydGVkCisgICAgICAgIHBy b3RvY29scy4KKyAgICAgICAgKHdlYktpdFdlYlNyY1NldFVyaSk6IENvbnNpZGVyIGJsb2IgVVJM cyBhcyB2YWxpZC4KKworMjAxMy0wNy0xNyAgQ2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lh QGlnYWxpYS5jb20+CisKICAgICAgICAgW0dTdHJlYW1lcl0gd2Via2l0d2Vic3JjOiB1c2UgU3Vi UmVzb3VyY2VMb2FkZXIKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTczNzQzCiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBo aWNzL2dzdHJlYW1lci9XZWJLaXRXZWJTb3VyY2VHU3RyZWFtZXIuY3BwIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL1dlYktpdFdlYlNvdXJjZUdTdHJlYW1lci5j cHAKaW5kZXggNjk2NGQ2MS4uNmE1ZjkzMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL1dlYktpdFdlYlNvdXJjZUdTdHJlYW1lci5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL1dlYktpdFdlYlNv dXJjZUdTdHJlYW1lci5jcHAKQEAgLTQ2MCw3ICs0NjAsOCBAQCBzdGF0aWMgYm9vbCB3ZWJLaXRX ZWJTcmNTdGFydChXZWJLaXRXZWJTcmMqIHNyYykKICAgICByZXF1ZXN0LnNldEhUVFBIZWFkZXJG aWVsZCgidHJhbnNmZXJNb2RlLmRsbmEiLCAiU3RyZWFtaW5nIik7CiAKICAgICBpZiAoQ2FjaGVk UmVzb3VyY2VMb2FkZXIqIGxvYWRlciA9IHByaXYtPnBsYXllci0+Y2FjaGVkUmVzb3VyY2VMb2Fk ZXIoKSkgewotICAgICAgICBDYWNoZWRSZXNvdXJjZVJlcXVlc3QgY2FjaGVSZXF1ZXN0KHJlcXVl c3QsIFJlc291cmNlTG9hZGVyT3B0aW9ucyhTZW5kQ2FsbGJhY2tzLCBEb05vdFNuaWZmQ29udGVu dCwgRG9Ob3RCdWZmZXJEYXRhLCBEb05vdEFsbG93U3RvcmVkQ3JlZGVudGlhbHMsIERvTm90QXNr Q2xpZW50Rm9yQ3Jvc3NPcmlnaW5DcmVkZW50aWFscywgRG9TZWN1cml0eUNoZWNrLCBVc2VEZWZh dWx0T3JpZ2luUmVzdHJpY3Rpb25zRm9yVHlwZSkpOworICAgICAgICBEYXRhQnVmZmVyaW5nUG9s aWN5IGJ1ZmZlcmluZ1BvbGljeSA9IHVybC5wcm90b2NvbElzKCJibG9iIikgPyBCdWZmZXJEYXRh IDogRG9Ob3RCdWZmZXJEYXRhOworICAgICAgICBDYWNoZWRSZXNvdXJjZVJlcXVlc3QgY2FjaGVS ZXF1ZXN0KHJlcXVlc3QsIFJlc291cmNlTG9hZGVyT3B0aW9ucyhTZW5kQ2FsbGJhY2tzLCBEb05v dFNuaWZmQ29udGVudCwgYnVmZmVyaW5nUG9saWN5LCBEb05vdEFsbG93U3RvcmVkQ3JlZGVudGlh bHMsIERvTm90QXNrQ2xpZW50Rm9yQ3Jvc3NPcmlnaW5DcmVkZW50aWFscywgRG9TZWN1cml0eUNo ZWNrLCBVc2VEZWZhdWx0T3JpZ2luUmVzdHJpY3Rpb25zRm9yVHlwZSkpOwogICAgICAgICBwcml2 LT5yZXNvdXJjZSA9IGxvYWRlci0+cmVxdWVzdFJhd1Jlc291cmNlKGNhY2hlUmVxdWVzdCk7CiAg ICAgICAgIGlmIChwcml2LT5yZXNvdXJjZSkgewogICAgICAgICAgICAgcHJpdi0+Y2xpZW50ID0g bmV3IFN0cmVhbWluZ0NsaWVudChzcmMpOwpAQCAtNTc0LDcgKzU3NSw3IEBAIHN0YXRpYyBHc3RV UklUeXBlIHdlYktpdFdlYlNyY1VyaUdldFR5cGUoR1R5cGUpCiAKIGNvbnN0IGdjaGFyKiBjb25z dCogd2ViS2l0V2ViU3JjR2V0UHJvdG9jb2xzKEdUeXBlKQogewotICAgIHN0YXRpYyBjb25zdCBj aGFyKiBwcm90b2NvbHNbXSA9IHsiaHR0cCIsICJodHRwcyIsIDAgfTsKKyAgICBzdGF0aWMgY29u c3QgY2hhciogcHJvdG9jb2xzW10gPSB7Imh0dHAiLCAiaHR0cHMiLCAiYmxvYiIsIDAgfTsKICAg ICByZXR1cm4gcHJvdG9jb2xzOwogfQogCkBAIC02MDEsNyArNjAyLDcgQEAgc3RhdGljIGdib29s ZWFuIHdlYktpdFdlYlNyY1NldFVyaShHc3RVUklIYW5kbGVyKiBoYW5kbGVyLCBjb25zdCBnY2hh ciogdXJpLCBHRXIKIAogICAgIEtVUkwgdXJsKEtVUkwoKSwgdXJpKTsKIAotICAgIGlmICghdXJs LmlzVmFsaWQoKSB8fCAhdXJsLnByb3RvY29sSXNJbkhUVFBGYW1pbHkoKSkgeworICAgIGlmICgh dXJsLmlzVmFsaWQoKSB8fCAoIXVybC5wcm90b2NvbElzSW5IVFRQRmFtaWx5KCkgJiYgIXVybC5w cm90b2NvbElzKCJibG9iIikpKSB7CiAgICAgICAgIGdfc2V0X2Vycm9yKGVycm9yLCBHU1RfVVJJ X0VSUk9SLCBHU1RfVVJJX0VSUk9SX0JBRF9VUkksICJJbnZhbGlkIFVSSSAnJXMnIiwgdXJpKTsK ICAgICAgICAgcmV0dXJuIEZBTFNFOwogICAgIH0KQEAgLTYxOCw3ICs2MTksNyBAQCBzdGF0aWMg R3N0VVJJVHlwZSB3ZWJLaXRXZWJTcmNVcmlHZXRUeXBlKHZvaWQpCiAKIHN0YXRpYyBnY2hhcioq IHdlYktpdFdlYlNyY0dldFByb3RvY29scyh2b2lkKQogewotICAgIHN0YXRpYyBnY2hhciogcHJv dG9jb2xzW10gPSB7KGdjaGFyKikgImh0dHAiLCAoZ2NoYXIqKSAiaHR0cHMiLCAwIH07CisgICAg c3RhdGljIGdjaGFyKiBwcm90b2NvbHNbXSA9IHsoZ2NoYXIqKSAiaHR0cCIsIChnY2hhciopICJo dHRwcyIsIChnY2hhciopICJibG9iIiwgMCB9OwogICAgIHJldHVybiBwcm90b2NvbHM7CiB9CiAK QEAgLTY0NSw3ICs2NDYsNyBAQCBzdGF0aWMgZ2Jvb2xlYW4gd2ViS2l0V2ViU3JjU2V0VXJpKEdz dFVSSUhhbmRsZXIqIGhhbmRsZXIsIGNvbnN0IGdjaGFyKiB1cmkpCiAKICAgICBLVVJMIHVybChL VVJMKCksIHVyaSk7CiAKLSAgICBpZiAoIXVybC5pc1ZhbGlkKCkgfHwgIXVybC5wcm90b2NvbElz SW5IVFRQRmFtaWx5KCkpIHsKKyAgICBpZiAoIXVybC5pc1ZhbGlkKCkgfHwgKCF1cmwucHJvdG9j b2xJc0luSFRUUEZhbWlseSgpICYmICF1cmwucHJvdG9jb2xJcygiYmxvYiIpKSkgewogICAgICAg ICBHU1RfRVJST1JfT0JKRUNUKHNyYywgIkludmFsaWQgVVJJICclcyciLCB1cmkpOwogICAgICAg ICByZXR1cm4gRkFMU0U7CiAgICAgfQo=