[webkit-reviews] review granted: [Bug 75646] Replace TextTrackCue "getCueAsSource" method with "text" attribute : [Attachment 121788] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 75646: Replace TextTrackCue "getCueAsSource" method with "text" attribute
https://bugs.webkit.org/show_bug.cgi?id=75646

Attachment 121788: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=121788&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.