[WPE][GTK] Missing API to pass client-side messages to new pages on the web extension side

[psaavedra]

b1f6ab59d937c89e9a31525a3ae3c2bafadadfdd

[WPE][GTK] Missing API to pass client-side messages to new pages on the web extension side
https://bugs.webkit.org/show_bug.cgi?id=238598

The WebProcessPool sends a Messages::WebProcess::InitializeWebExtensions
with serialized UserData information every time that a
DidInitiateLoadForResource message is being received from a
WebProcess for a WebKitWebView what still doesn't have a main
resource loaded.

The WebProcessPool contructs the UserData message using the data from the
priv->webExtensionsInitializationUserData in the WebKitWebView.
It requires of a new getInjectedBundleInitializationUserDataByPage()
method in the InjectedBundle API.

A WebKitWebView can update the priv->webExtensionsInitializationUserData by
listening the "initialize-web-extensions" signal and use the
webkit_web_view_set_web_extensions_initialization_user_data()

The WebProcess receives the InitializeWebExtensions message and
triggers an "initialize-web-extensions" signal for the
WebKitWebExtension.

A WebKitWebExtension can get the UserData message by listening the
"initialize-web-extensions" signal.

Reviewed by NOBODY (OOPS!).

* Source/WebKit/UIProcess/API/APIInjectedBundleClient.h:
(API::InjectedBundleClient::getInjectedBundleInitializationUserDataByPage):
* Source/WebKit/UIProcess/API/glib/WebKitInjectedBundleClient.cpp:
* Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:
(webkit_web_view_class_init):
(webkit_web_view_set_web_extensions_initialization_user_data):
(webkitWebViewHasMainResource):
(webkitWebViewInitializeWebExtensions):
* Source/WebKit/UIProcess/API/glib/WebKitWebViewPrivate.h:
* Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:
* Source/WebKit/UIProcess/API/wpe/WebKitWebView.h:
* Source/WebKit/UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::handleMessage):
* Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:
(parseUserData):
(webkitWebExtensionDidInitializeWebExtension):
* Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtensionPrivate.h:
* Source/WebKit/WebProcess/WebProcess.h:
* Source/WebKit/WebProcess/WebProcess.messages.in:
* Source/WebKit/WebProcess/glib/WebProcessGLib.cpp:
(WebKit::WebProcess::initializeWebExtensions):

[mcatanzaro]

No need for these parentheses:

Suggested change

	#if (PLATFORM(GTK) || PLATFORM(WPE))
	#if PLATFORM(GTK) || PLATFORM(WPE)

This API looks like it could be built unconditionally. Even if it won't be used on other ports, it's nicer to avoid the extra build guards when possible. We have to get the change approved by a WK2 owner regardless, so might as well ask if they'd be OK with this outside our guards.

[mcatanzaro]

Hi WK2 owners, do you have a preference here?

[aperezdc]

Not an owner, but I my feeling is that it would be nicer to not have the guards, too.

[mcatanzaro]

So my problem with this pull request is that, with no way to associate the message with any particular web page, you didn't actually accomplish the original goal, right? I would add a page-created-with-user-data signal instead, which would solve this problem better than initialize-web-extensions. (Then in the future upcoming API version, we can get rid of the original page-created and rename the new page-created-with-user-data to page-created.)

[psaavedra]

That is an interesting idea. If I understood well, I could implement that by modifying the CreateWebPage message by adding the getInjectedBundleInitializationUserDataByPage(*this, page) userData in the message:

- Source/WebKit/WebProcess/WebProcess.messages.in:    CreateWebPage(WebCore::PageIdentifier newPageID, struct WebKit::WebPageCreationParameters pageCreationParameters)
+ Source/WebKit/WebProcess/WebProcess.messages.in:    CreateWebPage(WebCore::PageIdentifier newPageID, struct WebKit::WebPageCreationParameters pageCreationParameters, WebKit::UserData userData)

Then, the message is used in this 2 places. Not sure if I can get access to the std::unique_ptr<WebPageInjectedBundleClient> m_injectedBundleClient; in there:

Source/WebKit/UIProcess/WebPageProxy.cpp:    send(Messages::WebProcess::CreateWebPage(m_webPageID, creationParameters(m_process, *m_drawingArea)), 0);
Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:    m_process->send(Messages::WebProcess::CreateWebPage(m_webPageID, parameters), 0);

[psaavedra]

Notice that I need access to the WebPageInjectedBundleClient because it is the one who has the reference to the WebContext. A reference that I need to get the WebView using webkitWebContextGetWebViewForPage(m_webContext, &page)

[psaavedra]

