[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