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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jul 22 17:53:58 PDT 2007

Adam Roben <aroben at apple.com> has granted Holger Freyther
<freyther at handhelds.org>'s request for review:
Bug 14678: [gdk] API Drafting

Attachment 15629: Partial implementation of the API

------- 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),
+    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

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

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

More information about the webkit-reviews mailing list