Bug 190369 – [WebVTT] Support inline WebVTT styles

• Status: NEW
• Component: WebCore Misc.

[Per Arne Vollan]

Inline WebVTT styles are currently not supported in WebKit, see https://w3c.github.io/webvtt/#styling.

[Per Arne Vollan]

rdar://problem/29265907

[Per Arne Vollan]

Created attachment 351812 [details]
Patch

[Eric Carlson]

Comment on attachment 351812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351812&action=review

> Source/WebCore/dom/ExtensionStyleSheets.cpp:151
> +        for (auto styleSheet : *owningPage->webVTTStyleSheets()) {

const auto&

> Source/WebCore/dom/ExtensionStyleSheets.cpp:154
> +            m_injectedAuthorStyleSheets.append(WTFMove(sheet));

Won't this make the style apply to *every* <track> in the document? Styles in a WebVTT file should only apply to the cues in one track.

> Source/WebCore/html/track/LoadableTextTrack.cpp:137
> +void LoadableTextTrack::newStyleSheetsAvailable(TextTrackLoader* loader)

TextTrackLoader&, and please file a bug about changing all TextTrackLoader* method parameters in this class.

> Source/WebCore/html/track/LoadableTextTrack.cpp:142
> +    for (auto cssString : newStyleSheets)

const auto& cssString

> Source/WebCore/html/track/LoadableTextTrack.cpp:144
> +    if (!newStyleSheets.isEmpty())

You could return immediately above if the vector is empty.

> Source/WebCore/html/track/WebVTTParser.cpp:359
> +    if (line.startsWith("STYLE") && line.substring(regionIdentifierLength).isAllSpecialCharacters<isASpace>())

You need a new constant, "regionIdentifierLength" is wrong for STYLE

> LayoutTests/media/track/track-cue-css.html:19
> +            waitForEvent('seeked', seeked);

Nit: this could be simplified to "waitForEvent('seeked', endTest);"

[Per Arne Vollan]

Created attachment 352345 [details]
Patch

[Eric Carlson]

Comment on attachment 352345 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352345&action=review

> Source/WebCore/html/track/WebVTTParser.h:176
> +    ParseState collectStyleSheet(const String& line);

Nit: "line" isn't necessary.

> Source/WebCore/loader/TextTrackLoader.h:75
> +    void newStyleSheetsParsed() override;

Nit: override -> final

[Per Arne Vollan]

Created attachment 352464 [details]
Patch

[Per Arne Vollan]

(In reply to Eric Carlson from comment #5)
> Comment on attachment 352345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352345&action=review
> 
> > Source/WebCore/html/track/WebVTTParser.h:176
> > +    ParseState collectStyleSheet(const String& line);
> 
> Nit: "line" isn't necessary.
> 
> > Source/WebCore/loader/TextTrackLoader.h:75
> > +    void newStyleSheetsParsed() override;
> 
> Nit: override -> final

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 352464 [details]
Patch

Clearing flags on attachment: 352464

Committed r237187: <https://trac.webkit.org/changeset/237187>

[Truitt Savell]

Looks like the new test media/track/track-cue-css.html

added in https://trac.webkit.org/changeset/237187/webkit

is a flakey image failure.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Ftrack%2Ftrack-cue-css.html

Diff:
https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK1%20(Tests)/r237255%20(8670)/media/track/track-cue-css-diffs.html