Bug 107579

Summary: REGRESSION(r140231): media track layout tests crashing
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: MediaAssignee: Dima Gorbik <dgorbik>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, allan.jensen, buildbot, cmarcelo, dglazkov, dgorbik, eric.carlson, esprehn, keishi, macpherson, menard, ojan.autocc, peter+ews, rniwa, sam, scherkus, silviapf, vcarbune, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed fix 0.1
none
Proposed fix 0.2
none
Proposed fix 0.3
buildbot: commit-queue-
Proposed fix 0.4
webkit.review.bot: commit-queue-
Proposed fix 0.5
eric.carlson: review+
Proposed fix 0.5 none

Hin-Chung Lam
Reported 2013-01-22 12:21:13 PST
On Chromium bots we recorded these crashes: media/track/track-css-cue-lifetime.html media/track/track-css-matching.html media/track/track-css-property-whitelist.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=@ToT - chromium.org&tests=media/track/track-css-cue-lifetime.html,media/track/track-css-matching.html,media/track/track-css-property-whitelist.html Log points to r140231. Crash log: crash log for DumpRenderTree (pid 14860): STDOUT: <empty> STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() STDERR: Backtrace: STDERR: std::_Init_locks::operator= [0x5FC49F63+172035] STDERR: std::_Init_locks::operator= [0x5FC49EEA+171914] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F29AB88+15414024] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F22C79E+14962462] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E52BA62+1327586] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E52AE50+1324496] STDERR: v8::Locker::StopPreemption [0x6BF16357+811159] STDERR: v8::Locker::StopPreemption [0x6BF18D6A+821930] STDERR: v8::Locker::StopPreemption [0x6C163D0C+3226188] STDERR: v8::Locker::StopPreemption [0x6C169EF1+3251249] STDERR: (No symbol) [0x1C80A3F6] STDERR: (No symbol) [0x1C83C53B] STDERR: (No symbol) [0x1C80E581] STDERR: (No symbol) [0x1C83B624] STDERR: (No symbol) [0x1C822679] STDERR: (No symbol) [0x1C8134CA] STDERR: v8::Locker::StopPreemption [0x6BEB9B05+432197] STDERR: v8::Locker::StopPreemption [0x6BEB98B4+431604] STDERR: v8::Function::Call [0x6BE33334+516] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EF43273+11909619] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EF42FF7+11908983] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F68CB3F+19550911] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F69A3F2+19606386] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5F69A0F0+19605616] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E99D49F+5987359] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E99CDDF+5985631] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E9CC0AB+6178859] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EB21217+7575959] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEC198+7358744] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEB8AC+7356460] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAE929C+7346716] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EAEA5FC+7351676] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5E9CC323+6179491] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EB73503+7912579] STDERR: std::_Init_locks::operator= [0x5FDF6E88+1929000] STDERR: std::_Init_locks::operator= [0x5FDF7033+1929427] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EE52233+10922419] STDERR: WebKit::WebFilterOperation::WebFilterOperation [0x5EE52146+10922182] STDERR: WebDropData::operator= [0x69E635FB+214387] STDERR: WebDropData::operator= [0x69E712CB+270915] STDERR: WebDropData::operator= [0x69E7121A+270738] STDERR: WebDropData::operator= [0x69E7113A+270514] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1DE35F+451578] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D292D6B+1191430] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D292A96+1190705] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D29350B+1193382] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D2934BA+1193301] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D29341A+1193141] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1DE35F+451578] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E9974+498191] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E9DA4+499263] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1EAC63+503038] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1F9444+562399] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1F8552+558573] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D18660C+91815] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E94B9+496980] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E920B+496294] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1B4CE9+281988] STDERR: tracked_objects::ParentChildPairSnapshot::ParentChildPairSnapshot [0x6D1E8501+492956] STDERR: (No symbol) [0x00D61A4D] STDERR: (No symbol) [0x00C7E45D] STDERR: (No symbol) [0x00CC6D38]
Attachments
Proposed fix 0.1 (5.26 KB, patch)
2013-01-24 15:23 PST, Dima Gorbik 		no flags
DetailsFormatted DiffDiff
Proposed fix 0.2 (5.27 KB, patch)
2013-01-24 15:46 PST, Dima Gorbik 		no flags
DetailsFormatted DiffDiff
Proposed fix 0.3 (22.63 KB, patch)
2013-01-28 16:53 PST, Dima Gorbik 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Proposed fix 0.4 (22.09 KB, patch)
2013-01-30 15:45 PST, Dima Gorbik 		webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Proposed fix 0.5 (22.10 KB, patch)
2013-01-30 17:37 PST, Dima Gorbik 		eric.carlson: review+
DetailsFormatted DiffDiff
Proposed fix 0.5 (22.08 KB, patch)
2013-01-31 15:07 PST, Dima Gorbik 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Hin-Chung Lam
Comment 1 2013-01-22 12:28:12 PST
Committed r140450: <http://trac.webkit.org/changeset/140450>
Hin-Chung Lam
Comment 2 2013-01-22 12:29:33 PST
Sorry that was a bad trace from Win.. Here's a trace on OSX 10.7 dbg: crash log for DumpRenderTree (pid 2583): STDOUT: <empty> STDERR: objc[2583]: Class MockCrApp is implemented in both /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/libwebkit.dylib and /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/DumpRenderTree.app/Contents/MacOS/DumpRenderTree. One of the two will be used. Which one is undefined. STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() STDERR: ../../third_party/WebKit/Source/WebCore/html/HTMLUnknownElement.h(57) : WebCore::HTMLUnknownElement *WebCore::toHTMLUnknownElement(WebCore::HTMLElement *) STDERR: 1 0x6f1403f WebCore::toHTMLUnknownElement(WebCore::HTMLElement*) STDERR: 2 0x6f11449 WebCore::createV8HTMLWrapper(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 3 0x7a82c14 WebCore::wrap(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 4 0x7a9f2d3 WebCore::wrap(WebCore::Node*, v8::Handle<v8::Object>, v8::Isolate*) STDERR: 5 0x6e46e64 v8::Handle<v8::Value> WebCore::toV8Fast<v8::AccessorInfo, WebCore::Node>(WebCore::Node*, v8::AccessorInfo const&, WebCore::Node*) STDERR: 6 0x6e26bd8 _ZN7WebCore14NodeV8InternalL20firstChildAttrGetterEN2v85LocalINS1_6StringEEERKNS1_12AccessorInfoE STDERR: 7 0x110efb0 v8::internal::JSObject::GetPropertyWithCallback(v8::internal::Object*, v8::internal::Object*, v8::internal::String*) STDERR: 8 0x110eaa9 v8::internal::Object::GetProperty(v8::internal::Object*, v8::internal::LookupResult*, v8::internal::String*, PropertyAttributes*) STDERR: 9 0x1050128 v8::internal::LoadIC::Load(v8::internal::InlineCacheState, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::String>) STDERR: 10 0x105803e v8::internal::LoadIC_Miss(v8::internal::Arguments, v8::internal::Isolate*) STDERR: 11 0x34a0a3f6 STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef STDERR: [0x000003d391af] STDERR: [0x000003d3914b] STDERR: [0x000003d38ddb] STDERR: [0x000096f1059b] STDERR: [0x0000ffffffff] STDERR: [0x000006f11449] STDERR: [0x000007a82c14] STDERR: [0x000007a9f2d3] STDERR: [0x000006e46e64] STDERR: [0x000006e26bd8] STDERR: [0x00000110efb0] STDERR: [0x00000110eaa9] STDERR: [0x000001050128] STDERR: [0x00000105803e] STDERR: [0x000034a0a3f6] STDERR: ax: bbadbeef, bx: 6fb2dd00, cx: 6fb2dde5, dx: 6fb2dde5 STDERR: di: 880b1f2, si: 880b19f, bp: c00a67b8, sp: c00a6780, ss: 23, flags: 10286 STDERR: ip: 6f14049, cs: 1b, ds: 23, es: 23, fs: 0, gs: f
Dima Gorbik
Comment 3 2013-01-22 13:04:40 PST
(In reply to comment #2) > Sorry that was a bad trace from Win.. Here's a trace on OSX 10.7 dbg: > > crash log for DumpRenderTree (pid 2583): > STDOUT: <empty> > STDERR: objc[2583]: Class MockCrApp is implemented in both /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/libwebkit.dylib and /Volumes/data/b/build/slave/WebKit_Mac10_7__dbg_/build/src/xcodebuild/Debug/DumpRenderTree.app/Contents/MacOS/DumpRenderTree. One of the two will be used. Which one is undefined. > STDERR: ASSERTION FAILED: !element || element->isHTMLUnknownElement() > STDERR: ../../third_party/WebKit/Source/WebCore/html/HTMLUnknownElement.h(57) : WebCore::HTMLUnknownElement *WebCore::toHTMLUnknownElement(WebCore::HTMLElement *) > STDERR: 1 0x6f1403f WebCore::toHTMLUnknownElement(WebCore::HTMLElement*) > STDERR: 2 0x6f11449 WebCore::createV8HTMLWrapper(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) > STDERR: 3 0x7a82c14 WebCore::wrap(WebCore::HTMLElement*, v8::Handle<v8::Object>, v8::Isolate*) I wonder why was this call made to toHTMLUnknownElement? Where the code for createV8HTMLWrapper is located?
Hin-Chung Lam
Comment 4 2013-01-22 13:11:34 PST
Adding abarth@ to see if he knows.
Hin-Chung Lam
Comment 5 2013-01-22 13:25:00 PST
Adding scherkus@chromium.org
Dima Gorbik
Comment 6 2013-01-22 16:29:57 PST
Discussed this with Elliott on IRC. Will subclass Element instead of HTMLElement for the WebVTTElement, because sometimes it doesn't use general element names that are declared in HTMLNames.in.
Dima Gorbik
Comment 7 2013-01-24 15:23:54 PST
Created attachment 184592 [details] Proposed fix 0.1
Radar WebKit Bug Importer
Comment 8 2013-01-24 15:24:59 PST
<rdar://problem/13081759>
Dima Gorbik
Comment 9 2013-01-24 15:46:00 PST
Created attachment 184597 [details] Proposed fix 0.2
Keishi Hattori
Comment 10 2013-01-24 23:32:22 PST
*** Bug 107921 has been marked as a duplicate of this bug. ***
Elliott Sprehn
Comment 11 2013-01-25 13:44:30 PST
Comment on attachment 184597 [details] Proposed fix 0.2 View in context: https://bugs.webkit.org/attachment.cgi?id=184597&action=review This won't work because now you create a <b> element in the xhtml namespace that isn't an HTMLElement. > Source/WebCore/html/track/TextTrackCue.cpp:512 > + clonedNode = HTMLElement::create(toElement(node)->tagQName(), static_cast<Document*>(m_scriptExecutionContext)); It would be much nicer if you just added a method on TextTrackCue called document() that did this and got rid of these casts all over. > Source/WebCore/html/track/WebVTTElement.cpp:37 > + : Element(tagName, document, CreateElement) This isn't okay since WebVTTParser::constructTreeFromToken still creates qnames in the xhtmlNamespace so your Elements are still in the HTML namespace, but now they're not even HTMLElement instances. :( You should put them in the empty namespace, or some webvtt namespace.
Dima Gorbik
Comment 12 2013-01-28 14:48:16 PST
*** Bug 108024 has been marked as a duplicate of this bug. ***
Dima Gorbik
Comment 13 2013-01-28 16:53:49 PST
Created attachment 185106 [details] Proposed fix 0.3
Dima Gorbik
Comment 14 2013-01-28 17:00:45 PST
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review > Source/WebCore/html/track/WebVTTElement.cpp:86 > - return adoptRef(new WebVTTElement(tagName, document)); > + RefPtr<WebVTTElement> newElement = adoptRef(new WebVTTElement(nodeType, document)); > + return newElement; Oops, this seem unnecessary.
Dima Gorbik
Comment 15 2013-01-28 17:00:48 PST
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review > Source/WebCore/html/track/WebVTTElement.cpp:86 > - return adoptRef(new WebVTTElement(tagName, document)); > + RefPtr<WebVTTElement> newElement = adoptRef(new WebVTTElement(nodeType, document)); > + return newElement; Oops, this seem unnecessary.
Build Bot
Comment 16 2013-01-28 17:10:23 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16125004
Peter Beverloo (cr-android ews)
Comment 17 2013-01-28 17:44:09 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16165825
Eric Carlson
Comment 18 2013-01-28 17:58:15 PST
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review > Source/WebCore/html/track/WebVTTElement.cpp:74 > +} You need a return here to prevent a compiler error on some ports. Please also add an ASSERT_NOT_REACHED(). > Source/WebCore/html/track/WebVTTElement.cpp:79 > +: Element(nodeTypeToTagName(nodeType), document, CreateElement) > +, m_isPastNode(0) > +, m_webVTTNodeType(nodeType) Nit: these should be indented. > Source/WebCore/html/track/WebVTTElement.cpp:124 > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); Why langAttributeName() instead of langAttr? Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect?
Dima Gorbik
Comment 19 2013-01-28 18:33:49 PST
> > Source/WebCore/html/track/WebVTTElement.cpp:124 > > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > Why langAttributeName() instead of langAttr? These are not standard html names, they are not defined in HTMLAttributeNames.in. Or would you like to rename langAttributeName to langAttr? > Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect? I think we set class for all nodes before. But in case of lang — yes, it should be set to all nodes, that was a bug before.
Eric Carlson
Comment 20 2013-01-28 18:51:36 PST
(In reply to comment #19) > > > Source/WebCore/html/track/WebVTTElement.cpp:124 > > > + htmlElement.get()->setAttribute(langAttributeName(), getAttribute(langAttributeName())); > > > + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > > > Why langAttributeName() instead of langAttr? > > These are not standard html names, they are not defined in HTMLAttributeNames.in. Or would you like to rename langAttributeName to langAttr? > I am not sure what you mean. This method creates the equivalent HTMLElement. In the current code we have: setAttribute(langAttr, ... so why would we not now have: setAttribute(HTMLNames::langAttr, ...
Dima Gorbik
Comment 21 2013-01-28 18:54:37 PST
(In reply to comment #20) > setAttribute(langAttr, ... > > so why would we not now have: > > setAttribute(HTMLNames::langAttr, ... Oh, I didn't release we have 'lang' is the html namespace. I guess langAttributeName() may be removed then.
Build Bot
Comment 22 2013-01-28 20:01:29 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16149937
EFL EWS Bot
Comment 23 2013-01-28 21:04:57 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/16188007
WebKit Review Bot
Comment 24 2013-01-28 21:50:03 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16161966
Build Bot
Comment 25 2013-01-28 22:43:09 PST
Comment on attachment 185106 [details] Proposed fix 0.3 Attachment 185106 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16185121
Elliott Sprehn
Comment 26 2013-01-29 15:11:12 PST
Comment on attachment 185106 [details] Proposed fix 0.3 View in context: https://bugs.webkit.org/attachment.cgi?id=185106&action=review Looking pretty good, just a couple things. > Source/WebCore/html/track/TextTrackCue.cpp:507 > + clonedNode = toWebVTTElement(node)->createEquivalentHTMLElement(document()); Is there a reason to pass in the document() instead of just using the document() inside the node? > Source/WebCore/html/track/TextTrackCue.h:255 > + inline Document* document() { return static_cast<Document*>(m_scriptExecutionContext); } This method is not needed, you already have ownerDocument() right above. Also you don't need "inline" if the method is inside the class def. > Source/WebCore/html/track/WebVTTElement.cpp:64 > + break; Remove the break statements after the returns. You don't need them. >> Source/WebCore/html/track/WebVTTElement.cpp:74 >> +} > > You need a return here to prevent a compiler error on some ports. Please also add an ASSERT_NOT_REACHED(). I don't think you actually need that return, ex. ShadowRoot::childTypeAllowed >> Source/WebCore/html/track/WebVTTElement.cpp:124 >> + htmlElement.get()->setAttribute(HTMLNames::classAttr, getAttribute(HTMLNames::classAttr)); > > Why langAttributeName() instead of langAttr? > > Previously we only set 'class' and 'lang' for voice, lang, and class nodes. Was that incorrect? This is an HTML Element, so you need to use HTMLNames::langAttr. > Source/WebCore/html/track/WebVTTElement.h:33 > + WebVTTNodeTypeNone, Make None = 0 by default. > Source/WebCore/html/track/WebVTTElement.h:77 > + unsigned m_webVTTNodeType:4; Spaces around the colon.
Dima Gorbik
Comment 27 2013-01-30 15:39:06 PST
(In reply to comment #26) > > Source/WebCore/html/track/TextTrackCue.cpp:507 > > + clonedNode = toWebVTTElement(node)->createEquivalentHTMLElement(document()); > > Is there a reason to pass in the document() instead of just using the document() inside the node? Document is an m_scriptExecutionContext which is a member of TextTrackCue. I guess it will not be visible inside the element.
Dima Gorbik
Comment 28 2013-01-30 15:45:40 PST
Created attachment 185600 [details] Proposed fix 0.4
Elliott Sprehn
Comment 29 2013-01-30 15:46:53 PST
Comment on attachment 185600 [details] Proposed fix 0.4 View in context: https://bugs.webkit.org/attachment.cgi?id=185600&action=review > Source/WebCore/html/track/WebVTTElement.cpp:93 > + htmlElement = HTMLElement::create(HTMLNames::spanTag, document); Just call document() here, you shouldn't need to pass it in.
WebKit Review Bot
Comment 30 2013-01-30 16:16:50 PST
Comment on attachment 185600 [details] Proposed fix 0.4 Attachment 185600 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16198933
Peter Beverloo (cr-android ews)
Comment 31 2013-01-30 16:17:23 PST
Comment on attachment 185600 [details] Proposed fix 0.4 Attachment 185600 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/16249020
Dima Gorbik
Comment 32 2013-01-30 17:37:39 PST
Created attachment 185635 [details] Proposed fix 0.5
Ryosuke Niwa
Comment 33 2013-01-30 18:06:59 PST
Comment on attachment 185635 [details] Proposed fix 0.5 View in context: https://bugs.webkit.org/attachment.cgi?id=185635&action=review > Source/WebCore/html/track/WebVTTElement.cpp:36 > +static QualifiedName& nodeTypeToTagName(WebVTTNodeType nodeType) Can we use const QualifiedName& instead? > Source/WebCore/html/track/WebVTTParser.cpp:361 > + if (token.name()[0] == 'r' && token.name()[1] == 't') Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()?
Dima Gorbik
Comment 34 2013-01-30 19:02:33 PST
(In reply to comment #33) > > Source/WebCore/html/track/WebVTTParser.cpp:361 > > + if (token.name()[0] == 'r' && token.name()[1] == 't') > > Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()? I am not really sure how you can compare pointers like this. The token comes from the parser, we don't use constant values like HTMLNames::rtTag.localName() to populate its name, if we did your second suggestion would work. We could construct AtomicStrings or Just Strings from the token and compare but I am not sure it's worth doing. I will use hash tables later if we end up with lots of different tokens in WebVTT in future.
Ryosuke Niwa
Comment 35 2013-01-30 19:11:18 PST
(In reply to comment #34) > (In reply to comment #33) > > > Source/WebCore/html/track/WebVTTParser.cpp:361 > > > + if (token.name()[0] == 'r' && token.name()[1] == 't') > > > > Is it really too slow to do token.name() == "rt" or token.name() == HTMLNames::rtTag.localName()? > > I am not really sure how you can compare pointers like this. The token comes from the parser, we don't use constant values like HTMLNames::rtTag.localName() to populate its name, if we did your second suggestion would work. We could construct AtomicStrings or Just Strings from the token and compare but I am not sure it's worth doing. I will use hash tables later if we end up with lots of different tokens in WebVTT in future. Ah, ok.
Eric Carlson
Comment 36 2013-01-31 07:03:23 PST
Comment on attachment 185635 [details] Proposed fix 0.5 View in context: https://bugs.webkit.org/attachment.cgi?id=185635&action=review > Source/WebCore/html/track/WebVTTParser.cpp:357 > + switch (token.name().size()) { > + case 1: > + if (token.name()[0] == 'c') > + return WebVTTNodeTypeClass; > + if (token.name()[0] == 'v') > + return WebVTTNodeTypeVoice; > + if (token.name()[0] == 'b') > + return WebVTTNodeTypeBold; > + if (token.name()[0] == 'i') > + return WebVTTNodeTypeItalic; > + if (token.name()[0] == 'u') Nit: this function does a lot of "token.name()". I would be cleaner to put the token name into a local variable.
Dima Gorbik
Comment 37 2013-01-31 15:07:03 PST
Created attachment 185875 [details] Proposed fix 0.5
WebKit Review Bot
Comment 38 2013-01-31 19:59:10 PST
Comment on attachment 185875 [details] Proposed fix 0.5 Clearing flags on attachment: 185875 Committed r141529: <http://trac.webkit.org/changeset/141529>
WebKit Review Bot
Comment 39 2013-01-31 19:59:17 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.