Bug 75646 – Replace TextTrackCue "getCueAsSource" method with "text" attribute

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

TextTrackCue's 'getCueAsSource' method has been replaced with a mutable 'text' attribute

[Eric Carlson]

Created attachment 121788 [details]
Proposed patch

[Radar WebKit Bug Importer]

<rdar://problem/10667556>

[Darin Adler]

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.

[Eric Carlson]

(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.

[Eric Carlson]

http://trac.webkit.org/changeset/104624