[webkit-reviews] review denied: [Bug 48511] [GTK] Implement Process/Thread Launcher classes and WebProcessGtkMain for WebKit2 : [Attachment 72686] ProcessLauncher and ThreadLauncher implementation for WebKit2 GTK port
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Dec 30 09:35:02 PST 2010
Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 48511: [GTK] Implement Process/Thread Launcher classes and
WebProcessGtkMain for WebKit2
https://bugs.webkit.org/show_bug.cgi?id=48511
Attachment 72686: ProcessLauncher and ThreadLauncher implementation for WebKit2
GTK port
https://bugs.webkit.org/attachment.cgi?id=72686&action=review
------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72686&action=review
Looks good! Some comments below.
> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:55
> + String commandLine(WEBKIT_WEB_PROCESS_PATH);
I don't understand why you are using a String here. Why not just use
WEBKIT_WEB_PROCESS_PATH below?
> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:68
> + ASSERT_NOT_REACHED();
> + }
I think it's better to try to handle the error here somehow, even if it's just
printing a message.
> WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:81
> + notImplemented();
Is this not implemented or simply an empty implementation?
> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:43
> + // FIXME: Implement.
This looks like a good place for notImplemented() :) I think this file is also
missing stubs for platformShutdown and platformClearResourceCaches.
> WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:54
> + g_type_init();
Do you also need to call g_thread_init() here? Only after 2.24 does
g_type_init() call g_thread_init().
More information about the webkit-reviews
mailing list