Bug 73721

Summary:

Add WebKit preferences for text track settings

Product:

WebKit

Reporter:

Eric Carlson <eric.carlson>

Component:

Media

Assignee:

Eric Carlson <eric.carlson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

darin, dglazkov, jeffm, sullivan, webkit-bug-importer, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

43668

Attachments:

Description	Flags
Proposed patch	webkit.review.bot: commit-queue-
Updated patch	none
Updated again	sullivan: review+, webkit.review.bot: commit-queue-
Updated patch	none
Yet another patch.	none

Eric Carlson

Reported 2011-12-02 16:52:11 PST

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

Attachments
Proposed patch (17.93 KB, patch) 2011-12-02 18:48 PST, Eric Carlson	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Updated patch (17.94 KB, patch) 2011-12-02 20:42 PST, Eric Carlson	no flags	Details Formatted Diff Diff
Updated again (16.44 KB, patch) 2011-12-03 07:27 PST, Eric Carlson	sullivan: review+ webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Updated patch (24.42 KB, patch) 2011-12-04 16:17 PST, Eric Carlson	no flags	Details Formatted Diff Diff
Yet another patch. (24.42 KB, patch) 2011-12-04 18:19 PST, Eric Carlson	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2011-12-02 16:52:43 PST

<rdar://problem/10522425>

Eric Carlson

Comment 2 2011-12-02 18:48:12 PST

Created attachment 117732 [details] Proposed patch

WebKit Review Bot

Comment 3 2011-12-02 19:32:42 PST

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

Comment 4 2011-12-02 20:41:40 PST

(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

Comment 5 2011-12-02 20:42:32 PST

Created attachment 117738 [details] Updated patch

Eric Carlson

Comment 6 2011-12-03 07:27:18 PST

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 7 2011-12-03 08:16:58 PST

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 8 2011-12-03 09:33:56 PST

Comment on attachment 117757 [details] Updated again Are you intentionally not exposing these settings via WebKit1 on Windows?

John Sullivan

Comment 9 2011-12-04 07:16:30 PST

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

Eric Carlson

Comment 10 2011-12-04 14:11:08 PST

(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 11 2011-12-04 14:11:29 PST

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

Comment 12 2011-12-04 14:20:48 PST

(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

Comment 13 2011-12-04 16:17:54 PST

Created attachment 117815 [details] Updated patch Updated to include Windows so the EWS bot can check it out.

John Sullivan

Comment 14 2011-12-04 16:34:28 PST

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

Eric Carlson

Comment 15 2011-12-04 18:19:25 PST

Created attachment 117818 [details] Yet another patch.

Eric Carlson

Comment 16 2011-12-04 18:21:06 PST

(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 17 2011-12-04 20:00:08 PST

Comment on attachment 117818 [details] Yet another patch. r+ if you’re right!

WebKit Review Bot

Comment 18 2011-12-05 00:42:45 PST

Comment on attachment 117818 [details] Yet another patch. Clearing flags on attachment: 117818 Committed r101977: <http://trac.webkit.org/changeset/101977>

WebKit Review Bot

Comment 19 2011-12-05 00:42:50 PST

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 20 2011-12-05 11:59:33 PST

(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

Comment 21 2011-12-05 11:59:40 PST

to go.

Eric Carlson

Comment 22 2011-12-05 12:08:47 PST

(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

Comment 23 2011-12-05 17:22:42 PST

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

Note You need to log in before you can comment on or make changes to this bug.