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.
Created attachment 167279 [details] Patch
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.
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
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.
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
Created attachment 167545 [details] Patch
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.
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)?
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.
(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.
(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.
(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.
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
Created attachment 167730 [details] Patch
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.
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.
Created attachment 167743 [details] Patch
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.
Comment on attachment 167743 [details] Patch Excellent!
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
(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.
Comment on attachment 167743 [details] Patch Clearing flags on attachment: 167743 Committed r130755: <http://trac.webkit.org/changeset/130755>
All reviewed patches have been landed. Closing bug.