[webkit-reviews] review denied: [Bug 98488] [GTK] Add support for running JavaScript from GResources : [Attachment 167545] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Simon Pena
<spena at igalia.com>'s request for review:
Bug 98488: [GTK] Add support for running JavaScript from GResources
https://bugs.webkit.org/show_bug.cgi?id=98488

Attachment 167545: Patch
https://bugs.webkit.org/attachment.cgi?id=167545&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review


This looks pretty good, we only need to fix a few minor things.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2432
> +static void resourcesStreamReadCallback(GObject* object, GAsyncResult*
result, gpointer data)

Use userData here instead of data to avoid confusion with the async data.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2441
> +	   g_simple_async_result_complete_in_idle(runJavascriptResult.get());

You can complete the operation directly here instead of in an idle, since we
are in a callback in the main thread already.

>>>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2487
>>>>> + 				(GOutputStreamSpliceFlags)
(G_OUTPUT_STREAM_SPLICE_CLOSE_SOURCE | G_OUTPUT_STREAM_SPLICE_CLOSE_TARGET),
>>>> 
>>>> Also, the style checker complains here and in the TestMain.cpp
indentation. Should I file a bug for that? Or is there one already (since this
indentation should be quite common in the Gtk port)?
>>> 
>>> Weird number of spaces at line-start.  Are you using a 4-space indent? 
[whitespace/indent] [3]
>> 
>> It's a new rule in the style checker, so it's probably best to just align
our style to it.
> 
> I think we should add at least the files in our API as exceptions to that
rule in the style checker.

Use C++ style cast for this.

>> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:63
>> +noinst_DATA += DerivedSources/WebKit2/webkit2gtk-tests-resources.gresource
> 
> I wonder if I have to take any measure to keep the tests working when WebKit
is distributed in a tarball?

I wonder how this can work, because noinst_DATA doesn't seem to be defined. You
need to make sure the generated files are cleaned. Add DISTCLEANFILES to clean
the resource bundle.

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:28
> +static void register_gresource(void)

static void registerGResource()

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:31
> +    GError* error = 0;

Use GOwnPtr for the error, or simply ignore the error, since we are going to
assert if we fail to load the resources.

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:33
> +    GOwnPtr<char> resourcesPath(g_build_filename(WEBKIT_DERIVED_SRC_DIR,
"WebKit2", "webkit2gtk-tests-resources.gresource", 0));

You can't use 0 in this case because NULL is a sentinel.

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:36
> +    if (!resource) {
> +	   g_warning("Could not load resource
webkit2gtk-tests-resources.gresource: %s\n",

This should never fail, so add a g_assert instead.

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:51
> +    register_gresource();

registerGResource()

>> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebView.cpp:651
>> +
> 
> I guess I should test that trying to run javascript from a non-existing
resource fails with a GError, too.

Yes, we could check that too.

>
Source/WebKit2/UIProcess/API/gtk/tests/resources/webkit2gtk-tests.gresource.xml
:4
> +    <file
alias="wk2gtk-test.js">Source/WebKit2/UIProcess/API/gtk/tests/resources/webkit2
gtk-tests.test.js</file>

Why not simply call the file test.js? or even link-title.js