[Webkit-unassigned] [Bug 98705] [EFL][WK2] Add Remote Web Inspector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=98705





--- Comment #20 from Seokju Kwon <seokju.kwon at gmail.com>  2012-12-08 05:55:59 PST ---
(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)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.