Bug 142786 – [GTK] webkit_web_extension_initialize[_with_user_data] not documented

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philip Chimento]

On this page [1] the function types WebKitWebExtensionInitializeFunction and WebKitWebExtensionInitializeWithUserDataFunction are documented, but nowhere does it state that those functions have specific names that webkit will look for when loading the web extension: webkit_web_extension_initialize and webkit_web_extension_initialize_with_user_data. These should be mentioned in the API documentation, perhaps even with a small howto for web extensions. Currently this information is only available by searching people's blogs or looking in the source code.

[1] http://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebExtension.html

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 249819 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

I have create some documentation for the WebKitWebExtension class so new user can easily found out how to use it. Please review this to check if there is something wrong or missing.

[Adrian Perez]

Comment on attachment 249819 [details]
Patch

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

(Reviewing informally…)

Thanks for the patch! I like the idea of having this documentation on how to make WebExtensions with the example code in the official documentation. So, it would be nice to land this patch after fixing a couple of nits.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:41
> + * #WebKitWebExtension works as a web plugin for the WebProcess. It allow you to execute code

I would just write “#WebKitWebExtensions is a plugin for the WebProcess” because calling it a “web plugin” might sounds misleading (people could understand that is a plugin made using web technologies, for example).

s/It allow/It allows/

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:45
> + * WebExtensions are a single compilation unit that are loaded similar to a GTK module.

s/loaded similar to/loaded in a way similar to/

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:52
> + *

Could we mark names of functions and macros with monospaced font? Provided we are using a recent-enough gtk-doc (which I think we are), it's just a matter of enclosing things in backticks, e.g.: `G_MODULE_EXPORT`.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:78
> + * WebKit has to know where it can to found the created #WebKitWebExtension. To do so you

s/where it can to found/where it can find/

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 249824 [details]
Patch

[Adrian Perez]

Comment on attachment 249824 [details]
Patch

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

(Again, reviewing informally…)

Thanks for the updated patch. The new version looks good to me, with just a very small nit. We'll need now a reviewer to rubber-stamp the changes, and get the patch landed ☺

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:43
> + * Javascript code, for example.

Nit: capitalization → s/Javascript/JavaScript/

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 249832 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

> (Again, reviewing informally…)

Thank you for the quick review!! :))

[Build Bot]

Comment on attachment 249832 [details]
Patch

Attachment 249832 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6057621261910016

New failing tests:
media/invalid-media-url-crash.html

[Build Bot]

Created attachment 249835 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 249832 [details]
Patch

Attachment 249832 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5481830966886400

New failing tests:
media/invalid-media-url-crash.html

[Build Bot]

Created attachment 249838 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 250778 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 250778 [details]
Patch

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

> Source/WebKit2/ChangeLog:8
> +        WebKitWebExtension has no documentation at all the only information

no documentation at all is at least inaccurate, WebKitWebExtension has a signal and a public method and both are documented. Initialization functions are also documented in WebKitWebExtension.h

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:37
> + * SECTION: WebKitWebExtension

So, the section documentation is what is missing then.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:41
> + * #WebKitWebExtension is a plugin for the WebProcess. It allows you to execute code in the

Do not use # here, since this creates a link to this point exactly. I would avoid using 'plugin' here, since it could be confused with browser plugins. I would use loadable module, for example.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:45
> + * WebExtensions are a single compilation unit that are loaded in a way similar to a GTK module.

I wouldn't mention GTK modules here either, I think previous paragraph already explained that this is loadable module loaded by the web process.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:46
> + * To create a #WebKitWebExtension you should write a module which implements a function

Do not use # here.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:47
> + * named `webkit_web_extension_initialize` or, in case you need to initilize the extension with

Use webkit_web_extension_initialize() instead of the quotes

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:48
> + * some data from the UI, `webkit_web_extension_initialize_with_user_data`. These functions

webkit_web_extension_initialize_with_user_data()

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:49
> + * types are WebKitWebExtensionInitializeFunction() and WebKitWebExtensionInitializeWithUserDataFunction()

WebKitWebExtensionInitializeFunction and WebKitWebExtensionInitializeWithUserDataFunction are not functions, use # instead of ().

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:50
> + * respectively. In addition the implemented function has to be public and it has to use

