Created attachment 121788 [details] Proposed patch

<rdar://problem/10667556>

Comment on attachment 121788 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=121788&action=review > Source/WebCore/html/TextTrackCue.cpp:331 > + OwnPtr<WebVTTParser> parser = WebVTTParser::create(0, m_scriptExecutionContext); > + m_documentFragment = parser->createDocumentFragmentFromCueText(m_content); Could we do this without a local variable? Or would that make the line too long? > Source/WebCore/html/TextTrackCue.h:92 > + String text() const { return m_content; } Seems we should return const String& here instead of String to avoid a bit of reference count churn. > Source/WebCore/html/track/WebVTTParser.cpp:252 > + if (!text.length()) > + return 0; Is it important to optimize the empty string case? Are there other cases we should optimize, such as “all whitespace”? > Source/WebCore/html/track/WebVTTParser.cpp:272 > + if (!m_currentContent.length()) > + return; Why is it important to special-case the empty case? Is that a common case? > Source/WebCore/html/track/WebVTTParser.cpp:274 > + RefPtr<DocumentFragment> attachmentRoot = createDocumentFragmentFromCueText(m_currentContent.toString()); I don’t see a check for 0 here. What guarantees this won’t return 0? > Source/WebCore/html/track/WebVTTParser.h:92 > + PassRefPtr<DocumentFragment> createDocumentFragmentFromCueText(const String&); There’s an extra space here after the > character.

(In reply to comment #3) > (From update of attachment 121788 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121788&action=review > > > Source/WebCore/html/TextTrackCue.cpp:331 > > + OwnPtr<WebVTTParser> parser = WebVTTParser::create(0, m_scriptExecutionContext); > > + m_documentFragment = parser->createDocumentFragmentFromCueText(m_content); > > Could we do this without a local variable? Or would that make the line too long? > Good suggestion, fixed. > > Source/WebCore/html/TextTrackCue.h:92 > > + String text() const { return m_content; } > > Seems we should return const String& here instead of String to avoid a bit of reference count churn. > Fixed. > > Source/WebCore/html/track/WebVTTParser.cpp:252 > > + if (!text.length()) > > + return 0; > > Is it important to optimize the empty string case? Are there other cases we should optimize, such as “all whitespace”? > > > Source/WebCore/html/track/WebVTTParser.cpp:272 > > + if (!m_currentContent.length()) > > + return; > > Why is it important to special-case the empty case? Is that a common case? > I removed these and found that we have a layout test that checks to make sure no cue is created for an entry without text. I don't see that requirement in the spec, but I am checking with people in the WebVTT community group to see if I am missing something. If it is not a spec requirement, I will write a bug to fix this. > > Source/WebCore/html/track/WebVTTParser.cpp:274 > > + RefPtr<DocumentFragment> attachmentRoot = createDocumentFragmentFromCueText(m_currentContent.toString()); > > I don’t see a check for 0 here. What guarantees this won’t return 0? > A NULL fragment should not be a problem. > > Source/WebCore/html/track/WebVTTParser.h:92 > > + PassRefPtr<DocumentFragment> createDocumentFragmentFromCueText(const String&); > > There’s an extra space here after the > character. Fixed.