Bug 51591 - [GTK] Disable flash plugin in GtkLauncher when using gtk3
Summary: [GTK] Disable flash plugin in GtkLauncher when using gtk3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk, PlatformOnly
Depends on:
Blocks:
 
Reported: 2010-12-24 07:21 PST by Carlos Garcia Campos
Modified: 2010-12-27 10:20 PST (History)
6 users (show)

See Also:


Attachments
Patch to disable flash plugin in GtkLauncher when using gtk3 (1.50 KB, patch)
2010-12-24 07:30 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (1.78 KB, patch)
2010-12-27 03:58 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2010-12-24 07:21:49 PST
Flash plugin uses gtk2 that is incompatible with gtk3, so when webkit is built with gtk3, flash plugin makes us crash.
Comment 1 Carlos Garcia Campos 2010-12-24 07:30:05 PST
Created attachment 77414 [details]
Patch to disable flash plugin in GtkLauncher when using gtk3
Comment 2 Eric Seidel (no email) 2010-12-24 08:55:37 PST
Comment on attachment 77414 [details]
Patch to disable flash plugin in GtkLauncher when using gtk3

Shouldn't this be in a separate helper function (static) for better code encapsulation?
Comment 3 Daniel Bates 2010-12-24 12:55:41 PST
Comment on attachment 77414 [details]
Patch to disable flash plugin in GtkLauncher when using gtk3

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

> Tools/GtkLauncher/main.c:247
> +    for (l = plugins; l; l = g_slist_next(l)) {

Nit: I suggest renaming 'l' (lowercase 'L' character) to some other variable name (maybe p?) because it looks similar to 'I' (capital eye character) for many fonts, including Courier, Helvetica and Lucida Grande. Moreover, 'p' is closer to the word plugin than 'l'.

Additionally, I suggest inlining the declaration of 'l' in the for-loop initialization clause since its scope is only relevant within the loop body.
Comment 4 Martin Robinson 2010-12-24 13:01:31 PST
Comment on attachment 77414 [details]
Patch to disable flash plugin in GtkLauncher when using gtk3

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

>> Tools/GtkLauncher/main.c:247
>> +    for (l = plugins; l; l = g_slist_next(l)) {
> 
> Nit: I suggest renaming 'l' (lowercase 'L' character) to some other variable name (maybe p?) because it looks similar to 'I' (capital eye character) for many fonts, including Courier, Helvetica and Lucida Grande. Moreover, 'p' is closer to the word plugin than 'l'.
> 
> Additionally, I suggest inlining the declaration of 'l' in the for-loop initialization clause since its scope is only relevant within the loop body.

Moving the declaration of 'l' may produce a warning since this file is compiled with -ansi. If that's the case, I'm in favor of dropping -ansi. We should be using c99 at this point.
Comment 5 Carlos Garcia Campos 2010-12-27 03:58:23 PST
Created attachment 77489 [details]
Updated patch

Thanks for the comments, this is an updated patch that moves the code to a function and renames the list iterator to 'p'. I haven't moved the list declaration to the for initialization clause because we are building with -ansi flag.
Comment 6 WebKit Commit Bot 2010-12-27 09:37:38 PST
Comment on attachment 77489 [details]
Updated patch

Clearing flags on attachment: 77489

Committed r74685: <http://trac.webkit.org/changeset/74685>
Comment 7 WebKit Commit Bot 2010-12-27 09:37:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 WebKit Review Bot 2010-12-27 10:20:33 PST
http://trac.webkit.org/changeset/74685 might have broken Leopard Intel Debug (Tests)