I would rephrase this a bit, something like. Web extensions must have a initialization function that could be either webkit_web_extension_initialize() with prototype #WebKitWebExtensionInitializeFunction or webkit_web_extension_initialize_with_user_data() with prototype #WebKitWebExtensionInitializeWithUserDataFunction. This function is called when the web process is initialized.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:51
> + * the `G_MODULE_EXPORT` macro.

#G_MODULE_EXPORT

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:71
> + * G_MODULE_EXPORT void
> + * webkit_web_extension_initialize_with_user_data (WebKitWebExtension *extension,
> + *                                                 const GVariant     *user_data)
> + * {
> + *     g_signal_connect (extension, "page-created",
> + *                       G_CALLBACK (web_page_created_callback),
> + *                       NULL);
> + * }

If we are not going to use the data in the example, it would be better to use webkit_web_extension_initialize() instead.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:76
> + * a #WebKitWebPage is created. You should also modify your building tools (e.g. CMake or
> + * Autotools) to compile this individual module.

I'm not sure we should mention build systems in api docs.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:78
> + * WebKit has to know where it can find the created #WebKitWebExtension. To do so you

Dot not use # here.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:80
> + * must be called as soon as posible in our app and before any WebProcess is launched.

in our app? I would just mention initialize-web-extensions signal as the recommended place to avoid confusions.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:83
> + * To provide the initialization data used by the `webkit_web_extension_initialize_with_user_data`

I guess you provide init data to the function, not by the function. In any case, I think Martin or any other native english speaker should review all this doc.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:100
> + *       context, WEB_EXTENSIONS_DIRECTORY);
> + *   webkit_web_context_set_web_extensions_initialization_user_data (
> + *      context, g_variant_new_uint32 (unique_id++));

It seems one of these lines is not correctly indented

[Marcos Chavarría Teijeiro (irc: chavaone)]

Created attachment 253437 [details]
Patch

[Marcos Chavarría Teijeiro (irc: chavaone)]

Comment on attachment 250778 [details]
Patch

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

>> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:83
>> + * To provide the initialization data used by the `webkit_web_extension_initialize_with_user_data`
> 
> I guess you provide init data to the function, not by the function. In any case, I think Martin or any other native english speaker should review all this doc.

I think this is OK because it is "initialization data **used by** the function". Anyway I agree that a English native speaker review would be very helpful.

[Michael Catanzaro]

Marcos's sentence is better. You could say "To provide initialization data to the function" instead, but Marcos's version is more clear. I would not say "To provide the initialization data used to the function."

A few comments:

"It allows you to execute code in the WebProcess and being able to use the DOM API, to change any request or to inject custom JavaScript code, for example." Since you started off a list of items that begin with "to," the rest of the items should work following "to" as well; in particular, the tense change "being able" is out of place. I don't know what the fancy grammar rule for that is (in school I was taught "parallelism," but the Internet thinks that is something different). There are many different ways to fix this, e.g. "It allows you to execute code in the web process; for example, you can use the DOM API, modify HTTP requests, or inject custom JavaScript."

"GTK module" -> "GTK+ module"

Add the word "process" after "in case you need to initilize the extension with some data from the UI"

"These functions types" -> "These functions' types" since this is possessive. Later in this sentence, add a comma before "respectively," and then one more after "In addition" in the next sentence.

"This function must be called as soon as posible in our app and before any WebProcess is launched." I would say "This function must be called as soon as posible in our app, before any WebProcess is launched."

I could nitpick quite a bit more, but I don't think there's value in that. The English is pretty good. :)

[Michael Catanzaro]

Actually, I almost missed this: initilize -> initialize

If you use gedit, you could temporarily turn on Highlight Misspelled Words to catch this.

[Carlos Garcia Campos]

(In reply to comment #15)
> Comment on attachment 250778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250778&action=review
> 
> >> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:83
> >> + * To provide the initialization data used by the `webkit_web_extension_initialize_with_user_data`
> > 
> > I guess you provide init data to the function, not by the function. In any case, I think Martin or any other native english speaker should review all this doc.
> 
> I think this is OK because it is "initialization data **used by** the
> function". Anyway I agree that a English native speaker review would be very
> helpful.

Right, I missed the *used* when I read it, that's why it sounded weird to me.

[Carlos Garcia Campos]

Comment on attachment 253437 [details]
Patch

Thanks

[WebKit Commit Bot]

Comment on attachment 253437 [details]
Patch

Clearing flags on attachment: 253437

Committed r184638: <http://trac.webkit.org/changeset/184638>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.