Bug 107398 - [GTK][GStreamer] FullscreenVideoControllerGtk implementation
Summary: [GTK][GStreamer] FullscreenVideoControllerGtk implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 106760 107458
Blocks:
  Show dependency tree / graph
 
Reported: 2013-01-20 07:12 PST by Philippe Normand
Modified: 2013-02-01 02:28 PST (History)
8 users (show)

See Also:

Attachments
Patch (41.86 KB, patch)
2013-01-20 07:23 PST, Philippe Normand 		no flags Details | Formatted Diff | Diff
Patch (41.85 KB, patch)
2013-01-26 07:15 PST, Philippe Normand 		no flags Details | Formatted Diff | Diff
Patch (41.89 KB, patch)
2013-01-30 07:00 PST, Philippe Normand 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 2 Philippe Normand 2013-01-26 07:15:10 PST 
Created attachment 184864 [details]
Patch
Comment 3 Philippe Normand 2013-01-30 07:00:42 PST 
Created attachment 185497 [details]
Patch

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

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]
Patch

Clearing r+ flag of landed patch