[Webkit-unassigned] [Bug 88094] Web Inspector: Add a WebInspectorServer on Linux using the GSocket API for the GTK port
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 11 04:46:31 PDT 2012
https://bugs.webkit.org/show_bug.cgi?id=88094
Carlos Garcia Campos <cgarcia at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #163316|review? |review-
Flag| |
--- Comment #17 from Carlos Garcia Campos <cgarcia at igalia.com> 2012-09-11 04:46:53 PST ---
(From update of attachment 163316)
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_socketConnection.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?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list