Bug 98705 – [EFL][WK2] Add Remote Web Inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Seokju Kwon]

Implementation of remove web inspector on efl port.

[Seokju Kwon]

Implementation of remote web inspector on efl port.

[Anton Obzhirov]

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?

[Seokju Kwon]

Created attachment 175750 [details]
Patch

[Gyuyoung Kim]

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.

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 177479 [details]
Patch

[Kenneth Rohde Christiansen]

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?

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 177735 [details]
Patch

[Gyuyoung Kim]

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 ?

[Seokju Kwon]

(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).

[Ryuan Choi]

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);

':' ?

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 177992 [details]
Patch

[Gyuyoung Kim]

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.

[Seokju Kwon]

(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.

[Seokju Kwon]

Created attachment 178327 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 178327 [details]
Patch

LGTM. But, someone else might want to have a final look.

[Chris Dumez]

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"

[Seokju Kwon]

(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)

[Seokju Kwon]

Created attachment 178350 [details]
Patch

[Chris Dumez]

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

[Seokju Kwon]

Created attachment 178354 [details]
Patch

[Chris Dumez]

Comment on attachment 178354 [details]
Patch

LGTM. cq=me

[WebKit Review Bot]

Comment on attachment 178354 [details]
Patch

Clearing flags on attachment: 178354

Committed r137035: <http://trac.webkit.org/changeset/137035>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Seokju, could you please document how to enable and use the functionality on our wiki?
Thanks!