107579 – REGRESSION(r140231): media track layout tests crashing

RESOLVED FIXED 107579

REGRESSION(r140231): media track layout tests crashing

https://bugs.webkit.org/show_bug.cgi?id=107579

Summary REGRESSION(r140231): media track layout tests crashing

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	Details Formatted Diff Diff
Proposed fix 0.2 (5.27 KB, patch) 2013-01-24 15:46 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
Proposed fix 0.3 (22.63 KB, patch) 2013-01-28 16:53 PST, Dima Gorbik	buildbot: commit-queue-	Details Formatted Diff Diff
Proposed fix 0.4 (22.09 KB, patch) 2013-01-30 15:45 PST, Dima Gorbik	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Proposed fix 0.5 (22.10 KB, patch) 2013-01-30 17:37 PST, Dima Gorbik	eric.carlson: review+	Details Formatted Diff Diff
Proposed fix 0.5 (22.08 KB, patch) 2013-01-31 15:07 PST, Dima Gorbik	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Media

Assignee

Dima Gorbik

Reported

2013-01-22 12:21 PST

Modified

2013-01-31 19:59 PST History

CC List

20 users Show

URL

Keywords InRadar

Duplicates (2)

107921 108024 View as bug list

Depends on

Blocks

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