[webkit-reviews] review granted: [Bug 117008] [WK2][GTK] add support for subtitles on webkit2GTK : [Attachment 209033] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Danilo Cesar Lemes de Paula
<danilo.cesar at collabora.co.uk>'s request for review:
Bug 117008: [WK2][GTK] add support for subtitles on webkit2GTK
https://bugs.webkit.org/show_bug.cgi?id=117008

Attachment 209033: Patch
https://bugs.webkit.org/attachment.cgi?id=209033&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.