[Webkit-unassigned] [Bug 37369] [GTK] Enable building whatever already exists of WebKit2

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37369





--- Comment #23 from Balazs Kelemen <kbalazs at webkit.org>  2010-10-25 03:04:17 PST ---
(From update of attachment 71620)
View in context: https://bugs.webkit.org/attachment.cgi?id=71620&action=review

Generally the patch looks very promising for me, however I do not know much about GTK and I am not a reviewer.
I think the MiniBrowser part should be separated to another patch, this is quite big even without that.
The main problem I see is that there are too many ifdefs in common code. Those should be avoided somehow.

> WebCore/platform/network/soup/cache/webkit/soup-cache.h:7
>   *

I have never saw the expressions "Portions Copyright" or any prefix before "all rights reserved" in WebKit before, so maybe
you should omit those for convenience.

> WebKit2/Platform/Module.h:69
>  

I do not think you should add copyright for that amount of changes, but of course it is your decision.

> WebKit2/Platform/CoreIPC/gtk/ConnectionGtk.cpp:97
> +    return true;

this should be "return m_socket > 0" as I see

> WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:139
> +#if PLATFORM(GTK)
> +    page->process()->send(DrawingAreaMessage::DidSetSizeDone, page->pageID(), CoreIPC::In(info().id, updateChunk->xID()));
> +#endif

We should avoid ifdefs in common code. You can send this message from invalidateBackingStore which is a platform method.

> WebKit2/UIProcess/ChunkedUpdateDrawingAreaProxy.cpp:160
> +#if PLATFORM(GTK)
> +    page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id, updateChunk->xID()));
> +#else
>      page->process()->send(DrawingAreaMessage::DidUpdate, page->pageID(), CoreIPC::In(info().id));
> +#endif

We should get rid of the ifdefs somehow. Is there a way to avoid the gtk specialization of didUpdate? I see the reason of
that but maybe you can do it in another way. One think that had been came to my mind is to use info().id for the key in
xIDMap and introduce a platform method that would be called from ChunkedUpdateArea::didUpdate, for example
'void platformDidUpdate(const DrawingAreaInfo&)'

> WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:67
> +// FIXME: Including "PluginView.h" at current location was re-defining the enum CodePath { Complex } to 0 in WebCore/platform/graphics/Font.h.
> +// The re-define is happening because of a macro COMPLEX in Xlib.h. Hence, moved the inclusion to the end of list for compilation on GTK platform.
> +#include <WebKit2/PluginView.h>
>  

Please try to fix this. I had similar problems that could be solved by including npruntime_internal.h instead of npruntime.h
in some files. You should precompile it and find where the macro is coming from.

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:102
> +#if PLATFORM(GTK)
> +    xIDMap.set(updateChunk.xID(), updateChunk.memory()).second;
> +#endif

why '.second'? I am assuming this has been left here accidentally :)

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:152
> +#if PLATFORM(GTK)
> +    xIDMap.set(updateChunk.xID(), updateChunk.memory()).second;
> +#endif

As I sad above, this should be avoided.

> WebKit2/WebProcess/WebPage/ChunkedUpdateDrawingArea.cpp:195
> +#if PLATFORM(GTK)
> +void ChunkedUpdateDrawingArea::didUpdate(uint32_t xID)
> +{
> +    m_isWaitingForUpdate = false;
> +    freeBitMapMemory(xID);
> +    // Display if needed.
> +    display();
> +}
> +
> +void ChunkedUpdateDrawingArea::didSetSizeDone(uint32_t xID)
> +{
> +    freeBitMapMemory(xID);
> +}
> +#endif

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.