[webkit-reviews] review denied: [Bug 14678] [gdk] API Drafting : [Attachment 15624] Partial implementation of the API

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has denied Holger Freyther
<freyther at handhelds.org>'s request for review:
Bug 14678: [gdk] API Drafting
http://bugs.webkit.org/show_bug.cgi?id=14678

Attachment 15624: Partial implementation of the API
http://bugs.webkit.org/attachment.cgi?id=15624&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
Index: WebCore/loader/gdk/FrameLoaderClientGdk.cpp

Now that a WebKit/gtk is starting to exist, all these client implementations
should move out of WebCore and into WebKit/gtk. ChromeClientGdk.cpp is missing
from your patch but it looks like you put it in platform/gdk, which is
definitely not the right place (even through that's where some of the other
clients are at the moment).

+    WebKitGtkFrame* frame = static_cast<FrameGdk*>(m_frame)->gtkFrame();

In Mac/Windows WebKit we have a set of overloaded kit()/core() functions to do
this kind of translation between WebKit and WebCore types.

+typedef struct _WebKitGtkPage WebKitGtkPage;

You can just say `struct WebKitGtkPage` since this is C++ code.

+typedef struct _WebKitGtkFrame WebKitGtkFrame;

Ditto.

+    WebKitGtkFrame* gtkFrame() const { return m_gtkFrame; }

You shouldn't be holding onto WebKit types inside WebCore.  On Windows we get
back to the WebFrame* through the FrameLoaderClient* pointer (a little ugly,
but it keeps the separation).

I'm not going to look at the code in Api/ since I'm unfamiliar with Gtk+. It
looks like this patch is a big step forward, but r- based on
ChromeClientGdk.cpp being in platform/gdk.