[webkit-reviews] review denied: [Bug 37369] [GTK] Enable building whatever already exists of WebKit2 : [Attachment 74419] Makefile changes to building the WebKit2 GTK port

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 37369: [GTK] Enable building whatever already exists of WebKit2
https://bugs.webkit.org/show_bug.cgi?id=37369

Attachment 74419: Makefile changes to building the WebKit2 GTK port
https://bugs.webkit.org/attachment.cgi?id=74419&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74419&action=review

Very nice patch. Thanks for updating it. Just a couple more lingering issues. I
think it's very close.

> WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

This line should be removed.

> WebCore/platform/network/soup/cache/webkit/soup-cache.h:27
> +#define WEBKIT_API __attribute__((visibility("default")))
> +#define WEBKIT_OBSOLETE_API WEBKIT_API __attribute__((deprecated))

This will actually break the GTK+ Windows build. You should probably include
the MSVC support here too.

> WebKit2/GNUmakefile.am:580
> +# Ensure that the prefix header is built before

You are no longer using using a precompiled header, so I think this comment is
out of date. :)

> WebKit2/WebKit2Prefix.h:33
> +#if !defined (BUILDING_GTK__)
>  #include <wtf/Platform.h>
>  #include <wtf/DisallowCType.h>
>  #ifdef __cplusplus
>  #include <wtf/FastMalloc.h>
>  #endif
> +#endif /* !defined (BUILDING_GTK__) */

Why do you need to conditionally include these?

> WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-33
> -#include "PluginView.h"

I think the right way to solve this problem is to only include the big list of
-I directories in WebCore if we are building WebKit 1. Otherwise, we'll
probably keep running into this issue. Do you mind trying that?

> configure.ac:903
> +HTTP_STACK="soup"
> +WEBKITGTK_PC_NAME=${WEBKITGTK_PC_NAME}2
> +AC_SUBST([HTTP_STACK])
> +AC_SUBST([WEBKITGTK_PC_NAME])

If HTTP_STACK is hardcoded, why not just remove it? Generally we have
standardized on soup. If you want to re-add support for cURL, maybe we can
address it in another patch? I think it's much more feasible for WebKit2, if
you really need it for something.

> configure.ac:989
> +AC_CONFIG_FILES([

If posible. please indent the inside of this conditional.

> configure.ac:1008
> +AC_CONFIG_FILES([
>
+WebKit2/gtk/${WEBKITGTK_PC_NAME}-${WEBKITGTK_API_VERSION}.pc:WebKit2/gtk/webki
t2.pc.in
> +]
>
+,[WEBKITGTK_API_VERSION=$WEBKITGTK_API_VERSION,WEBKITGTK_PC_NAME=$WEBKITGTK_PC
_NAME]
> +)

Same here.