Add WebKit preferences for text track settings

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

r101674 added WebCore settings to enable and disable subtitle, caption, and description text tracks. Add web preferences for them.

[Radar WebKit Bug Importer]

<rdar://problem/10522425>

[Eric Carlson]

Created attachment 117732 [details]
Proposed patch

[WebKit Review Bot]

Comment on attachment 117732 [details]
Proposed patch

Attachment 117732 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10733219

[Eric Carlson]

(In reply to comment #3)
> (From update of attachment 117732 [details])
> Attachment 117732 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/10733219

Blah blah, blah blah, blah blah.

[Eric Carlson]

Created attachment 117738 [details]
Updated patch

[Eric Carlson]

Created attachment 117757 [details]
Updated again

Don't export the settings from the Mac version of WebCore, it does't build with ENABLE_VIDEO_TRACK defined.

[WebKit Review Bot]

Comment on attachment 117757 [details]
Updated again

Attachment 117757 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10690474

New failing tests:
svg/custom/linking-uri-01-b.svg

[Jeff Miller]

Comment on attachment 117757 [details]
Updated again

Are you intentionally not exposing these settings via WebKit1 on Windows?

[John Sullivan]

Note that I reviewed this before I saw Jeff’s comment about WebKit1 on Windows.

[Eric Carlson]

(In reply to comment #9)
> Note that I reviewed this before I saw Jeff’s comment about WebKit1 on Windows.

Thanks, I will fix that oversight before committing.

[Darin Adler]

Comment on attachment 117757 [details]
Updated again

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

> Source/WebCore/page/Settings.cpp:854
> +#if ENABLE(VIDEO_TRACK)
> +void Settings::setShouldDisplaySubtitles(bool flag)
> +{
> +    m_shouldDisplaySubtitles = flag;
> +}
> +
> +void Settings::setShouldDisplayCaptions(bool flag)
> +{
> +    m_shouldDisplayCaptions = flag;
> +}
> +
> +void Settings::setShouldDisplayTextDescriptions(bool flag)
> +{
> +    m_shouldDisplayTextDescriptions = flag;
> +}
> +#endif

Why this change? You moved these into the cpp file, but they are the same as what was in the .h file?

[Eric Carlson]

(In reply to comment #11)
> (From update of attachment 117757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117757&action=review
> 
> > Source/WebCore/page/Settings.cpp:854
> > +#if ENABLE(VIDEO_TRACK)
> > +void Settings::setShouldDisplaySubtitles(bool flag)
> > +{
> > +    m_shouldDisplaySubtitles = flag;
> > +}
> > +
> > +void Settings::setShouldDisplayCaptions(bool flag)
> > +{
> > +    m_shouldDisplayCaptions = flag;
> > +}
> > +
> > +void Settings::setShouldDisplayTextDescriptions(bool flag)
> > +{
> > +    m_shouldDisplayTextDescriptions = flag;
> > +}
> > +#endif
> 
> Why this change? You moved these into the cpp file, but they are the same as what was in the .h file?

The functions were inlined when they were in the .h file (noted in the ChangeLog), so the symbols couldn't be exported from WebCore . I can move them back if you would rather.

[Eric Carlson]

Created attachment 117815 [details]
Updated patch

Updated to include Windows so the EWS bot can check it out.

[John Sullivan]

The new patch doesn’t have review? set. (If it did, I would review+ it.)

[Eric Carlson]

Created attachment 117818 [details]
Yet another patch.

[Eric Carlson]

(In reply to comment #14)
> The new patch doesn’t have review? set. (If it did, I would review+ it.)

I remembered this time, and this one should actually compile!

[John Sullivan]

Comment on attachment 117818 [details]
Yet another patch.

r+ if you’re right!

[WebKit Review Bot]

Comment on attachment 117818 [details]
Yet another patch.

Clearing flags on attachment: 117818

Committed r101977: <http://trac.webkit.org/changeset/101977>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #12)
=> The functions were inlined when they were in the .h file (noted in the ChangeLog), so the symbols couldn't be exported from WebCore.

For functions in the header with the inline keyword, you shouldn’t need to export them from WebCore. So I think that would have been a better way to god.

[Darin Adler]

to go.

[Eric Carlson]

(In reply to comment #20)
> (In reply to comment #12)
> => The functions were inlined when they were in the .h file (noted in the ChangeLog), so the symbols couldn't be exported from WebCore.
> 
> For functions in the header with the inline keyword, you shouldn’t need to export them from WebCore. So I think that would have been a better way to go.

Thanks, I will change it in a follow up patch.

[Eric Carlson]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #12)
> > => The functions were inlined when they were in the .h file (noted in the ChangeLog), so the symbols couldn't be exported from WebCore.
> > 
> > For functions in the header with the inline keyword, you shouldn’t need to export them from WebCore. So I think that would have been a better way to go.
> 
> Thanks, I will change it in a follow up patch.

https://bugs.webkit.org/show_bug.cgi?id=73879 is for the revert.