RESOLVED FIXED 73721
Add WebKit preferences for text track settings 
https://bugs.webkit.org/show_bug.cgi?id=73721
Summary Add WebKit preferences for text track settings
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-
DetailsFormatted DiffDiff
Updated patch (17.94 KB, patch)
2011-12-02 20:42 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Updated again (16.44 KB, patch)
2011-12-03 07:27 PST, Eric Carlson 		sullivan: review+
webkit.review.bot: commit-queue-
DetailsFormatted DiffDiff
Updated patch (24.42 KB, patch)
2011-12-04 16:17 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
Yet another patch. (24.42 KB, patch)
2011-12-04 18:19 PST, Eric Carlson 		no flags
DetailsFormatted DiffDiff
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.