105353 2012-12-18 14:52:36 -0800 Minor improvements to HTMLMediaElement 2013-01-07 15:59:58 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 105386 1 joepeck joepeck eric.carlson feature-media-reviews fpizlo joepeck ojan.autocc oliver webkit.review.bot oldest_to_newest 794083 0 joepeck 2012-12-18 14:52:36 -0800 While reading through HTMLMediaElement I noticed some cases where there could be string improvements: <http://trac.webkit.org/wiki/EfficientStrings> Patch to follow. 794084 1 joepeck 2012-12-18 14:53:33 -0800 There was also a case (HTMLMediaElement::setLoop) which was doing extra work. 794101 2 180039 joepeck 2012-12-18 15:10:46 -0800 Created attachment 180039 [PATCH] Proposed Improvements 794117 3 joepeck 2012-12-18 15:37:52 -0800 This would be the path from setBooleanAttribute -> parseAttribute, which handles loopAttr: * thread #1: tid = 0x1c03, 0x000000011216bc42 WebCore`WebCore::HTMLMediaElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 34 at HTMLMediaElement.cpp:372, stop reason = breakpoint 2.1 frame #0: 0x000000011216bc42 WebCore`WebCore::HTMLMediaElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 34 at HTMLMediaElement.cpp:372 frame #1: 0x00000001121f06fb WebCore`WebCore::HTMLVideoElement::parseAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 379 at HTMLVideoElement.cpp:124 frame #2: 0x0000000111edb1e2 WebCore`WebCore::Element::attributeChanged(this=0x00007fa62523e500, name=0x00000001143cf820, newValue=0x0000000110fcb358) + 130 at Element.cpp:776 frame #3: 0x0000000112fea845 WebCore`WebCore::StyledElement::attributeChanged(this=0x00007fa62523e500, name=0x00000001143cf820, newValue=0x0000000110fcb358) + 181 at StyledElement.cpp:168 frame #4: 0x0000000111edfad9 WebCore`WebCore::Element::didAddAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 73 at Element.cpp:2497 frame #5: 0x0000000111edfa86 WebCore`WebCore::Element::addAttributeInternal(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) + 150 at Element.cpp:1706 frame #6: 0x0000000111ee481f WebCore`WebCore::Element::setAttributeInternal(this=0x00007fa62523e500, index=18446744073709551615, name=0x00000001143cf820, newValue=0x0000000110fcb358, inSynchronizationOfLazyAttribute=NotInSynchronizationOfLazyAttribute) + 127 at Element.cpp:731 frame #7: 0x0000000111ed961a WebCore`WebCore::Element::setAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=0x0000000110fcb358) + 74 at Element.cpp:714 frame #8: 0x0000000111ed95ad WebCore`WebCore::Element::setBooleanAttribute(this=0x00007fa62523e500, name=0x00000001143cf820, value=true) + 61 at Element.cpp:288 794211 4 180039 eric.carlson 2012-12-18 17:30:08 -0800 Comment on attachment 180039 [PATCH] Proposed Improvements Nice, thank you! 794240 5 180039 webkit.review.bot 2012-12-18 18:15:10 -0800 Comment on attachment 180039 [PATCH] Proposed Improvements Clearing flags on attachment: 180039 Committed r138097: <http://trac.webkit.org/changeset/138097> 794241 6 webkit.review.bot 2012-12-18 18:15:13 -0800 All reviewed patches have been landed. Closing bug. 794276 7 fpizlo 2012-12-18 19:28:29 -0800 I think you guys just set my machine on fire: [pizlo@bigmac OpenSource] DYLD_FRAMEWORK_PATH=WebKitBuild/Debug/ WebKitBuild/Debug/DumpRenderTree LayoutTests/media/media-can-play-webm.html ASSERTION FAILED: m_length /Volumes/Data/pizlo/quartary/OpenSource/Source/WTF/wtf/text/StringImpl.h(252) : WTF::StringImpl::StringImpl(const char *, unsigned int, WTF::StringImpl::ConstructFromLiteralTag) 1 0x10ebcba44 WTF::StringImpl::StringImpl(char const*, unsigned int, WTF::StringImpl::ConstructFromLiteralTag) 2 0x10ebc9be9 WTF::StringImpl::StringImpl(char const*, unsigned int, WTF::StringImpl::ConstructFromLiteralTag) 3 0x10ebc1a16 WTF::StringImpl::createFromLiteral(char const*) 4 0x10ebd4840 WTF::String::String(WTF::ASCIILiteral) 5 0x10ebd47fd WTF::String::String(WTF::ASCIILiteral) 6 0x11060dd86 WebCore::HTMLMediaElement::canPlayType(WTF::String const&, WTF::String const&, WebCore::KURL const&) const 7 0x110a98029 WebCore::jsHTMLMediaElementPrototypeFunctionCanPlayType(JSC::ExecState*) 8 0x460869801265 9 0x10e8e8184 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*) 10 0x10e8e08a9 JSC::Interpreter::execute(JSC::EvalExecutable*, JSC::ExecState*, JSC::JSValue, JSC::JSScope*) 11 0x10e8dfff7 JSC::eval(JSC::ExecState*) 12 0x10eae7d75 llint_slow_path_call_eval 13 0x10eaf00a7 llint_op_call_eval 14 0x10e8e8184 JSC::JITCode::execute(JSC::JSStack*, JSC::ExecState*, JSC::JSGlobalData*) 15 0x10e8e53ff JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 16 0x10e763a12 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 17 0x11089ad92 WebCore::JSMainThreadExecState::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) 18 0x1109dc796 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) 19 0x1103cf363 WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul>&) 20 0x1103cf135 WebCore::EventTarget::fireEventListeners(WebCore::Event*) 21 0x110319bd0 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) 22 0x110320be8 WebCore::DOMWindow::dispatchLoadEvent() 23 0x11017a0bf WebCore::Document::dispatchWindowLoadEvent() 24 0x110177bf0 WebCore::Document::implicitClose() 25 0x11049572b WebCore::FrameLoader::checkCallImplicitClose() 26 0x1104953f3 WebCore::FrameLoader::checkCompleted() 27 0x110495595 WebCore::FrameLoader::loadDone() 28 0x10fec68e2 WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*) 29 0x1114eb28f WebCore::SubresourceLoader::releaseResources() 30 0x1112be579 WebCore::ResourceLoader::didFinishLoading(double) 31 0x1114eae75 WebCore::SubresourceLoader::didFinishLoading(double) 794335 8 webkit.review.bot 2012-12-18 21:37:39 -0800 Re-opened since this is blocked by bug 105386 794802 9 joepeck 2012-12-19 10:47:33 -0800 Ahh, from the backtrace above it sounds like this was ASCIILiteral("") in HTMLMediaElement::canPlayType. StringImpl ends up doing strlen, gets a length of 0, and uses a constructor that asserts a non-0 length. 794857 10 oliver 2012-12-19 12:20:47 -0800 Fixed the zero length ASCIILiteral bug in r138187 You should be able to roll back in now. 801771 11 joepeck 2013-01-07 15:59:58 -0800 Rolling back in after Oliver's change. Landed r139006: <http://trac.webkit.org/changeset/139006> 180039 2012-12-18 15:10:46 -0800 2012-12-18 18:15:10 -0800 [PATCH] Proposed Improvements proposed-improvements-1.patch text/plain 5021 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBmZDVmOGJhLi5hYzFiYmQyIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjQg QEAKIDIwMTItMTItMTggIEpvc2VwaCBQZWNvcmFybyAgPHBlY29yYXJvQGFwcGxlLmNvbT4KIAor ICAgICAgICBNaW5vciBpbXByb3ZlbWVudHMgdG8gSFRNTE1lZGlhRWxlbWVudAorICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA1MzUzCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBodG1sL0hUTUxNZWRpYUVs ZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVudDo6Y2FuUGxheVR5 cGUpOgorICAgICAgICAoV2ViQ29yZTo6Y3JlYXRlRmlsZVVSTEZvckFwcGxpY2F0aW9uQ2FjaGVS ZXNvdXJjZSk6CisgICAgICAgIChXZWJDb3JlOjpzdHJpbmdGb3JOZXR3b3JrU3RhdGUpOgorICAg ICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVudDo6cHJlbG9hZCk6CisgICAgICAgIChXZWJD b3JlOjpIVE1MTWVkaWFFbGVtZW50OjpnZXRQbHVnaW5Qcm94eVBhcmFtcyk6CisgICAgICAgIFVz ZSBBU0NJSUxpdGVyYWwgZm9yIGxpdGVyYWwgc3RyaW5ncyBiZWNvbWluZyBXVEZTdHJpbmdzLgor CisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50OjpzZXRMb29wKToKKyAgICAgICAg QnkgZ29pbmcgdGhyb3VnaCBzZXRBdHRyaWJ1dGUsIHBhcnNlQXR0cmlidXRlIHdvdWxkIGFscmVh ZHkgdGFrZSBjYXJlIG9mCisgICAgICAgIHVwZGF0aW5nIHRoZSBkaXNhYmxlIHNsZWVwLCBzbyB0 aGUgY2FsbCBoZXJlIGlzIG5vdCBuZWVkZWQuCisKKzIwMTItMTItMTggIEpvc2VwaCBQZWNvcmFy byAgPHBlY29yYXJvQGFwcGxlLmNvbT4KKwogICAgICAgICBTdHJpbmcgaW1wcm92ZW1lbnRzIGlu IGdlb2xvY2F0aW9uIGFuZCBub3RpZmljYXRpb25zIG1vZHVsZXMKICAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwNTM1MgogCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwIGIvU291cmNlL1dlYkNvcmUvaHRt bC9IVE1MTWVkaWFFbGVtZW50LmNwcAppbmRleCBjMDQwNWMyLi5iN2YwZjQ1IDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9X ZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5jcHAKQEAgLTY2NSwxMyArNjY1LDEzIEBAIFN0 cmluZyBIVE1MTWVkaWFFbGVtZW50OjpjYW5QbGF5VHlwZShjb25zdCBTdHJpbmcmIG1pbWVUeXBl LCBjb25zdCBTdHJpbmcmIGtleVN5CiAgICAgc3dpdGNoIChzdXBwb3J0KQogICAgIHsKICAgICAg ICAgY2FzZSBNZWRpYVBsYXllcjo6SXNOb3RTdXBwb3J0ZWQ6Ci0gICAgICAgICAgICBjYW5QbGF5 ID0gIiI7CisgICAgICAgICAgICBjYW5QbGF5ID0gQVNDSUlMaXRlcmFsKCIiKTsKICAgICAgICAg ICAgIGJyZWFrOwogICAgICAgICBjYXNlIE1lZGlhUGxheWVyOjpNYXlCZVN1cHBvcnRlZDoKLSAg ICAgICAgICAgIGNhblBsYXkgPSAibWF5YmUiOworICAgICAgICAgICAgY2FuUGxheSA9IEFTQ0lJ TGl0ZXJhbCgibWF5YmUiKTsKICAgICAgICAgICAgIGJyZWFrOwogICAgICAgICBjYXNlIE1lZGlh UGxheWVyOjpJc1N1cHBvcnRlZDoKLSAgICAgICAgICAgIGNhblBsYXkgPSAicHJvYmFibHkiOwor ICAgICAgICAgICAgY2FuUGxheSA9IEFTQ0lJTGl0ZXJhbCgicHJvYmFibHkiKTsKICAgICAgICAg ICAgIGJyZWFrOwogICAgIH0KICAgICAKQEAgLTkyOSw3ICs5MjksNyBAQCBzdGF0aWMgS1VSTCBj cmVhdGVGaWxlVVJMRm9yQXBwbGljYXRpb25DYWNoZVJlc291cmNlKGNvbnN0IFN0cmluZyYgcGF0 aCkKICNlbHNlCiAgICAgS1VSTCB1cmw7CiAKLSAgICB1cmwuc2V0UHJvdG9jb2woImZpbGUiKTsK KyAgICB1cmwuc2V0UHJvdG9jb2woQVNDSUlMaXRlcmFsKCJmaWxlIikpOwogICAgIHVybC5zZXRQ YXRoKHBhdGgpOwogI2VuZGlmCiAgICAgcmV0dXJuIHVybDsKQEAgLTE1NzEsMTMgKzE1NzEsMTMg QEAgc3RhdGljIHZvaWQgbG9nTWVkaWFMb2FkUmVxdWVzdChQYWdlKiBwYWdlLCBjb25zdCBTdHJp bmcmIG1lZGlhRW5naW5lLCBjb25zdCBTdHIKIHN0YXRpYyBTdHJpbmcgc3RyaW5nRm9yTmV0d29y a1N0YXRlKE1lZGlhUGxheWVyOjpOZXR3b3JrU3RhdGUgc3RhdGUpCiB7CiAgICAgc3dpdGNoIChz dGF0ZSkgewotICAgIGNhc2UgTWVkaWFQbGF5ZXI6OkVtcHR5OiByZXR1cm4gIkVtcHR5IjsKLSAg ICBjYXNlIE1lZGlhUGxheWVyOjpJZGxlOiByZXR1cm4gIklkbGUiOwotICAgIGNhc2UgTWVkaWFQ bGF5ZXI6OkxvYWRpbmc6IHJldHVybiAiTG9hZGluZyI7Ci0gICAgY2FzZSBNZWRpYVBsYXllcjo6 TG9hZGVkOiByZXR1cm4gIkxvYWRlZCI7Ci0gICAgY2FzZSBNZWRpYVBsYXllcjo6Rm9ybWF0RXJy b3I6IHJldHVybiAiRm9ybWF0RXJyb3IiOwotICAgIGNhc2UgTWVkaWFQbGF5ZXI6Ok5ldHdvcmtF cnJvcjogcmV0dXJuICJOZXR3b3JrRXJyb3IiOwotICAgIGNhc2UgTWVkaWFQbGF5ZXI6OkRlY29k ZUVycm9yOiByZXR1cm4gIkRlY29kZUVycm9yIjsKKyAgICBjYXNlIE1lZGlhUGxheWVyOjpFbXB0 eTogcmV0dXJuIEFTQ0lJTGl0ZXJhbCgiRW1wdHkiKTsKKyAgICBjYXNlIE1lZGlhUGxheWVyOjpJ ZGxlOiByZXR1cm4gQVNDSUlMaXRlcmFsKCJJZGxlIik7CisgICAgY2FzZSBNZWRpYVBsYXllcjo6 TG9hZGluZzogcmV0dXJuIEFTQ0lJTGl0ZXJhbCgiTG9hZGluZyIpOworICAgIGNhc2UgTWVkaWFQ bGF5ZXI6OkxvYWRlZDogcmV0dXJuIEFTQ0lJTGl0ZXJhbCgiTG9hZGVkIik7CisgICAgY2FzZSBN ZWRpYVBsYXllcjo6Rm9ybWF0RXJyb3I6IHJldHVybiBBU0NJSUxpdGVyYWwoIkZvcm1hdEVycm9y Iik7CisgICAgY2FzZSBNZWRpYVBsYXllcjo6TmV0d29ya0Vycm9yOiByZXR1cm4gQVNDSUlMaXRl cmFsKCJOZXR3b3JrRXJyb3IiKTsKKyAgICBjYXNlIE1lZGlhUGxheWVyOjpEZWNvZGVFcnJvcjog cmV0dXJuIEFTQ0lJTGl0ZXJhbCgiRGVjb2RlRXJyb3IiKTsKICAgICBkZWZhdWx0OiByZXR1cm4g ZW1wdHlTdHJpbmcoKTsKICAgICB9CiB9CkBAIC0yMzEwLDEzICsyMzEwLDEzIEBAIFN0cmluZyBI VE1MTWVkaWFFbGVtZW50OjpwcmVsb2FkKCkgY29uc3QKIHsKICAgICBzd2l0Y2ggKG1fcHJlbG9h ZCkgewogICAgIGNhc2UgTWVkaWFQbGF5ZXI6Ok5vbmU6Ci0gICAgICAgIHJldHVybiAibm9uZSI7 CisgICAgICAgIHJldHVybiBBU0NJSUxpdGVyYWwoIm5vbmUiKTsKICAgICAgICAgYnJlYWs7CiAg ICAgY2FzZSBNZWRpYVBsYXllcjo6TWV0YURhdGE6Ci0gICAgICAgIHJldHVybiAibWV0YWRhdGEi OworICAgICAgICByZXR1cm4gQVNDSUlMaXRlcmFsKCJtZXRhZGF0YSIpOwogICAgICAgICBicmVh azsKICAgICBjYXNlIE1lZGlhUGxheWVyOjpBdXRvOgotICAgICAgICByZXR1cm4gImF1dG8iOwor ICAgICAgICByZXR1cm4gQVNDSUlMaXRlcmFsKCJhdXRvIik7CiAgICAgICAgIGJyZWFrOwogICAg IH0KIApAQCAtMjUyMCw5ICsyNTIwLDYgQEAgdm9pZCBIVE1MTWVkaWFFbGVtZW50OjpzZXRMb29w KGJvb2wgYikKIHsKICAgICBMT0coTWVkaWEsICJIVE1MTWVkaWFFbGVtZW50OjpzZXRMb29wKCVz KSIsIGJvb2xTdHJpbmcoYikpOwogICAgIHNldEJvb2xlYW5BdHRyaWJ1dGUobG9vcEF0dHIsIGIp OwotI2lmIFBMQVRGT1JNKE1BQykKLSAgICB1cGRhdGVEaXNhYmxlU2xlZXAoKTsKLSNlbmRpZgog fQogCiBib29sIEhUTUxNZWRpYUVsZW1lbnQ6OmNvbnRyb2xzKCkgY29uc3QKQEAgLTM5NjYsMTQg KzM5NjMsMTQgQEAgdm9pZCBIVE1MTWVkaWFFbGVtZW50OjpnZXRQbHVnaW5Qcm94eVBhcmFtcyhL VVJMJiB1cmwsIFZlY3RvcjxTdHJpbmc+JiBuYW1lcywgVmUKICAgICBpZiAoaXNWaWRlbygpKSB7 CiAgICAgICAgIEtVUkwgcG9zdGVyVVJMID0gZ2V0Tm9uRW1wdHlVUkxBdHRyaWJ1dGUocG9zdGVy QXR0cik7CiAgICAgICAgIGlmICghcG9zdGVyVVJMLmlzRW1wdHkoKSAmJiBmcmFtZSAmJiBmcmFt ZS0+bG9hZGVyKCktPndpbGxMb2FkTWVkaWFFbGVtZW50VVJMKHBvc3RlclVSTCkpIHsKLSAgICAg ICAgICAgIG5hbWVzLmFwcGVuZCgiX21lZGlhX2VsZW1lbnRfcG9zdGVyXyIpOworICAgICAgICAg ICAgbmFtZXMuYXBwZW5kKEFTQ0lJTGl0ZXJhbCgiX21lZGlhX2VsZW1lbnRfcG9zdGVyXyIpKTsK ICAgICAgICAgICAgIHZhbHVlcy5hcHBlbmQocG9zdGVyVVJMLnN0cmluZygpKTsKICAgICAgICAg fQogICAgIH0KIAogICAgIGlmIChjb250cm9scygpKSB7Ci0gICAgICAgIG5hbWVzLmFwcGVuZCgi X21lZGlhX2VsZW1lbnRfY29udHJvbHNfIik7Ci0gICAgICAgIHZhbHVlcy5hcHBlbmQoInRydWUi KTsKKyAgICAgICAgbmFtZXMuYXBwZW5kKEFTQ0lJTGl0ZXJhbCgiX21lZGlhX2VsZW1lbnRfY29u dHJvbHNfIikpOworICAgICAgICB2YWx1ZXMuYXBwZW5kKEFTQ0lJTGl0ZXJhbCgidHJ1ZSIpKTsK ICAgICB9CiAKICAgICB1cmwgPSBzcmMoKTsKQEAgLTM5ODIsNyArMzk3OSw3IEBAIHZvaWQgSFRN TE1lZGlhRWxlbWVudDo6Z2V0UGx1Z2luUHJveHlQYXJhbXMoS1VSTCYgdXJsLCBWZWN0b3I8U3Ry aW5nPiYgbmFtZXMsIFZlCiAKICAgICBtX2N1cnJlbnRTcmMgPSB1cmw7CiAgICAgaWYgKHVybC5p c1ZhbGlkKCkgJiYgZnJhbWUgJiYgZnJhbWUtPmxvYWRlcigpLT53aWxsTG9hZE1lZGlhRWxlbWVu dFVSTCh1cmwpKSB7Ci0gICAgICAgIG5hbWVzLmFwcGVuZCgiX21lZGlhX2VsZW1lbnRfc3JjXyIp OworICAgICAgICBuYW1lcy5hcHBlbmQoQVNDSUlMaXRlcmFsKCJfbWVkaWFfZWxlbWVudF9zcmNf IikpOwogICAgICAgICB2YWx1ZXMuYXBwZW5kKG1fY3VycmVudFNyYy5zdHJpbmcoKSk7CiAgICAg fQogfQo=