Bug 117008

Summary: [GTK] add support for subtitles on webkitGTK
Product: WebKit Reporter: Danilo Cesar Lemes de Paula <danilo.cesar>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, eflews.bot, eric.carlson, esprehn+autocc, glenn, gns, gtk-ews, gyuyoung.kim, gyuyoung.kim, jer.noble, macpherson, menard, mrobinson, rakuco, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 119978, 120629    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Danilo Cesar Lemes de Paula 2013-05-29 16:08:51 PDT
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();
Comment 1 Danilo Cesar Lemes de Paula 2013-05-29 16:10:05 PDT
Created attachment 203283 [details]
Patch
Comment 2 WebKit Commit Bot 2013-05-29 16:11:45 PDT
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
Comment 3 Danilo Cesar Lemes de Paula 2013-05-29 16:13:53 PDT
Created attachment 203285 [details]
Patch
Comment 4 Danilo Cesar Lemes de Paula 2013-05-29 16:20:08 PDT
Created attachment 203286 [details]
Patch
Comment 5 Gustavo Noronha (kov) 2013-05-29 16:22:50 PDT
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.
Comment 6 Danilo Cesar Lemes de Paula 2013-08-14 06:59:03 PDT
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.
Comment 7 Danilo Cesar Lemes de Paula 2013-08-14 07:05:02 PDT
Created attachment 208724 [details]
Patch
Comment 8 Martin Robinson 2013-08-14 07:32:01 PDT
Is it really useful to have subtitles disabled?
Comment 9 Danilo Cesar Lemes de Paula 2013-08-16 12:48:45 PDT
Created attachment 208946 [details]
Patch
Comment 10 EFL EWS Bot 2013-08-16 13:59:07 PDT
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 11 EFL EWS Bot 2013-08-16 21:00:07 PDT
Comment on attachment 208946 [details]
Patch

Attachment 208946 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1430428
Comment 12 Danilo Cesar Lemes de Paula 2013-08-18 05:49:44 PDT
(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
Comment 13 Danilo Cesar Lemes de Paula 2013-08-18 05:52:23 PDT
*** Bug 117649 has been marked as a duplicate of this bug. ***
Comment 14 Danilo Cesar Lemes de Paula 2013-08-18 07:04:56 PDT
Created attachment 209030 [details]
Patch
Comment 15 EFL EWS Bot 2013-08-18 07:10:04 PDT
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 16 EFL EWS Bot 2013-08-18 07:13:02 PDT
Comment on attachment 209030 [details]
Patch

Attachment 209030 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1478479
Comment 17 Martin Robinson 2013-08-18 07:14:38 PDT
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.
Comment 18 Danilo Cesar Lemes de Paula 2013-08-18 07:25:26 PDT
(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.
Comment 19 Danilo Cesar Lemes de Paula 2013-08-18 07:29:37 PDT
Created attachment 209033 [details]
Patch
Comment 20 Martin Robinson 2013-08-18 07:59:31 PDT
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.
Comment 21 Danilo Cesar Lemes de Paula 2013-08-19 06:40:35 PDT
Created attachment 209082 [details]
Patch
Comment 22 Martin Robinson 2013-08-19 06:49:14 PDT
Comment on attachment 209082 [details]
Patch

Do we also need to update pixel results for tests that use the media player?
Comment 23 Danilo Cesar Lemes de Paula 2013-08-21 06:32:50 PDT
(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 24 Danilo Cesar Lemes de Paula 2013-08-21 06:33:20 PDT
Comment on attachment 209082 [details]
Patch

asking for r+ and cq+
Comment 25 Martin Robinson 2013-08-21 07:42:14 PDT
(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?
Comment 26 Danilo Cesar Lemes de Paula 2013-08-21 09:36:35 PDT
(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.
Comment 27 Danilo Cesar Lemes de Paula 2013-08-23 12:07:17 PDT
Created attachment 209487 [details]
Patch
Comment 28 Martin Robinson 2013-08-23 12:26:35 PDT
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?
Comment 29 Danilo Cesar Lemes de Paula 2013-08-23 12:47:44 PDT
Created attachment 209494 [details]
Patch
Comment 30 Danilo Cesar Lemes de Paula 2013-08-26 06:37:10 PDT
Created attachment 209644 [details]
Patch
Comment 31 WebKit Commit Bot 2013-08-26 06:40:06 PDT
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.
Comment 32 Danilo Cesar Lemes de Paula 2013-08-26 06:45:12 PDT
Created attachment 209647 [details]
Patch
Comment 33 Danilo Cesar Lemes de Paula 2013-08-28 10:51:26 PDT
Created attachment 209902 [details]
Patch
Comment 34 Danilo Cesar Lemes de Paula 2013-08-28 10:53:52 PDT
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 35 Gustavo Noronha (kov) 2013-08-29 12:11:15 PDT
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.
Comment 36 Danilo Cesar Lemes de Paula 2013-08-30 14:11:32 PDT
Created attachment 210159 [details]
Patch
Comment 37 Danilo Cesar Lemes de Paula 2013-08-30 14:17:22 PDT
(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 38 kov's GTK+ EWS bot 2013-08-30 14:21:23 PDT
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 39 kov's GTK+ EWS bot 2013-08-30 14:42:51 PDT
Comment on attachment 210159 [details]
Patch

Attachment 210159 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1624962
Comment 40 Danilo Cesar Lemes de Paula 2013-09-02 06:48:25 PDT
Created attachment 210281 [details]
Patch
Comment 41 Danilo Cesar Lemes de Paula 2013-09-02 07:16:59 PDT
(In reply to comment #40)
> Created an attachment (id=210281) [details]
> Patch

Changing document()-> to document().
Comment 42 Danilo Cesar Lemes de Paula 2013-09-03 03:37:45 PDT
Created attachment 210348 [details]
Patch
Comment 43 Danilo Cesar Lemes de Paula 2013-09-03 03:40:32 PDT
(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.
Comment 44 Danilo Cesar Lemes de Paula 2013-09-03 12:03:58 PDT
Created attachment 210396 [details]
Patch
Comment 45 Danilo Cesar Lemes de Paula 2013-09-03 12:05:40 PDT
Comment on attachment 210396 [details]
Patch

This version should work on webkit1 and webkit2, "Off" and "Auto" buttons included.
Comment 46 Gustavo Noronha (kov) 2013-09-03 12:28:23 PDT
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 47 WebKit Commit Bot 2013-09-04 10:38:02 PDT
Comment on attachment 210396 [details]
Patch

Clearing flags on attachment: 210396

Committed r155048: <http://trac.webkit.org/changeset/155048>
Comment 48 WebKit Commit Bot 2013-09-04 10:38:08 PDT
All reviewed patches have been landed.  Closing bug.