There's no way to enable subtitles support on webkit2gtk. Subtitle support is a feature already supported by webcore that can be enabled by WebPreferences->shouldDisplaySubtitles();
Created attachment 203283 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 203285 [details] Patch
Created attachment 203286 [details] Patch
Comment on attachment 203286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203286&action=review In principle I'm OK with this setting, I can think of browsers letting users disable subtitles even if the site wants to show them, in particular in cases in which the built-in controls (which should have a cc button that would enable/disable subtitles AFAIK) are not used, but we have been taking an approach of only adding stuff that API users are actually using, do you have a use case for this setting already? > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1134 > + Missing * > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1136 > + * Since: 2.3 Should be 2.2 > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1143 > + FALSE, Any particular reason why we should not turn it on by default? I think working by default would be better. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:2842 > + bool currentValue = priv->preferences->shouldDisplaySubtitles(); > + if (currentValue == enabled) No reason to create a variable just for this check.
The goal of this patch changed to "Adding support to <video><track> subtitles on webkit2gtk" since it's not only about enable/disable the methods.
Created attachment 208724 [details] Patch
Is it really useful to have subtitles disabled?
Created attachment 208946 [details] Patch
Comment on attachment 208946 [details] Patch Attachment 208946 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1471594
Comment on attachment 208946 [details] Patch Attachment 208946 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1430428
(In reply to comment #11) > (From update of attachment 208946 [details]) > Attachment 208946 [details] did not pass efl-ews (efl): > Output: http://webkit-queues.appspot.com/results/1430428(In reply to comment #11) > (From update of attachment 208946 [details]) > Attachment 208946 [details] did not pass efl-ews (efl): > Output: http://webkit-queues.appspot.com/results/1430428 This build issue should be fixed by https://bugs.webkit.org/show_bug.cgi?id=119978
*** Bug 117649 has been marked as a duplicate of this bug. ***
Created attachment 209030 [details] Patch
Comment on attachment 209030 [details] Patch Attachment 209030 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1511093
Comment on attachment 209030 [details] Patch Attachment 209030 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1478479
Comment on attachment 209030 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209030&action=review > Source/WebCore/css/mediaControlsGtk.css:227 > + background-image: url('data:image/svg+xml,<svg id="svg7384" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="16" width="16" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/"><metadata id="metadata90"><rdf:RDF><cc:Work rdf:about=""><dc:format>image/svg+xml</dc:format><dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/><dc:title>Gnome Symbolic Icon Theme</dc:title></cc:Work></rdf:RDF></metadata><g id="layer9" transform="translate(-101,-357)"><path id="path12148" style="block-progression:tb;color:#bebebe;direction:ltr;text-indent:0;text-align:start;enable-background:accumulate;text-transform:none;" fill="#bebebe" d="m104.75,357.06c-2.0602,0-3.75,1.6898-3.75,3.75v4.4375c0,2.0602,1.6898,3.75,3.75,3.75h4.9375l3.75,2.6562,1.5938,1.125v-1.9688l-0.0313-2.5c1.1106-0.59715,1.9688-1.6526,1.9688-3.0625v-4.4375c0-2.0602-1.6898-3.75-3.75-3.75h-8.4688zm0,2,8.4688,0c0.9868,0,1.75,0.7632,1.75,1.75v4.4375c0,0.86273-0.63508,1.541-1.125,1.625l-0.84,0.12v0.84375,1.0312l-2.4062-1.6875-0.25-0.1875h-0.3125-5.2812c-0.9868,0-1.75-0.7632-1.75-1.75v-4.4375c0-0.9868,0.7632-1.75,1.75-1.75z"/></g></svg>'); Are other ports putting the icon in the CSS itself? > Source/WebCore/css/mediaControlsGtk.css:232 > + position: absolute; Absolute positioning? > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1155 > + WebKitSettings* settings = WEBKIT_SETTINGS(g_object_new(WEBKIT_TYPE_SETTINGS, NULL)); > + settings->priv->preferences->setShouldDisplaySubtitles(TRUE); > + return settings; It probably makes sense to do this in webKitSettingsConstructed instead.
(In reply to comment #17) > (From update of attachment 209030 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209030&action=review > > > Source/WebCore/css/mediaControlsGtk.css:227 > > + background-image: url('data:image/svg+xml,<svg id="svg7384" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns="http://www.w3.org/2000/svg" height="16" width="16" version="1.1" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/"><metadata id="metadata90"><rdf:RDF><cc:Work rdf:about=""><dc:format>image/svg+xml</dc:format><dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/><dc:title>Gnome Symbolic Icon Theme</dc:title></cc:Work></rdf:RDF></metadata><g id="layer9" transform="translate(-101,-357)"><path id="path12148" style="block-progression:tb;color:#bebebe;direction:ltr;text-indent:0;text-align:start;enable-background:accumulate;text-transform:none;" fill="#bebebe" d="m104.75,357.06c-2.0602,0-3.75,1.6898-3.75,3.75v4.4375c0,2.0602,1.6898,3.75,3.75,3.75h4.9375l3.75,2.6562,1.5938,1.125v-1.9688l-0.0313-2.5c1.1106-0.59715,1.9688-1.6526,1.9688-3.0625v-4.4375c0-2.0602-1.6898-3.75-3.75-3.75h-8.4688zm0,2,8.4688,0c0.9868,0,1.75,0.7632,1.75,1.75v4.4375c0,0.86273-0.63508,1.541-1.125,1.625l-0.84,0.12v0.84375,1.0312l-2.4062-1.6875-0.25-0.1875h-0.3125-5.2812c-0.9868,0-1.75-0.7632-1.75-1.75v-4.4375c0-0.9868,0.7632-1.75,1.75-1.75z"/></g></svg>'); > > Are other ports putting the icon in the CSS itself? As far as I know only apple is doing that. I would love to use a Gtk themed image, but there's no gtk images related to subtitling. The SVG I'm using is a stripped version of a chat image inside Gtk code. > > > Source/WebCore/css/mediaControlsGtk.css:232 > > + position: absolute; > > Absolute positioning? Well, yes. We're using the same approach for the volume slider. > > > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1155 > > + WebKitSettings* settings = WEBKIT_SETTINGS(g_object_new(WEBKIT_TYPE_SETTINGS, NULL)); > > + settings->priv->preferences->setShouldDisplaySubtitles(TRUE); > > + return settings; > > It probably makes sense to do this in webKitSettingsConstructed instead. I will push a new patch fixing that.
Created attachment 209033 [details] Patch
Comment on attachment 209033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209033&action=review Looks good, but please consider fixing a few nits before landing... > Source/WebCore/html/shadow/MediaControlsGtk.cpp:270 > + if (m_closedCaptionsContainer) { Maybe an early return here instead? > Source/WebCore/html/shadow/MediaControlsGtk.cpp:272 > + if (m_closedCaptionsContainer->isShowing()) > + hideClosedCaptionTrackList(); Ditto. > Source/WebCore/html/shadow/MediaControlsGtk.cpp:291 > + m_closedCaptionsContainer->show(); > + > + m_panel->setInlineStyleProperty(CSSPropertyPointerEvents, CSSValueNone); You can probably remove this empty line. > Source/WebCore/html/shadow/MediaControlsGtk.cpp:301 > + m_closedCaptionsContainer->hide(); > + > + m_panel->removeInlineStyleProperty(CSSPropertyPointerEvents); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:147 > + prefs->setShouldDisplaySubtitles(TRUE); > ExperimentalFeatures features; On the other hand, this probably deserves an empty newline. :) > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:1136 > readWriteConstructParamFlags)); > - > } I think this change is left over.
Created attachment 209082 [details] Patch
Comment on attachment 209082 [details] Patch Do we also need to update pixel results for tests that use the media player?
(In reply to comment #22) > (From update of attachment 209082 [details]) > Do we also need to update pixel results for tests that use the media player? As far as I could see, the pixel tests do not include the captions/subtitles button. So the player shouldn't change. However, might be a good idea to activate the tests on media/video-controls-captions-trackmenu.html instead of media/video-controls-captions.html I can change that when I start with the missing bits so the new tests can pass.
Comment on attachment 209082 [details] Patch asking for r+ and cq+
(In reply to comment #23) > I can change that when I start with the missing bits so the new tests can pass. What's still missing in the implementation?
(In reply to comment #25) > (In reply to comment #23) > > > I can change that when I start with the missing bits so the new tests can pass. > > What's still missing in the implementation? Right now if you click on the CC button you need to select an option so the menu can close. Apple solved this by capturing all events on the whole document and closing the dialog when the user clicks anywhere. Implementing this EventHandler is already on my task list for the next week. But since the subtitling support was totally disable on webkit2gtk, I imagined that it was ok to split that and push it separately.
Created attachment 209487 [details] Patch
Comment on attachment 209487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209487&action=review > Source/WebCore/html/shadow/MediaControlsGtk.cpp:46 > + return listener->type() == GObjectEventListenerType > + ? static_cast<const MediaControlsGtkEventListener*>(listener) > + : 0; This should really be one line. > Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp:146 > + prefs->setShouldDisplaySubtitles(TRUE); TRUE -> true > LayoutTests/platform/gtk/TestExpectations:793 > +webkit.org/b/101670 media/video-controls-captions-trackmenu.html [ Failure Pass ] > +webkit.org/b/101670 media/video-controls-captions-trackmenu-sorted.html [ Pass ] > +webkit.org/b/101670 media/video-controls-captions-trackmenu-localized.html [ Pass ] > +webkit.org/b/101670 media/video-controls-captions-trackmenu-hide-on-click.html [ Pass ] > +webkit.org/b/101670 media/video-controls-captions-trackmenu-hide-on-click-outside.html [ Pass ] I believe that marking them as passing here is like marking them flaky. Are these flaky?
Created attachment 209494 [details] Patch
Created attachment 209644 [details] Patch
Attachment 209644 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/gtk/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/mediaControlsGtk.css', u'Source/WebCore/html/shadow/MediaControlElements.cpp', u'Source/WebCore/html/shadow/MediaControlElements.h', u'Source/WebCore/html/shadow/MediaControlsGtk.cpp', u'Source/WebCore/html/shadow/MediaControlsGtk.h', u'Source/WebCore/page/CaptionUserPreferences.cpp', u'Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp', u'Source/WebCore/platform/gtk/RenderThemeGtk.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/UIProcess/API/gtk/WebKitSettings.cpp']" exit_code: 1 LayoutTests/platform/gtk/TestExpectations:1212: expecting "[", "#", or end of line instead of "[Failure" [test/expectations] [5] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 209647 [details] Patch
Created attachment 209902 [details] Patch
Comment on attachment 209902 [details] Patch the latest patch fix a small problem with the event handler blocking clicks on on the menu. Since the feature wasn't accepted upstream yet, I sending it inside this patch.
Comment on attachment 209902 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=209902&action=review > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:793 > - return String::fromUTF8(C_("Subtitles", "Menu section heading for subtitles")); > + return String::fromUTF8(_("Subtitles")); I think you should keep the context string, just put it in the proper place (first argument). > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:798 > - return String::fromUTF8(C_("Off", "Menu item label for the track that represents disabling closed captions")); > + return String::fromUTF8(_("Off")); ditto. > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:803 > - return String::fromUTF8(C_("No label", "Menu item label for a closed captions track that has no other name")); > + return String::fromUTF8(_("No label")); ditto.
Created attachment 210159 [details] Patch
(In reply to comment #35) > (From update of attachment 209902 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=209902&action=review > > > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:793 > > - return String::fromUTF8(C_("Subtitles", "Menu section heading for subtitles")); > > + return String::fromUTF8(_("Subtitles")); > > I think you should keep the context string, just put it in the proper place (first argument). > > > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:798 > > - return String::fromUTF8(C_("Off", "Menu item label for the track that represents disabling closed captions")); > > + return String::fromUTF8(_("Off")); > > ditto. > > > Source/WebCore/platform/gtk/LocalizedStringsGtk.cpp:803 > > - return String::fromUTF8(C_("No label", "Menu item label for a closed captions track that has no other name")); > > + return String::fromUTF8(_("No label")); > > ditto. Thanks for the comments kov... Fixed all those things...
Comment on attachment 210159 [details] Patch Attachment 210159 [details] did not pass gtk-wk2-ews (gtk-wk2): Output: http://webkit-queues.appspot.com/results/1627908
Comment on attachment 210159 [details] Patch Attachment 210159 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/1624962
Created attachment 210281 [details] Patch
(In reply to comment #40) > Created an attachment (id=210281) [details] > Patch Changing document()-> to document().
Created attachment 210348 [details] Patch
(In reply to comment #42) > Created an attachment (id=210348) [details] > Patch Removed the shouldDisplaySubtitles part from WebSettings since it tries to force webkit2 to use subtitles.
Created attachment 210396 [details] Patch
Comment on attachment 210396 [details] Patch This version should work on webkit1 and webkit2, "Off" and "Auto" buttons included.
Danilo and I were worried about the addition of the off and auto menu items to the core code, Eric says it seems fine to him, so I'm good with that =)
Comment on attachment 210396 [details] Patch Clearing flags on attachment: 210396 Committed r155048: <http://trac.webkit.org/changeset/155048>
All reviewed patches have been landed. Closing bug.