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

[bugzilla-daemon at webkit.org]

Adam Roben <aroben at apple.com> has granted 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 15629: Partial implementation of the API
http://bugs.webkit.org/attachment.cgi?id=15629&action=edit

------- Additional Comments from Adam Roben <aroben at apple.com>
+    EditorClientGdk* editorClient = new EditorClientGdk;
+    private_data->page = new Page(new ChromeClientGdk(page), new
ContextMenuClientGdk, editorClient, 0, new InspectorClientGdk);
+    editorClient->setPage(private_data->page);

Seems like EditorClientGdk should take a WebKitGtkPage* argument in its
constructor. Just a FIXME is good enough for now.

+     * FIXME: Will be different with multiple frames

It would be good to say what/how/why this will be different so that you don't
forget it and other people understand.

+    PassRefPtr<SharedBuffer> sharedBuffer = new SharedBuffer(strdup(content),
strlen(content));    
+    SubstituteData substituteData(sharedBuffer, String(content_mime_type),
String(content_encoding), KURL("about:blank"), url);

I believe the correct idiom here is to make sharedBuffer a RefPtr (not
PassRefPtr), then pass `sharedBuffer.release()` into the SubstituteData
constructor.

Nothing in WebKit/gtk should be in the WebCore namespace (though I realize that
right now many things are in WebCore that should be in WebKit/gtk). I think you
can move everything in webkitgtkprivate.cpp out of the WebCore namespace. Would
be good to move ChromeClientGdk, too, though that could be a separate patch.

+FloatRect ChromeClientGdk::windowRect() {

+bool ChromeClientGdk::scrollbarsVisible() {

Brace should be on the next line.

+float ChromeClientGdk::scaleFactor()
+{
+    notImplemented();
+    return 1.0;
+}

Should be `1.0f` to avoid double -> float conversion.

+    if (!cresult)
+	 return false;
+    else {
+	 result = UTF8Encoding().decode(cresult, strlen(cresult));
+	 g_free(cresult);
+	 return true;
+    }

Don't need to say `else` after the return.

+    for my $i (0 .. $#ARGV) {
+	 my $opt = $ARGV[$i];

Since you don't use $i anywhere else, you should just do:

foreach my $opt (@ARGV) {
}

r=me, though I think it would be nice to get someone with GTK+ experience to
have a look at it (maybe Alp Toker?). You should also keep in mind what Maciej
said about the benefits of keeping the API relatively close to the Mac/Windows
API.

It's great to see all those WebCore headers gone from GdkLauncher!