Implementation of remove web inspector on efl port.
Implementation of remote web inspector on efl port.
I guess if libsoup is used for EFL as well GTK Inspector Server can be re-factored to become Soup Inspector server or something like that. It is a bit mixing of the terms because GTK Inspector Server uses Glib (GSocket) interface but here you are. What do you think guys?
Created attachment 175750 [details] Patch
As Anton said, I think we should consider if we can share GTK implementation first. It seems to me that this patch is almost same with GTK implementation. For example, you might to change WebInspectorServerGtk with WebInspectorServerSoup.cpp to share it.
(In reply to comment #4) > As Anton said, I think we should consider if we can share GTK implementation first. It seems to me that this patch is almost same with GTK implementation. > > For example, you might to change WebInspectorServerGtk with WebInspectorServerSoup.cpp to share it. It is an initial patch for efl port. The patch sharing the server with gtk will be uploaded.
Created attachment 177479 [details] Patch
Comment on attachment 177479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177479&action=review > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:85 > + ClientMap::iterator end = m_clientMap.end(); newline before this > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:100 > + } newline after this > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:47 > + initialized = true; It doesnt seem initialized at this point, what if the below fails?
(In reply to comment #7) > (From update of attachment 177479 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177479&action=review > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:85 > > + ClientMap::iterator end = m_clientMap.end(); > > newline before this > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:100 > > + } > > newline after this > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:47 > > + initialized = true; > > It doesnt seem initialized at this point, what if the below fails? Right. I will fix them.
Created attachment 177735 [details] Patch
Comment on attachment 177479 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177479&action=review >> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:47 >> + initialized = true; > > It doesnt seem initialized at this point, what if the below fails? In Gtk port, it looks they wanted to initialize this variable only once. I think they used this as guard pointer to prevent to call more than once. > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:61 > + initialized = WebInspectorServer::shared().listen(bindAddress, port); initialized is to ensure initInspectorServer() is executed only once, so it should set to true always, even if the env var is not present or the server fails to run. https://bugs.webkit.org/show_bug.cgi?id=88094#c25 Don't initInspectorServer() be called only once ?
(In reply to comment #10) > (From update of attachment 177479 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177479&action=review > > >> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:47 > >> + initialized = true; > > > > It doesnt seem initialized at this point, what if the below fails? > > In Gtk port, it looks they wanted to initialize this variable only once. I think they used this as guard pointer to prevent to call more than once. > > > Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp:61 > > + initialized = WebInspectorServer::shared().listen(bindAddress, port); > initialized is to ensure initInspectorServer() is executed only once, so it should set to true always, even if the env var is not present or the server fails to run. > https://bugs.webkit.org/show_bug.cgi?id=88094#c25 > > Don't initInspectorServer() be called only once ? Sound reasonable. Qt and GTK port are same (initInspectorServer() is executed only once).
Comment on attachment 177735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177735&action=review > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:42 > +static String inspectorResourcePath() > +{ > + String inspectorFilesPath = WEB_INSPECTOR_INSTALL_DIR; > + if (access(inspectorFilesPath.utf8().data(), R_OK)) > + inspectorFilesPath = WEB_INSPECTOR_DIR; > + > + return inspectorFilesPath; > +} IMHO, how about moving this into WebCore/platform/efl to reduce duplication? For example, EflInspectorUtilities.h? In addition, don't we return "file://" + inspectorFilesPath ? > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:53 > + String localPath = inspectorResourcePath() + ((path == "/") ? "/inspectorPageIndex.html" : path); Can we improve this line using https://trac.webkit.org/wiki/EfficientStrings ? > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:54 > + if (localPath.isNull()) I am wonering how localPath can be null > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:98 > + builder.appendLiteral("\", \"inspectorUrl\": \""); > + builder.appendLiteral("/inspector.html?page="); As one line? > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:50 > + String bindAddress = "127.0.0.1"; ASCIILiteral("...") > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:54 > + serverAddress.split(":", result); ':' ?
(In reply to comment #12) > (From update of attachment 177735 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177735&action=review > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:42 > > +static String inspectorResourcePath() > > +{ > > + String inspectorFilesPath = WEB_INSPECTOR_INSTALL_DIR; > > + if (access(inspectorFilesPath.utf8().data(), R_OK)) > > + inspectorFilesPath = WEB_INSPECTOR_DIR; > > + > > + return inspectorFilesPath; > > +} > > IMHO, how about moving this into WebCore/platform/efl to reduce duplication? > > For example, EflInspectorUtilities.h? > > In addition, don't we return "file://" + inspectorFilesPath ? > Good proposal. I will file a bug for it later. we don't need "file://" for argument of file APIs. > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:53 > > + String localPath = inspectorResourcePath() + ((path == "/") ? "/inspectorPageIndex.html" : path); > > Can we improve this line using https://trac.webkit.org/wiki/EfficientStrings ? > Well, QT and GTK ports use it like this. I want to use it for consistency, if it's not bad. Could you please let me know how to imprvoe if you have it? > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:54 > > + if (localPath.isNull()) > > I am wonering how localPath can be null > Right. Unnecessary code. > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:98 > > + builder.appendLiteral("\", \"inspectorUrl\": \""); > > + builder.appendLiteral("/inspector.html?page="); > > As one line? > IMO, It seems to make it more readable. > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:50 > > + String bindAddress = "127.0.0.1"; > > ASCIILiteral("...") > Ok. > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:54 > > + serverAddress.split(":", result); > > ':' ? Ok.
Created attachment 177992 [details] Patch
Comment on attachment 177992 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177992&action=review > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:53 > + String localPath = inspectorResourcePath() + ((path == "/") ? "/inspectorPageIndex.html" : path); At least, it looks you can use ASCIILiteral("/inspectorPageIndex.html") instead of "/inspectorPageIndex.html". > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:46 > + It would be good if you add a comment why *initialized* should be true.
(In reply to comment #15) > (From update of attachment 177992 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177992&action=review > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:53 > > + String localPath = inspectorResourcePath() + ((path == "/") ? "/inspectorPageIndex.html" : path); > > At least, it looks you can use ASCIILiteral("/inspectorPageIndex.html") instead of "/inspectorPageIndex.html". > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:46 > > + > > It would be good if you add a comment why *initialized* should be true. All done.
Created attachment 178327 [details] Patch
Comment on attachment 178327 [details] Patch LGTM. But, someone else might want to have a final look.
Comment on attachment 178327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178327&action=review I think this is a nice functionality to have, thanks for the patch. I have a few minor comments before the patch lands though. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:37 > + String inspectorFilesPath = WEB_INSPECTOR_INSTALL_DIR; You should use String::fromUTF8() here in case there are non-ascii characters in the path. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:39 > + inspectorFilesPath = WEB_INSPECTOR_DIR; Ditto. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:69 > + if (bytesRead <= 0) We could even check (bytesRead < fileStat.st_size) I believe. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:73 > + String ext = localPath.substring(extStart != notFound ? extStart + 1 : 0); Looks to me that it is going to return the whole path when '.' cannot be found which is probably not what you want. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:74 > + contentType = WebCore::MIMETypeRegistry::getMIMETypeForExtension(ext); Shouldn't we handle the case where there is no file extension? The EFL implementation of getMIMETypeForExtension() does not check for empty extension string and will iterate over the extension array for no reason. We should probably avoid calling it uselessly. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:82 > + builder.appendLiteral("[ "); We don't technically need the space here, right? How about append('[') ? Same comments for the code below. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:102 > + CString cstr = builder.toString().utf8(); You should avoid abbreviations in variable names. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:104 > + contentType = "application/json; charset=utf-8"; contentType = String("application/json; charset=utf-8", ConstructFromLiteral); ? > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:40 > +static void initInspectorServer() We usually avoid abbreviations in WebKit. init -> initialize ? > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:62 > + port = result[1].toInt(&ok); toUInt() would be slightly better. > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:65 > + LOG_ERROR("Couldn't parse the port. Use 2999 instead."); "Use" -> "Using" > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:68 > + LOG_ERROR("Couldn't parse %s, wrong format? Use 127.0.0.1:2999 instead.", serverAddress.utf8().data()); "Using"
(In reply to comment #19) > (From update of attachment 178327 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178327&action=review > > I think this is a nice functionality to have, thanks for the patch. I have a few minor comments before the patch lands though. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:37 > > + String inspectorFilesPath = WEB_INSPECTOR_INSTALL_DIR; > > You should use String::fromUTF8() here in case there are non-ascii characters in the path. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:39 > > + inspectorFilesPath = WEB_INSPECTOR_DIR; > > Ditto. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:69 > > + if (bytesRead <= 0) > > We could even check (bytesRead < fileStat.st_size) I believe. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:73 > > + String ext = localPath.substring(extStart != notFound ? extStart + 1 : 0); > > Looks to me that it is going to return the whole path when '.' cannot be found which is probably not what you want. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:74 > > + contentType = WebCore::MIMETypeRegistry::getMIMETypeForExtension(ext); > > Shouldn't we handle the case where there is no file extension? The EFL implementation of getMIMETypeForExtension() does not check for empty extension string and will iterate over the extension array for no reason. We should probably avoid calling it uselessly. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:82 > > + builder.appendLiteral("[ "); > > We don't technically need the space here, right? How about append('[') ? Same comments for the code below. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:102 > > + CString cstr = builder.toString().utf8(); > > You should avoid abbreviations in variable names. > > > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:104 > > + contentType = "application/json; charset=utf-8"; > > contentType = String("application/json; charset=utf-8", ConstructFromLiteral); ? > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:40 > > +static void initInspectorServer() > > We usually avoid abbreviations in WebKit. init -> initialize ? > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:62 > > + port = result[1].toInt(&ok); > > toUInt() would be slightly better. > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:65 > > + LOG_ERROR("Couldn't parse the port. Use 2999 instead."); > > "Use" -> "Using" > > > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:68 > > + LOG_ERROR("Couldn't parse %s, wrong format? Use 127.0.0.1:2999 instead.", serverAddress.utf8().data()); > > "Using" All done. (All resources for remote web inspector : *.js|html|css|gif|png, refer to Source/PlatformEfl.cmake)
Created attachment 178350 [details] Patch
Comment on attachment 178350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178350&action=review > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:73 > + String ext = localPath.substring(extStart != notFound ? extStart + 1 : 0); As I commented before: substring(0) will return the whole string, which is not what you want if there is no file extension. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:74 > + contentType = WebCore::MIMETypeRegistry::getMIMETypeForExtension(ext); Also commented before, we should probably check if ext is empty before calling getMIMETypeForExtension() since our implementation of getMIMETypeForExtension() does not. > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:82 > + builder.appendLiteral("["); append('[') is better > Source/WebKit2/UIProcess/InspectorServer/efl/WebInspectorServerEfl.cpp:101 > + builder.appendLiteral("]"); Ditto
Created attachment 178354 [details] Patch
Comment on attachment 178354 [details] Patch LGTM. cq=me
Comment on attachment 178354 [details] Patch Clearing flags on attachment: 178354 Committed r137035: <http://trac.webkit.org/changeset/137035>
All reviewed patches have been landed. Closing bug.
Seokju, could you please document how to enable and use the functionality on our wiki? Thanks!