Bug 107398

Summary: [GTK][GStreamer] FullscreenVideoControllerGtk implementation
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: MediaAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: eric.carlson, feature-media-reviews, gustavo, menard, mrobinson, pnormand, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 106760, 107458    
Bug Blocks:    
Description Flags
Patch none

Description Philippe Normand 2013-01-20 07:12:07 PST
See also bug 106760
Comment 1 Philippe Normand 2013-01-20 07:23:51 PST
Created attachment 183672 [details]
Comment 2 Philippe Normand 2013-01-26 07:15:10 PST
Created attachment 184864 [details]
Comment 3 Philippe Normand 2013-01-30 07:00:42 PST
Created attachment 185497 [details]

Rebased against ToT
Comment 4 Gustavo Noronha (kov) 2013-01-31 05:11:31 PST
Comment on attachment 185497 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=185497&action=review

> Source/WebCore/platform/graphics/gtk/FullscreenVideoControllerGtk.h:82
> +    unsigned long m_exitFullcreenActionActivateSignalHandler;

Two naming nits about this class: these really aren't handlers but rather ids, so how about s/Handler/ID/? And secondly, keeping Gtk out of this class' name would avoid having an #if PLATFORM() block for each of GStreamer-based ports in the MediaPlayerPrivate part of this patch, wouldn't it?
Comment 5 Philippe Normand 2013-01-31 06:16:17 PST
Hum but how would that work?

I expect each port to have its own FullscreenController implementation deriving from the base class. Having a Gtk subsclass kinda makes sense to me :)

I guess I'm misunderstanding your point
Comment 6 Philippe Normand 2013-02-01 02:21:18 PST
Carlos clarified this for me, the create method should be moved to the GStreamer base class, avoiding the ugly #define FullscreenVideoControllerClass in the player so I can directly use the base class there.
Comment 7 Philippe Normand 2013-02-01 02:27:25 PST
Committed r141566: <http://trac.webkit.org/changeset/141566>
Comment 8 Philippe Normand 2013-02-01 02:28:26 PST
Comment on attachment 185497 [details]

Clearing r+ flag of landed patch