Bug 98488 – [GTK] Add support for running JavaScript from GResources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Simon Pena]

This would allow to receive a path to a gresource instead of data, something
that would be useful for frequently used JavaScript which could then be embedded
in the application.

[Simon Pena]

Created attachment 167279 [details]
Patch

[Simon Pena]

Carlos: currently the resources are linked to the TestWebKitView unit test. I'll take a look at linking them to the test library, or if that fails, keeping them in a separate gresource file.

This patch makes use of the bug #98489 GRefPtr support for GBytes.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 167279 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2453:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2466:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 167279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167279&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2444
> + * then call webkit_web_view_run_javascript_finish() to get the result

webkit_web_view_run_javascript_finish() -> webkit_web_view_run_javascript_from_gresource_finish()

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2459
> +    GRefPtr<GBytes> resourceBytes = adoptGRef(g_resources_lookup_data(resource, G_RESOURCE_LOOKUP_FLAGS_NONE, &error));

g_resources_lookup_data() gets the data synchronously, since this method is asynchronous, it might block the main thread. We could use g_resources_open_stream(), read the stream asynchronously and then pass the resulting data to runJavaScriptInMainFrame() to complete the operation.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2478
> + * This is an example of using webkit_web_view_run_javascript_from_gresource() with a script returning
> + * a number:

No need to repeat the example here, just point to webkit_web_view_run_javascript_finish() for more details.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:334
> +WEBKIT_API WebKitJavascriptResult *
> +webkit_web_view_run_javascript_from_gresource_finish(WebKitWebView             *web_view,
> +                                                     GAsyncResult              *result,
> +                                                     GError                   **error);

leave a space between function name an ( and line all other methods

> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:64
> +DerivedSources/WebKit2/webkit2gtk-tests-resources.h: Source/WebKit2/UIProcess/API/gtk/tests/resources/webkit2gtk-tests.gresource.xml
> +	$(AM_V_GEN) $(GLIB_COMPILE_RESOURCES) --target=$@ --generate-header --sourcedir=$(srcdir) $<

I don't think we need the header

[Simon Pena]

Created attachment 167545 [details]
Patch

[Simon Pena]

Comment on attachment 167545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2446
> +    gpointer outputStreamData = g_memory_output_stream_get_data(G_MEMORY_OUTPUT_STREAM(outputStream));

I extracted the outputStream data into a variable for clarity. I can simply put the g_memory_output_stream_get_data in the reinterpret_cast, but that would make the line "way longer than 80" characters...

> 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?

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

[Simon Pena]

Comment on attachment 167545 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review

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

[WebKit Review Bot]

Attachment 167545 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:37:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2487:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2488:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2489:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

(In reply to comment #8)
> (From update of attachment 167545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review
> 
> > 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)?

It's a new rule in the style checker, so it's probably best to just align our style to it.

[Martin Robinson]

(In reply to comment #7)
> (From update of attachment 167545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2446
> > +    gpointer outputStreamData = g_memory_output_stream_get_data(G_MEMORY_OUTPUT_STREAM(outputStream));
> 
> I extracted the outputStream data into a variable for clarity. I can simply put the g_memory_output_stream_get_data in the reinterpret_cast, but that would make the line "way longer than 80" characters...

Not saying what you did was wrong (it's probably a good idea), but lines in WebKit typically span well beyond 120 characters. There's no hard rule about when to break a line, but some people start breaking them at 120.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 167545 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=167545&action=review
> > 
> > > 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)?
> 
> 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.

[Carlos Garcia Campos]

Comment on attachment 167545 [details]
Patch

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/webkit2gtk-tests.test.js</file>

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

[Simon Pena]

Created attachment 167730 [details]
Patch

[WebKit Review Bot]

Attachment 167730 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2487:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2488:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2489:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 167730 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167730&action=review

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:33
> +    resource = g_resource_load(resourcesPath.get(), 0);

Sorry, I didn't noticed this before, you could move the variable declaration here:

GResource* resource = g_resource_load(resourcesPath.get(), 0);

> Source/WebKit2/UIProcess/API/gtk/tests/TestMain.cpp:34
> +

Remove this empty line.

[Simon Pena]

Created attachment 167743 [details]
Patch

[WebKit Review Bot]

Attachment 167743 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h"
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2448:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2472:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2487:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2488:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2489:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 167743 [details]
Patch

Excellent!

[WebKit Review Bot]

Comment on attachment 167743 [details]
Patch

Rejecting attachment 167743 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14211752

[Carlos Garcia Campos]

(In reply to comment #20)
> (From update of attachment 167743 [details])
> Rejecting attachment 167743 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/14211752

We haven't changed LayoutTests/ChangeLog, let's try again.

[WebKit Review Bot]

Comment on attachment 167743 [details]
Patch

Clearing flags on attachment: 167743

Committed r130755: <http://trac.webkit.org/changeset/130755>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.