[webkit-reviews] review denied: [Bug 88094] Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port : [Attachment 163316] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 11 04:46:29 PDT 2012
Carlos Garcia Campos <cgarcia at igalia.com> has denied Anton Obzhirov
<a.obzhirov at samsung.com>'s request for review:
Bug 88094: Web Inspector: Add a WebInspectorServer on Linux using the GSocket
API for the GTK port
https://bugs.webkit.org/show_bug.cgi?id=88094
Attachment 163316: Patch
https://bugs.webkit.org/attachment.cgi?id=163316&action=review
------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=163316&action=review
I don't know much about the inspector nor the inspector server, setting r-
because of the memory leaks, styles issues, etc.
> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:103
> + m_socketConnection = adoptGRef(socketConnection);
You should not adopt the ref, but take a reference.
> Source/WebCore/platform/network/soup/SocketStreamHandleSoup.cpp:116
> + m_outputStream =
G_POLLABLE_OUTPUT_STREAM(g_io_stream_get_output_stream(G_IO_STREAM(m_socketConn
ection.get())));
> + m_inputStream =
g_io_stream_get_input_stream(G_IO_STREAM(m_socketConnection.get()));
> + m_readBuffer = new char[READ_BUFFER_SIZE];
> +
> + g_input_stream_read_async(m_inputStream.get(), m_readBuffer,
READ_BUFFER_SIZE, G_PRIORITY_DEFAULT, 0,
> + reinterpret_cast<GAsyncReadyCallback>(readReadyCallback), m_id);
> +
> + m_state = Open;
> +
> + if (!m_client)
> + return;
> +
> + m_client->didOpenSocketStream(this);
Instead of duplicating all this code, since the socket is already connected
this could be just:
m_id = activateHandle(this);
connected(socketConnection, 0);
For this to work, we would also need to fix connectedCallback() to use GRefPtr
and adopGRef for the socket connection (note that it's leaked in case of
error), and take a ref instead of adopting it in connected().
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:51
> + GRefPtr<GFile> file(g_file_new_for_path(localPath.latin1().data()));
Use adoptGRef() to avoid leaking the GFile. GFile expects UTF-8 so you should
use localPath.utf8().data() instead of latin1
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:54
> + if (file) {
g_file_new always returns a valid pointer, so you don't need this check
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:56
> + GRefPtr<GFileInfo> fileInfo(g_file_query_info(file.get(),
G_FILE_ATTRIBUTE_STANDARD_SIZE, G_FILE_QUERY_INFO_NONE, NULL,
&error.outPtr()));
Use adoptGRef here too. Use 0 instead of NULL. Also since you are not handling
the error, you could pass 0, and check the return value, returning if fileInfo
is NULL.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:61
> + GRefPtr<GFileInputStream> inputStream(g_file_read(file.get(), NULL,
&error.outPtr()));
You should use adopGRef here too, use 0 instad of NULL, and pass 0 for the
error too since you are not handling the error.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:66
> + size = g_file_info_get_size(fileInfo.get());
> + data.grow(size);
This could be a single line:
data.grow(g_file_info_get_size(fileInfo.get()));
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:70
> + gsize bytesRead = 0;
> + if (!g_input_stream_read_all(G_INPUT_STREAM(inputStream.get()),
data.data(), data.size(), &bytesRead, 0, 0))
> + return false;
bytesRead is unused, you could simply pass NULL to g_input_stream_read_all
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebInspectorServerGtk.cpp:74
> + size_t extStart = localPath.reverseFind('.');
> + String ext = localPath.substring(extStart != notFound ? extStart + 1
: 0);
> + contentType =
WebCore::MIMETypeRegistry::getMIMETypeForExtension(ext);
Instead of this, you could pass G_FILE_ATTRIBUTE_STANDARD_FAST_CONTENT_TYPE to
g_file_query_info and get the mime type from the GFileInfo with
g_file_info_get_content_type().
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:55
> + WebSocketServer* server = (WebSocketServer*)self;
Use C++ style casts. In this case we can simply avoid the cast, since the
callback is already "casted", you can pass WebSocketServer* server as parameter
instead of gpointer self
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:57
> + // reference connection before accepting it
> + g_object_ref_sink(connection);
Don't do this, pass the connection to SocketStreamHandle::create() and the
constructor will tak a reference.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:69
> + m_socketService = adoptGRef(g_socket_service_new());
> + if (m_socketService.get()) {
g_socket_service_new() always returns a valid pointer.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:72
> + if (!g_socket_service_is_active(m_socketService.get()))
> + g_socket_service_start(m_socketService.get());
g_socket_service_start already checks whether the service is active or not, so
you can call g_socket_service_start(m_socketService.get()); directly.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:86
> + GSocketService* socketService = m_socketService.get();
> + if (!socketService) {
> + LOG_ERROR("Failed to get socket service.");
> + return false;
> + }
I think this shouldn't happen, if you want to be extra sure, add an assert
ASSERT(socketService);
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:89
> + GInetAddress* address =
g_inet_address_new_from_string(bindAddress.latin1().data());
> + GSocketAddress* socketAddress = g_inet_socket_address_new(address,
port);
Use GRefPtr and adoptGRef in both cases. Use bindAddress.utf8().data() instead
of latin1 too.
> Source/WebKit2/UIProcess/InspectorServer/gtk/WebSocketServerGtk.cpp:105
> + gboolean result =
g_socket_listener_add_address(G_SOCKET_LISTENER(socketService),
> + socketAddress,
> + G_SOCKET_TYPE_STREAM,
> + G_SOCKET_PROTOCOL_TCP, 0, 0, 0);
> +
> + if (!result)
> + LOG_ERROR("Failed to add inet address to web inspector server
listener.");
> +
> + g_object_unref(socketAddress);
> + g_object_unref(address);
> +
> + if (!result)
> + return false;
> +
> + return true;
All this could be just
return g_socket_listener_add_address(G_SOCKET_LISTENER(m_socketService.get()),
socketAddress.get(), G_SOCKET_TYPE_STREAM, G_SOCKET_PROTOCOL_TCP, 0, 0, 0);
> Source/WebKit2/UIProcess/InspectorServer/qt/WebInspectorServerQt.cpp:50
> - String localPath = (path == "/") ?
"/webkit/resources/inspectorPageIndex.html" : path;
> + String localPath = (path == "/") ?
"/webkit/inspector/inspectorPageIndex.html" : path;
Is this a bug in Qt implementation or something that has changed in this patch?
> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:66
> + initialized = WebInspectorServer::shared().listen(bindAddress, port);
Do we want this even if the env variable is not present?
> Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:78
> + initInspectorServer();
How is the inspector server run? does it run in the UI process?
More information about the webkit-reviews
mailing list