[webkit-reviews] review denied: [Bug 15669] Build with -DXP_UNIX and -lXt for GTK+/X11 port : [Attachment 17593] Updated patch to fix alp's issues with previous patch

[bugzilla-daemon at webkit.org]

Alp Toker <alp at atoker.com> has denied Rodney Dawes <dobey at wayofthemonkey.com>'s
request for review:
Bug 15669: Build with -DXP_UNIX and -lXt for GTK+/X11 port
http://bugs.webkit.org/show_bug.cgi?id=15669

Attachment 17593: Updated patch to fix alp's issues with previous patch
http://bugs.webkit.org/attachment.cgi?id=17593&action=edit

------- Additional Comments from Alp Toker <alp at atoker.com>
>+sub enableGtkPlugins()
>+{
>+    determineEnableGtkPlugins();
>+    return $enableGtkPlugins;
>+}
>+
>+sub determineEnableGtkPlugins()
>+{
>+    return if defined($enableGtkPlugins);
>+
>+    if (checkArgv("--disable-gtk-plugins")) {
>+	  $enableGtkPlugins = 0;
>+    } else {
>+	  $enableGtkPlugins = 1;
>+    }
>+}

My comment from the previous review was:

  Also, linking to Xt doesn't sound like the right thing to do in standard
  WebKit/GTK+ builds. There should be an optional switch to enable this kind of

  feature.

If you really want to contribute this as a standalone patch independent of the
actual code, can you be sure to change this to disabled by default? There's no
indication that linking to Xt is a good default, especially when nothing in the
codebase uses it yet.

Usually small build system changes like this are included in the same patch as
the actual implementation. It's difficult to review one or the other by itself
(there is no indication why X intrinsics is being linked at all here).