[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 #25 from Amruth Raj <amruthraj at motorola.com>  2010-10-25 07:22:56 PST ---
(In reply to comment #23)
> (From update of attachment 71620 [details])
> 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.
Yes. Will split the patch and re-submit.
> 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
Yes. Will incorporate this change.
> 
> > 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.
invalidate would be too early to send a confirmation as the handler in the WebProcess would delete the GdkPixmap and the next function in the UI Process, drawUpdateChunkIntoBackingStore would use this GdkPixmap.
> 
> > 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&)'
info().id refers to the DrawingArea id. Each DrawingArea may result in multiple UpdateChunk's. So, this id cannot be the key in this case.
I could think of another alternative(i'm not sure if that is a good idea) ie to define ID() in UpdateChunk() for all the platforms and use it wherever required.
> 
> > 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.
This is an incorrect FIXME comment that we left. Sorry for that. We just wanted to prevent WebCore/PluginView.h inclusion while compiling this file. 
> 
> > 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 :)
Yes. Thanks for pointing this out. Will correct this.
> 
> > 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.
This can be moved to ChunkedUpdateDrawingAreaGtk.cpp, however, the declarations in the .h file need to be there.

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