better thought, I dont think changing the signature of `CreateWebPage(WebCore::PageIdentifier newPageID, struct WebKit::WebPageCreationParameters pageCreationParameters) could be a good idea. That actually breaks the current API, right?

[psaavedra]

An alternative can be to add the UserData info in the WebPageCreationParameters

diff --git a/Source/WebKit/Shared/WebPageCreationParameters.h b/Source/WebKit/Shared/WebPageCreationParameters.h
index cb741693..64025d24 100644
--- a/Source/WebKit/Shared/WebPageCreationParameters.h
+++ b/Source/WebKit/Shared/WebPageCreationParameters.h
@@ -30,6 +30,7 @@
 #include "SandboxExtension.h"
 #include "SessionState.h"
 #include "UserContentControllerParameters.h"
+#include "UserData.h"
 #include "WebCoreArgumentCoders.h"
 #include "WebPageGroupData.h"
 #include "WebPageProxyIdentifier.h"
@@ -262,6 +263,8 @@ struct WebPageCreationParameters {
 #if HAVE(TOUCH_BAR)
     bool requiresUserActionForEditingControlsManager { false };
 #endif
+
+    std::optional<UserData> userData;
 };
 
 } // namespace WebKit

[mcatanzaro]

Not sure if I can get access to the std::unique_ptr m_injectedBundleClient; in there:

You'll could add a public getter function WebProcessPool::injectedBundleClient for this... but actually, what you're really looking for -- if you take my advice -- is not really injected bundle initialization user data at all, but web page initialization user data! So you might not actually need this. Instead of adding code to WebProcessPool and WebKitWebExtension, can you try doing everything with just WebPageCreationParameters, and forget about getInjectedBundleInitializationUserDataByPage? I think it would work?

[psaavedra]

Just to ensure I catch you. You are suggesting to move the user data as part of the WebView properties and pass it as part of the pageConfiguration in the webkitWebContextCreatePageForWebView(). is that?. Something like this?

diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
index 1b128a4c..3d1c761e 100644
--- a/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp
@@ -1893,6 +1893,7 @@ void webkitWebContextCreatePageForWebView(WebKitWebContext* context, WebKitWebVi
     pageConfiguration->setRelatedPage(relatedView ? &webkitWebViewGetPage(relatedView) : nullptr);
     pageConfiguration->setUserContentController(userContentManager ? webkitUserContentManagerGetUserContentControllerProxy(userContentManager) : nullptr);
     pageConfiguration->setControlledByAutomation(webkit_web_view_is_controlled_by_automation(webView));
+    pageConfiguration->setUseData(webkit_web_view_get_user_data(webView) : nullptr);
 
     WebKitWebsiteDataManager* manager = webkitWebViewGetWebsiteDataManager(webView);
     if (!manager)
diff --git a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
index 845dda08..28c7838f 100644
--- a/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
+++ b/Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp
@@ -213,6 +213,8 @@ enum {
     PROP_MICROPHONE_CAPTURE_STATE,
     PROP_DISPLAY_CAPTURE_STATE,
 
+    PROP_USER_DATA,
+
     N_PROPERTIES,
 };
 
@@ -905,6 +907,10 @@ static void webkitWebViewSetProperty(GObject* object, guint propId, const GValue
     case PROP_DISPLAY_CAPTURE_STATE:
         webkit_web_view_set_display_capture_state(webView, static_cast<WebKitMediaCaptureState>(g_value_get_enum(value)));
         break;
+    case PROP_USER_DATA:
+	gpointer userData = g_value_get_object(value);
+        webView->priv->userData = userData ? G_TYPE_CHECK_INSTANCE_CAST(userData, G_TYPE_VARIANT, GVariant) : nullptr;
+        break;
     default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propId, paramSpec);
     }
@@ -985,6 +991,9 @@ static void webkitWebViewGetProperty(GObject* object, guint propId, GValue* valu
     case PROP_DISPLAY_CAPTURE_STATE:
         g_value_set_enum(value, webkit_web_view_get_display_capture_state(webView));
         break;
+    case PROP_USER_DATA:
+        g_value_set_variant(value, webView->priv->userData.get());
+        break;
     default:
         G_OBJECT_WARN_INVALID_PROPERTY_ID(object, propId, paramSpec);
     }
@@ -1454,6 +1463,16 @@ static void webkit_web_view_class_init(WebKitWebViewClass* webViewClass)
         WEBKIT_MEDIA_CAPTURE_STATE_NONE,
         WEBKIT_PARAM_READWRITE);
 
+    /*
+     * Since: 2.38
+     */
+    sObjProperties[PROP_USER_DATA] = g_param_spec_object(
+        "user-data",
+        "User Data",
+        _("Associated custom user data"),
+        G_TYPE_VARIANT,
+        WEBKIT_PARAM_READWRITE);
+
     g_object_class_install_properties(gObjectClass, N_PROPERTIES, sObjProperties);
 
     /**

Then add it to the WebPageCreationParameters attached to the CreateWebPage IPC message and expose it as userData in a page-created-user-data signal.

[mcatanzaro]

Just to ensure I catch you. You are suggesting to move the user data as part of the WebView properties

Hmm... that had not been my intent, but it does seem to be the only way to implement my suggestion. You have to have the user data ready when the web view is created because it's required when calling webkitWebContextCreatePageForWebView in webkitWebViewConstructed. And the only way to do that would be to use a GObject property. Meh.

I suppose using a GObject property for this would be OK. (I would name it web-process-extension-user-data instead of just user-data.) But here's an alternative suggestion: instead of trying to implement Claudio's first proposal from https://bugs.webkit.org/show_bug.cgi?id=238598#c0, you could try to implement the second proposal instead: just add a WebKitWebView::page-created signal, then you have a solution that's actually more flexible than sending user data when the web page is initialized, and you dodge all of the problems we've discussed thus far. In particular, you'd have a way to send messages to new web pages (as requested by the title of the bug), whereas what you have now only allows you to send data to a new web extension, which can have many pages.

[mcatanzaro]

just add a WebKitWebView::page-created signal

Also, the application can use the existing IPC APIs and you don't need to worry about it. All you'd need to do is make sure the signal is emitted at the right time, and the application can handle the rest.

[mcatanzaro]

You could get close to a WebKitWebView::page-created by firing a signal from PageClient::didChangeWebPageID. That's called from WebPageProxy::swapToProvisionalPage. Swap to provisional page is not the same thing as page creation, though....

[mcatanzaro]

Note: this doesn't make sense because all web views in this web context correspond to this extension. (This problem would go away if you added a new page-created signal instead.)

[psaavedra]

That could be a wrong assumption. Since 2.26 [1][2], where WEBKIT_PROCESS_MODEL_SHARED_SECONDARY_PROCESS is set deprecated, the Web context can be set with WEBKIT-PROCESS-MODEL-MULTIPLE-SECONDARY-PROCESSES . In that scenario every single WebView uses a dedicated WebProcess.

[1] https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebContext.html
[2] https://webkitgtk.org/reference/webkit2gtk/stable/WebKitWebContext.html#WEBKIT-PROCESS-MODEL-MULTIPLE-SECONDARY-PROCESSES:CAPS

[mcatanzaro]

OK yes, that's true (in fact, that's the only supported process model currently). But what I really meant was that having WebKitWebView send initialization data corresponding to only the WebKitWebExtension, not corresponding to a WebKitWebPage, is not very useful. Consider that each WebKitWebExtension will receive this initialization data many times, once each time a new WebKitWebPage is created, but with no association to that page (unless you implement it manually by sending the page ID via the user data variant). It's going to be confusing for application developers, and difficult to use correctly. You can avoid that by associating your initialization data with the web page, which is what Claudio was requesting with his bug report.

[psaavedra]

I see your point. Still the signal can be useful for sending initialization data for the WebExtension from the UI-process for every single WebView separately but I am also agree with a page-created-with-user-data signal associated with the WebPage it could be even more useful.

[mcatanzaro]

(unless you implement it manually by sending the page ID via the user data variant)

Actually even this won't work, because the UI process only knows the page ID at the time that you call webkit_web_view_get_page_id(). There's no way for the UI process to detect process swaps or prewarmed processes. So I'm fairly confident that it's not actually possible to use this API to resolve https://bugs.webkit.org/show_bug.cgi?id=238598. Having a WebKitWebView::page-created signal would help with this.

[mcatanzaro]

Also, this needs API tests in Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebExtensions.cpp (see also Tools/TestWebKitAPI/Tests/WebKitGLib/WebExtensionTest.cpp). That won't be quick/easy, but it's required for new API. I've marked this PR as a draft in the meantime.

[aperezdc]

This is not really needed. Adding a vfunc to the class struct is done when one wants to provide a default implementation and/or to allow overriding it in subclasses. You actually pass 0 as the class_offset when creating the signal below, which means this slot is unused.

If you end up adding a signal to WebKitWebView you do not need to consume our last padding entry. Even without a slot in the class struct, a subclass can still use g_signal_override_class_handler() to override the default implementation of an vfunc. It's a bit more involved, but I think it's a reasonable tradeoff for vfuncs which do not need a default implementation and are not likely to be overriding in subclasses.

[shivamidow]

This is an unofficial review for practice.

We can avoid an embedded if statement here by using an early return.

WebKitWebView* webView = webkitWebContextGetWebViewForPage();
if (!webView || webkitWebViewHasMainResources(webView))
    return nullptr;

GRefPtr<GVariant> data = webkitWebViewInitializeWebExtensions(webView);
...
return API::String::create(String::fromUTF8(dataString.get()));