Bug 163366 – [GTK] No way for applications to set notification permissions without waiting for permission request

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Websites can check the Notification.permission DOM property [1] to check if they have permission to show notifications without triggering an intrusive permission request (that's done via Notification.requestPermission()). But the WebKitGTK+ API has no way to affirmatively indicate notifications are allowed. The only way is via the permission request API, which is too late. It's a problem for Epiphany. For example, open the Riot Matrix client in a web app; you will immediately see an info bar informing the user that Riot does not have permission to send desktop notifications, even though Epiphany automatically grants notification permission in web app mode when requested.

The problem is in WebKitNotificationProvider.cpp, where we need to implement notificationPermissionsCallback of WKNotificationProviderV0. The WKDictionaryRef returned by that callback should be set by some new API function. Simple proposal:

void webkit_web_context_set_notification_permissions (WebKitWebContext *context, GHashTable *permissions);

where the hash table is a map of strings to bools.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Notifications_API/Using_the_Notifications_API

[Carlos Alberto Lopez Perez]

Using Adrian's vector-gnome application If I open this HTML5 notification API test website like:


$ ./vector http://www.bennish.net/web-notifications.html

And I click on the 'Authorize' button on that website I get the request automatically granted without showing an infobar at all.


However, using the same program with the Riot Matrix client, the infobar appears.


Why is that?


Also I just checked this on the Riot Matrix client (with vector-gnome):

 * Open it and ignore the infobar about granting desktop notifications
 * Go to settings on Riot (a gear icon at the bottom right)
 * Click on the checkbox "Enable desktop notifications"

And you have now desktop notifications working without clicking on the infobar.


So perhaps the issue is related with the way the Riot Matrix webapp client checks if notifications are enabled/granted/working ?

[Carlos Alberto Lopez Perez]

A quick test based on this doc:

./vector https://jsfiddle.net/o8hfzbrc/

gives notifications are already granted


Here seems to be the code from Riot Matrix React SDK: https://github.com/matrix-org/matrix-react-sdk/blob/master/src/Notifier.js ... Is not obvious to me what is going wrong.

[Michael Catanzaro]

What's going wrong is that checking Notification.permission via JavaScript does not trigger a WebKitNotificationPermissionRequest (nor should it). The sites where notification permissions work properly simply don't check this property. If http://www.bennish.net/web-notifications.html were to attempt to tell you if notifications are authorized without creating a permission request (via Notification.requestPermission()), then it would say that notifications are not authorized (even though the permission request would be automatically granted by the application. WebKit already has a means to prepopulate notification permissions (see comment #0) that we need to expose in the WebKitGTK+ API.

[Carlos Alberto Lopez Perez]

(In reply to comment #3)
> What's going wrong is that checking Notification.permission via JavaScript
> does not trigger a WebKitNotificationPermissionRequest (nor should it). The
> sites where notification permissions work properly simply don't check this
> property. If http://www.bennish.net/web-notifications.html were to attempt
> to tell you if notifications are authorized without creating a permission
> request (via Notification.requestPermission()), then it would say that
> notifications are not authorized (even though the permission request would
> be automatically granted by the application. WebKit already has a means to
> prepopulate notification permissions (see comment #0) that we need to expose
> in the WebKitGTK+ API.

I see. Thanks for the explanation.

I was confused, I was not checking properly that Notification.permission started with a value of "default" before the call to requestPermission()

Checked with the inspector:

> Notification.permission
< "default"
> Notification.requestPermission()
< undefined
> Notification.permission
< "granted"


Notification.permission

[Michael Catanzaro]

Created attachment 295281 [details]
Patch

[Gustavo Noronha (kov)]

Comment on attachment 295281 [details]
Patch

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

I don't think I understand the need for this feature. Wouldn't it be the same if the application simply handled the permission request and looked at its hashtable to decide whether it should allow or not?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1244
> + * Sets initial desktop notification permissions for the @context.
> + * @permissions must be a #GHashTable of strings containing security
> + * origins (protocol, host, and optional port, e.g.
> + * http://www.example.com but not http://www.example.com/) to boolean
> + * values (stored using GINT_TO_POINTER()). No

I wonder if the hashtable will be friendly to introspection. Maybe we can add a method that takes uri and decision, the user has to call it as many times as it wants, instead?

[Michael Catanzaro]

(In reply to comment #6)
> I don't think I understand the need for this feature. Wouldn't it be the
> same if the application simply handled the permission request and looked at
> its hashtable to decide whether it should allow or not?

Like I explain above, the problem is that the Notification JS API has separate functions to (a) look up the permission, and (b) request permission. In the case of (a), we don't want the browser to actually display a permission request.

We could still use WebKitNotificationPermissionRequest by adding a property to indicate whether the request should be displayed to the user, but that's harder as it requires modifying cross-platform code.

> I wonder if the hashtable will be friendly to introspection. Maybe we can
> add a method that takes uri and decision, the user has to call it as many
> times as it wants, instead?

Hm, good point, I don't know about introspection....

[Adrian Perez]

(In reply to comment #7)
> (In reply to comment #6)
> > [...]
> >
> > I wonder if the hashtable will be friendly to introspection. Maybe we can
> > add a method that takes uri and decision, the user has to call it as many
> > times as it wants, instead?
> 
> Hm, good point, I don't know about introspection....

Hash tables should play with introspection just fine, as long as the needed
annotations are added. In this case the following would be needed:

  (element-type utf8 gboolean)

This is supported since gobject-introspection 0.5.0, which is a non-issue
as it is a super old version :-)

[Michael Catanzaro]

(In reply to comment #8)
> Hash tables should play with introspection just fine, as long as the needed
> annotations are added. In this case the following would be needed:
> 
>   (element-type utf8 gboolean)

Thanks, let's do this....

[Michael Catanzaro]

Created attachment 295428 [details]
Patch

[Gustavo Noronha (kov)]

(In reply to comment #7)
> Like I explain above, the problem is that the Notification JS API has
> separate functions to (a) look up the permission, and (b) request
> permission. In the case of (a), we don't want the browser to actually
> display a permission request.
> 
> We could still use WebKitNotificationPermissionRequest by adding a property
> to indicate whether the request should be displayed to the user, but that's
> harder as it requires modifying cross-platform code.

I know that, but I still don't understand why the API user cannot simply reply to the request, since it knows the sites it should always allow, anyway. Is there any case where we need the site's look up permission to return true from the onset?

The browser wouldn't display the permission request. Consider the following handler for the permission request signal:

if (hosts_to_always_allow.contains(url.host)) {
  webkit_permission_request_allow (request);
  return TRUE;
}

g_object_ref (request);
show_request_infobar (request);
return TRUE;

[Michael Catanzaro]

(In reply to comment #11) 
> I know that, but I still don't understand why the API user cannot simply
> reply to the request, since it knows the sites it should always allow,
> anyway. Is there any case where we need the site's look up permission to
> return true from the onset?
> 
> The browser wouldn't display the permission request. Consider the following
> handler for the permission request signal:
> 
> if (hosts_to_always_allow.contains(url.host)) {
>   webkit_permission_request_allow (request);
>   return TRUE;
> }
> 
> g_object_ref (request);
> show_request_infobar (request);
> return TRUE;

This is possible, but there are two challenges:

 * If we do that, we must add a property to the permission request to indicate to the browser that it must not display the permission request. Otherwise the browser would have to way to distinguish between JavaScript calling Notification.permission (which must never trigger a user-visible request) and Notification.requestPermission(). It would break old versions of Epiphany using the notifications API without checking the property, although it's not a horrible problem. This means we have to add new API regardless, although it would be a different API.

 * Currently, Notification.permission is resolved entirely in the web process, and only Notification.requestPermission() goes to the UI process. Notification.requestPermission() is promise-based so the callback occurs asynchronously. If we use the existing WebKitNotificationPermissionRequest API to handle Notification.permission, that would require changing Notification.permission to use a *synchronous* UI process callback the first time it is called for a host, blocking all JS execution until it is resolved. It would require WK2 owner approval as we would have to change the cross-platform code to do this. Also, it will break existing versions of Epiphany in the case where notification permission has not yet been granted, as they will display a permission request to the user anyway and the web process will just hang until it's resolved.

So it's a tough call, but there are some serious disadvantages to using the existing WebKitNotificationPermissionRequest API.

[Gustavo Noronha (kov)]

(In reply to comment #12)
> This is possible, but there are two challenges:
> 
>  * If we do that, we must add a property to the permission request to
> indicate to the browser that it must not display the permission request.
> Otherwise the browser would have to way to distinguish between JavaScript
> calling Notification.permission (which must never trigger a user-visible
> request) and Notification.requestPermission(). It would break old versions
> of Epiphany using the notifications API without checking the property,
> although it's not a horrible problem. This means we have to add new API
> regardless, although it would be a different API.

I guess this is the bit I'm not understanding. Isn't Epiphany the one that would be pre-loading the permissions anyway? Doesn't Epiphany need to be modified already to add the call to this API and to keep track of a hash table?

I don't see why we need that permission. The browser *is* the one handling this, it has all the information it needs to not show the info bar. Notification.permission does not trigger a request, there is no need for a property.


>  * Currently, Notification.permission is resolved entirely in the web
> process, and only Notification.requestPermission() goes to the UI process.
> Notification.requestPermission() is promise-based so the callback occurs
> asynchronously. If we use the existing WebKitNotificationPermissionRequest
> API to handle Notification.permission, that would require changing
> Notification.permission to use a *synchronous* UI process callback the first
> time it is called for a host, blocking all JS execution until it is
> resolved. It would require WK2 owner approval as we would have to change the
> cross-platform code to do this. Also, it will break existing versions of
> Epiphany in the case where notification permission has not yet been granted,
> as they will display a permission request to the user anyway and the web
> process will just hang until it's resolved.
> 
> So it's a tough call, but there are some serious disadvantages to using the
> existing WebKitNotificationPermissionRequest API.

Why would we want Notification.permission to generate a request? The spec does not call for a request to be generated from Notification.permission, right? This is what should happen from my point of view:

1. Web app checks Notification.permission -> "default" (means denied with no request done yet)

2. Web app calls Notification.requestPermission

3. Epiphany gets the permission-requested signal

4. Epiphany checks its database of pre-approved sites

4.1 if the site is pre-approved, reply to the request immediately without showing an infobar

4.2 if the site is pre-denied, reply to the request immediately without showing an infobar

4.3 if no decision is known for the site, show an infobar, reply to request async

[Gustavo Noronha (kov)]

(In reply to comment #13)
> I don't see why we need that permission.

s/permission/property/

[Michael Catanzaro]

> > So it's a tough call, but there are some serious disadvantages to using the
> > existing WebKitNotificationPermissionRequest API.
> 
> Why would we want Notification.permission to generate a request? The spec
> does not call for a request to be generated from Notification.permission,
> right? This is what should happen from my point of view:
> 
> 1. Web app checks Notification.permission -> "default" (means denied with no
> request done yet)
> 
> 2. Web app calls Notification.requestPermission

That's where the problem is. There exist real-world web apps that don't actually do that. In the case of Riot, if it sees "default" it displays an info bar "You do not have permission to display notifications, click here to request permission" and only calls Notification.requestPermission if the user clicks on its info bar. And the web app is right to do that, because "default" does mean "denied" like you say. It's our fault for not returning "granted" there. We can either solve it by adding API to preload permissions (as my patch does), or by overloading the existing WebKitNotificationPermissionRequest API (with the negative consequences I described above).

[Adrian Perez]

(In reply to comment #15)
> > > So it's a tough call, but there are some serious disadvantages to using the
> > > existing WebKitNotificationPermissionRequest API.
> > 
> > Why would we want Notification.permission to generate a request? The spec
> > does not call for a request to be generated from Notification.permission,
> > right? This is what should happen from my point of view:
> > 
> > 1. Web app checks Notification.permission -> "default" (means denied with no
> > request done yet)
> > 
> > 2. Web app calls Notification.requestPermission
> 
> That's where the problem is. There exist real-world web apps that don't
> actually do that. In the case of Riot, if it sees "default" it displays an
> info bar "You do not have permission to display notifications, click here to
> request permission" and only calls Notification.requestPermission if the
> user clicks on its info bar. And the web app is right to do that, because
> "default" does mean "denied" like you say. It's our fault for not returning
> "granted" there. We can either solve it by adding API to preload permissions
> (as my patch does), or by overloading the existing
> WebKitNotificationPermissionRequest API (with the negative consequences I
> described above).

Today I have found out that WhatsApp Web works in Epiphany now, and it also
does a similar thing: It first checks “Notification.permission” and if it
sees “default”, it will show a message in the left sidebar which the user
can click, at which point “Notification.requestPermission” is used. Likely
there are many other applications out in the wild that do something similar.

The WebNotification spec allows for this kind of behaviours, and we should
provide a spec-compliant implementation, even if it's more difficult to
implement.

[Gustavo Noronha (kov)]

Comment on attachment 295428 [details]
Patch

Ah, so the web app itself is showing an infobar. OK, I thought you were talking about the browser's one. Well, I guess it makes sense.

[Michael Catanzaro]

OK thanks.

I'll wait until Monday to land this, in case Carlos has any comments.

[Carlos Garcia Campos]

Comment on attachment 295428 [details]
Patch

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

> Source/WebKit2/ChangeLog:7
> +

You should explain here the purpose of this new API.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:164
> +    m_notificationPermissions = API::Dictionary::create(permissionsMap);

API::Dictionary::create(WTFMove(permissionsMap));

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:56
> +    RefPtr<API::Dictionary> m_notificationPermissions { nullptr };

RefPtr already initializes its pointer to nullptr in the constructor.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1238
> + * values (stored using GINT_TO_POINTER()). No

Boolean values that the user has to store using G_INT_TO_POINTER, looks complex and weak. Maybe it would be easier if we have two parameters, one for the list of security origins to allow, and another one for the ones to deny. Similar to the whitelist/blakclist parameters of webkit_user_style_sheet_new and webkit_user_script_new

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1243
> + * #WebKitNotificationPermissionRequest will ever be generated for
> + * domains represented in the @permissions. If the boolean value
> + * corresponding to the domain in @permissions is %TRUE, then the domain
> + * will always have permission to show desktop notifications; if it is
> + * %FALSE, then it will never have permission to do so. This function

You started talking about security origins, and now you say domains. That's confusing. I think we should either ask for domains, or maybe expose security origins as WebKitSecurityOrigin, instead of requesting strings and explaining what a security origin is. I'm sure it will be used by other parts of the API eventually.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1250
> + * This function only affects web processes that have not already been
> + * created, so it should usually be called before creating the first
> + * #WebKitWebView.

We can avoid this by using a signal, like we do for initializing web extensions. notificationPermissions callback is called everytime a web process is spawned, with a signal, the user just connect to it and provides the permissions, we don't even need to store them, and it's possible to provide updated permissions.

> Tools/ChangeLog:17
> +        * TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp: Add ability to defer web view creation.

Why do you needs this?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:645
> +    MAKE_GLIB_TEST_FIXTURE_WITH_SETUP_TEARDOWN(NotificationWebViewTest, setup, teardown);
> +
> +    static void setup()
> +    {
> +        WebViewTest::shouldInitializeWebViewInConstructor = false;
> +    }
> +
> +    static void teardown()
> +    {
> +        WebViewTest::shouldInitializeWebViewInConstructor = true;
> +    }

Wouldn't it be possible to add a initialize virtual function to WebViewTest that calls initializeWebView by default? You could override it to do nothing and call initializeWebView on you own later if needed. I still don't understand why it's needed though.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:731
> +        auto test = static_cast<NotificationWebViewTest*>(userData);

auto*

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:746
> +        GError* error = nullptr;
> +        WebKitJavascriptResult* jsResult = webkit_web_view_run_javascript_finish(test->m_webView, result, &error);
> +        g_assert(!error);
> +
> +        JSGlobalContextRef context = webkit_javascript_result_get_global_context(jsResult);
> +        JSValueRef value = webkit_javascript_result_get_value(jsResult);
> +        g_assert(JSValueIsString(context, value));
> +
> +        // Possible values: "denied", "granted", "default" (implicit deny)
> +        JSStringRef jsStringValue = JSValueToStringCopy(context, value, nullptr);
> +        test->m_hasPermission = JSStringIsEqualToUTF8CString(jsStringValue, "granted");
> +        JSStringRelease(jsStringValue);
> +        webkit_javascript_result_unref(jsResult);
> +
> +        g_main_loop_quit(test->m_mainLoop);

All this is already implemented by WebViewTest, see below.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:753
> +        webkit_web_view_run_javascript(m_webView, "Notification.permission;", nullptr, hasPermissionCallback, this);
> +        g_main_loop_run(m_mainLoop);
> +        return m_hasPermission;

This could be:

auto* result = runJavaScriptAndWaitUntilFinished("Notification.permission;", nullptr);
g_assert(result);
GUniquePtr<char> value(javascriptResultToCString(result));
return !g_strcmp0(value.get(), "granted");

[Michael Catanzaro]

(In reply to comment #19) 
> You should explain here the purpose of this new API.

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:164
> > +    m_notificationPermissions = API::Dictionary::create(permissionsMap);
> 
> API::Dictionary::create(WTFMove(permissionsMap));

Right, oops. I'm still not familiar enough with move operations.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:56
> > +    RefPtr<API::Dictionary> m_notificationPermissions { nullptr };
> 
> RefPtr already initializes its pointer to nullptr in the constructor.

OK.

> Boolean values that the user has to store using G_INT_TO_POINTER, looks
> complex and weak. Maybe it would be easier if we have two parameters, one
> for the list of security origins to allow, and another one for the ones to
> deny. Similar to the whitelist/blakclist parameters of
> webkit_user_style_sheet_new and webkit_user_script_new

Good idea.

> You started talking about security origins, and now you say domains. That's
> confusing. I think we should either ask for domains, or maybe expose
> security origins as WebKitSecurityOrigin, instead of requesting strings and
> explaining what a security origin is. I'm sure it will be used by other
> parts of the API eventually.

Hm, the problem is a permission for http://example.com will have no effect on https://example.com, same as a permission for http://example.com:80 has no effect on http://example.com:8000. So we can't ask for domains. I guess we could expose a WebKitSecurityOrigin struct with separate protocol/host/port like SoupURI, is that what you suggest?

> We can avoid this by using a signal, like we do for initializing web
> extensions. notificationPermissions callback is called everytime a web
> process is spawned, with a signal, the user just connect to it and provides
> the permissions, we don't even need to store them, and it's possible to
> provide updated permissions.

Hm, good point, otherwise new web process would miss recently-added notification permissions.

> > Tools/ChangeLog:17
> > +        * TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp: Add ability to defer web view creation.
> 
> Why do you needs this?

To set the permissions before the web process is created by webkit_web_view_new, otherwise the web process never sees any of the permissions and the test just fails. It's going to be needed regardless, to connect to a future notification-permission signal of the web context before it's emitted when the first web view is created.

> Wouldn't it be possible to add a initialize virtual function to WebViewTest
> that calls initializeWebView by default? You could override it to do nothing
> and call initializeWebView on you own later if needed. I still don't
> understand why it's needed though.

No, because you can't call virtual functions from a constructor. The only alternative I could think of was to add explicit initialize calls in every test case that uses a WebViewTest, which we surely don't want to do.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:731
> > +        auto test = static_cast<NotificationWebViewTest*>(userData);
> 
> auto*

Really? I don't think so? auto& is needed to disambiguate between a copy and a reference, but here the rhs is already a pointer.
 
> This could be:
> 
> auto* result = runJavaScriptAndWaitUntilFinished("Notification.permission;",
> nullptr);
> g_assert(result);
> GUniquePtr<char> value(javascriptResultToCString(result));
> return !g_strcmp0(value.get(), "granted");

I'll try it.

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #19) 
> > You should explain here the purpose of this new API.
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:164
> > > +    m_notificationPermissions = API::Dictionary::create(permissionsMap);
> > 
> > API::Dictionary::create(WTFMove(permissionsMap));
> 
> Right, oops. I'm still not familiar enough with move operations.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:56
> > > +    RefPtr<API::Dictionary> m_notificationPermissions { nullptr };
> > 
> > RefPtr already initializes its pointer to nullptr in the constructor.
> 
> OK.
> 
> > Boolean values that the user has to store using G_INT_TO_POINTER, looks
> > complex and weak. Maybe it would be easier if we have two parameters, one
> > for the list of security origins to allow, and another one for the ones to
> > deny. Similar to the whitelist/blakclist parameters of
> > webkit_user_style_sheet_new and webkit_user_script_new
> 
> Good idea.
> 
> > You started talking about security origins, and now you say domains. That's
> > confusing. I think we should either ask for domains, or maybe expose
> > security origins as WebKitSecurityOrigin, instead of requesting strings and
> > explaining what a security origin is. I'm sure it will be used by other
> > parts of the API eventually.
> 
> Hm, the problem is a permission for http://example.com will have no effect
> on https://example.com, same as a permission for http://example.com:80 has
> no effect on http://example.com:8000. So we can't ask for domains. I guess
> we could expose a WebKitSecurityOrigin struct with separate
> protocol/host/port like SoupURI, is that what you suggest?

See patch attached to bug #146589, you have WebKitSecurityOrigin implemented there.

> > We can avoid this by using a signal, like we do for initializing web
> > extensions. notificationPermissions callback is called everytime a web
> > process is spawned, with a signal, the user just connect to it and provides
> > the permissions, we don't even need to store them, and it's possible to
> > provide updated permissions.
> 
> Hm, good point, otherwise new web process would miss recently-added
> notification permissions.
> 
> > > Tools/ChangeLog:17
> > > +        * TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp: Add ability to defer web view creation.
> > 
> > Why do you needs this?
> 
> To set the permissions before the web process is created by
> webkit_web_view_new, otherwise the web process never sees any of the
> permissions and the test just fails. It's going to be needed regardless, to
> connect to a future notification-permission signal of the web context before
> it's emitted when the first web view is created.

Yes, but you don't need any change in WebView creation. Notification permissions are a web context thing, so you can add a static Test::setNotificatinPermissions(), that you can call from setup and teardown (with nullptr in this case). Then, in Test() when setting up the context you connect to the new signal and set the permissions if they have been set.

> > Wouldn't it be possible to add a initialize virtual function to WebViewTest
> > that calls initializeWebView by default? You could override it to do nothing
> > and call initializeWebView on you own later if needed. I still don't
> > understand why it's needed though.
> 
> No, because you can't call virtual functions from a constructor. The only
> alternative I could think of was to add explicit initialize calls in every
> test case that uses a WebViewTest, which we surely don't want to do.

Ah, right!

> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:731
> > > +        auto test = static_cast<NotificationWebViewTest*>(userData);
> > 
> > auto*
> 
> Really? I don't think so? auto& is needed to disambiguate between a copy and
> a reference, but here the rhs is already a pointer.

It's not because it's needed, it's just for readability, making it explicit that what you get is a pointer.
 
> > This could be:
> > 
> > auto* result = runJavaScriptAndWaitUntilFinished("Notification.permission;",
> > nullptr);
> > g_assert(result);
> > GUniquePtr<char> value(javascriptResultToCString(result));
> > return !g_strcmp0(value.get(), "granted");
> 
> I'll try it.

[Michael Catanzaro]

(In reply to comment #21)
> See patch attached to bug #146589, you have WebKitSecurityOrigin implemented
> there.

OK, will you be returning to that patch this cycle, or do you want me to just copy it from there?

> Yes, but you don't need any change in WebView creation. Notification
> permissions are a web context thing, so you can add a static
> Test::setNotificatinPermissions(), that you can call from setup and teardown
> (with nullptr in this case). Then, in Test() when setting up the context you
> connect to the new signal and set the permissions if they have been set.

Good idea, that's better.

[Michael Catanzaro]

(In reply to comment #19)
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1250
> > + * This function only affects web processes that have not already been
> > + * created, so it should usually be called before creating the first
> > + * #WebKitWebView.
> 
> We can avoid this by using a signal, like we do for initializing web
> extensions. notificationPermissions callback is called everytime a web
> process is spawned, with a signal, the user just connect to it and provides
> the permissions, we don't even need to store them, and it's possible to
> provide updated permissions.

I think the only correct place to emit the signal would be from a new platform function at the top of WebProcessPool::createNewWebProcess. E.g. the existing initialize-web-extensions signal happens too late, after the notification permissions have already been copied inside WebProcessPool::createNewWebProcess.

[Michael Catanzaro]

(In reply to comment #23)
> I think the only correct place to emit the signal would be from a new
> platform function at the top of WebProcessPool::createNewWebProcess. E.g.
> the existing initialize-web-extensions signal happens too late, after the
> notification permissions have already been copied inside
> WebProcessPool::createNewWebProcess.

It's actually kinda tricky to find a good way to call back into WebKitWebContext from WebProcessPool. Hopefully we don't need to add a new Client class just for this one callback. Do you have any suggestions, Carlos?

[Carlos Garcia Campos]

We already have the callback, it's notificationPermissions, use that to emit our signal.

[Michael Catanzaro]

(In reply to comment #25)
> We already have the callback, it's notificationPermissions, use that to emit
> our signal.

Good point.

[Michael Catanzaro]

(In reply to comment #21)
> Yes, but you don't need any change in WebView creation. Notification
> permissions are a web context thing, so you can add a static
> Test::setNotificatinPermissions(), that you can call from setup and teardown
> (with nullptr in this case). Then, in Test() when setting up the context you
> connect to the new signal and set the permissions if they have been set.

I tried this. While it's possible, it means you need a different Test subclass for each call to Test::setNotificationPermissions, to have a different implementation of setup. That's terrible; we surely want to be able to set the permissions from the testcase itself. Deferring the creation of the webview is more general, so it could be useful in the future, and seems better. And this isn't the first time I've wanted more control over when the webview is created in API tests; although I don't remember what the previous time was, I bet we might want this again in the future. So I haven't changed how the test works.

[Michael Catanzaro]

Created attachment 297834 [details]
Patch

[Michael Catanzaro]

Created attachment 297861 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 297861 [details]
Patch

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

> Source/WebKit2/PlatformGTK.cmake:225
> +    UIProcess/API/gtk/WebKitSecurityOrigin.cpp
> +    UIProcess/API/gtk/WebKitSecurityOrigin.h
> +    UIProcess/API/gtk/WebKitSecurityOriginPrivate.h

I think we can move security origin to its own patch/bug.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:59
> +    return toAPI(toNotificationProvider(clientInfo)->notificationPermissions().leakRef());

Why leakRef()? If we are leaking our ref why do we keep a m_notificationPermissions member?

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:167
> +RefPtr<API::Dictionary> WebKitNotificationProvider::notificationPermissions()
> +{
> +    webkitWebContextSetInitialNotificationPermissions(m_webContext);
> +    return m_notificationPermissions;

Do we really need to keep the m_notificationPermissions member? Could we make webkitWebContextSetInitialNotificationPermissions return a RefPtr<API:Dictionary> and simply return it here transferring the ownership to the caller?

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:173
> +void WebKitNotificationProvider::setNotificationPermissions(HashMap<String, RefPtr<API::Object>>&& permissionsMap)
> +{
> +    m_notificationPermissions = API::Dictionary::create(WTFMove(permissionsMap));
> +}

And we don't need this at all

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:50
> +    _WebKitSecurityOrigin(WebCore::SecurityOrigin* coreSecurityOrigin)

WebKitSecurityOrigin always wraps a WebCore::SecurityOrigin and all create methods return a Ref<>, so I think we could use a reference here, or even better a Ref<> and also use Ref<> instead of RefPtr for the member, but that's already the other constructor. since this one is not realy used in this patch, simply remove it, if we need it eventually, we can add it later.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:51
> +        : securityOrigin(coreSecurityOrigin)

There was a bug in my patch, referenceCount should be initialized to 1 here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:56
> +        : securityOrigin(WTFMove(coreSecurityOrigin))

And here.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:60
> +    RefPtr<WebCore::SecurityOrigin> securityOrigin;

I think this could be Ref<>

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:74
> +WebKitSecurityOrigin* webkitSecurityOriginCreate(WebCore::SecurityOrigin* coreSecurityOrigin)
> +{
> +    WebKitSecurityOrigin* origin = static_cast<WebKitSecurityOrigin*>(fastMalloc(sizeof(WebKitSecurityOrigin)));
> +    new (origin) WebKitSecurityOrigin(coreSecurityOrigin);
> +    return origin;
> +}

This is unused in this patch, let's remove it for now, we can add it eventually if actually needed.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:110
> +    if (port && WebCore::defaultPortForProtocol(protocol) != static_cast<uint16_t>(port))

Use guint16 instead of guint, then you don't need any cast here and it matches WebCore::SecurityOrigin. Here you are checking port != 0, so I assume you are considering that passing 0 as port means using the default. That should be document.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:131
> + * webkit_security_origin_new_from_string:
> + * @uri: The URI for the new origin
> + *
> + * Create a new security origin from the provided URI. Components of
> + * @uri other than protocol, host, and port will be ignored.
> + *
> + * Returns: (transfer full): A #WebKitSecurityOrigin.
> + *
> + * Since: 2.16
> + */
> +WebKitSecurityOrigin* webkit_security_origin_new_from_string(const gchar* uri)
> +{
> +    g_return_val_if_fail(uri, nullptr);
> +
> +    return webkitSecurityOriginCreate(WebCore::SecurityOrigin::createFromString(String::fromUTF8(uri)));

There's an interesting mix here. The method is called from_string, and it indeed uses the createFromString() method, but the parameter is uri, and documentation says you provide a URI indeed. String is supposed to be a string representation of a security origin (so, it doesn't make sense the doc saying that other uri components will be ignored, for example). I think what we want here is the URL constructor, I would use for instead of from too, webkit_security_origin_new_for_uri(). Instead of saying that components will be ignored we should document that a security origin represents, so it's not that the path/query/fragment is ignored, it's that we are creating the security origin corresponding to the given URI. And of course we should use create() method that receives a URL. We could create a SoupURI and g_return_val_if_fail if it's invalid instead of just checking it's not null.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:223
> + * Get the port of @origin. This function will always return 0 if the
> + * port is the default port for the given protocol. For example,
> + * http://example.com has the same security origin as
> + * http://example.com:80, and this function will return 0 for a
> + * #WebKitSecurityOrigin constructed from either string.

Exactly, that's what we should document in webkit_security_origin_new(), since we also allow to pass 0 there meaning the default. Maybe we could add a macro to make it more explicit

#define WEBKIT_SECURITY_ORIGIN_PORT_DEFAULT 0

Or something similar.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:244
> + * Get a string representation of @origin. Be aware that this function
> + * may return %NULL if the origin is unique. (For example, this will
> + * occur if @origin represents a sandboxed iframe.)

So, we are introducing here the concept of unique origins. We should explain what a unique origin is (maybe not here, but in the section documention), and probably also add webkit_security_origin_is_unique(). We should also explain what the string representation is (like the url without the path/query/fragment parts).

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:246
> + * Returns: (allow-none): a string representation of @origin.

, or %NULL if @origin is unique

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:250
> +const gchar* webkit_security_origin_to_string(WebKitSecurityOrigin* origin)

This is probably my personal opinion, but to_string sounds to me like a conversion, and conversion methods normally return a temp value and the transfer is ownership. Here we are caching and returning a const char*, which I think is very convenient, but then I think as_string or simply get_string_representation like any other getter.

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.h:65
> +WEBKIT_API guint

guin16

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:114
> +    SET_INITIAL_NOTIFICATION_PERMISSIONS,

what about INITIALIZE_NOTIFICATION_PERMISSIONS for consistency with the web extensions initialization signal?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
> +     * This signal is emitted when a new web process is about to be
> +     * launched. It signals the most appropriate moment to use
> +     * webkit_web_context_set_initial_notification_permissions().

In the case of web extensions it makes sense to explain that this happens before the new web process is spawned, in this case I wouldn't say anything about processes, just that this is emitted when the web context needs to initialize notification permissions, and that it can be emitted multiple times.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1245
> +    String string = webkitSecurityOriginGetSecurityOrigin(origin).toString();
> +    if (string != "null")

It would be better to use webkit_security_origin_as_string() instead, since the value is cached and you would only need to check if it's nullptr. Also we don't need webkitSecurityOriginGetSecurityOrigin at all in such case.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1260
> + * webkit_web_context_set_initial_notification_permissions:

initialize_notification_permissions?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1288
> +    context->priv->notificationProvider->setNotificationPermissions(WTFMove(map));

Ok, I see now that we need to keep the permissions somewhere, either here in the context or in the provider, so it's ok to keep them in the provider. But then we should not transfer the ownership, because the user can call this at any moment and not handle the signal for example.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:760
> +}

I would also add tests for non HTTP uris, like file://

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:775
> +    Test::add("WebKitSecurityOrigin", "basic", testSecurityOriginBasic);

Why is this in TestWebKitWebContext.cpp?

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:-38
> -    assertObjectIsDeletedWhenTestFinishes(G_OBJECT(m_userContentManager.get()));

You can leave this here, the m_userContentManager is still initialized here.

[Carlos Garcia Campos]

Comment on attachment 297861 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:250
>> +const gchar* webkit_security_origin_to_string(WebKitSecurityOrigin* origin)
> 
> This is probably my personal opinion, but to_string sounds to me like a conversion, and conversion methods normally return a temp value and the transfer is ownership. Here we are caching and returning a const char*, which I think is very convenient, but then I think as_string or simply get_string_representation like any other getter.

the transfer is ownership? :-D I meant the ownership is transferred.

[Carlos Garcia Campos]

Comment on attachment 297861 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:235
> +    if (auto port = origin->securityOrigin->port())
> +        return *port;
> +    return 0;

We can simplify this by using value_or()

[Michael Catanzaro]

(In reply to comment #30)
> I think we can move security origin to its own patch/bug.

Probably wise, yes. I will post an updated patch responding to these comments in a different bug, then return to this notifications one once we've landed that. Thanks for reviewing.

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:59
> > +    return toAPI(toNotificationProvider(clientInfo)->notificationPermissions().leakRef());
> 
> Why leakRef()? If we are leaking our ref why do we keep a
> m_notificationPermissions member?

Hm, maybe that's just wrong. I would have added it in at some point a month ago when trying to make it not crash, but I suppose I need a paired unref somewhere? The to/from API casting is really confusing, and there's no docs so I have to dig through so many files to guess how it works. :(

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:167
> > +RefPtr<API::Dictionary> WebKitNotificationProvider::notificationPermissions()
> > +{
> > +    webkitWebContextSetInitialNotificationPermissions(m_webContext);
> > +    return m_notificationPermissions;
> 
> Do we really need to keep the m_notificationPermissions member? Could we
> make webkitWebContextSetInitialNotificationPermissions return a
> RefPtr<API:Dictionary> and simply return it here transferring the ownership
> to the caller?

(You discovered below that it's still needed.)

> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:173
> > +void WebKitNotificationProvider::setNotificationPermissions(HashMap<String, RefPtr<API::Object>>&& permissionsMap)
> > +{
> > +    m_notificationPermissions = API::Dictionary::create(WTFMove(permissionsMap));
> > +}
> 
> And we don't need this at all

Ditto.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:50
> > +    _WebKitSecurityOrigin(WebCore::SecurityOrigin* coreSecurityOrigin)
> 
> WebKitSecurityOrigin always wraps a WebCore::SecurityOrigin and all create
> methods return a Ref<>, so I think we could use a reference here, or even
> better a Ref<> and also use Ref<> instead of RefPtr for the member, but
> that's already the other constructor. since this one is not realy used in
> this patch, simply remove it, if we need it eventually, we can add it later.

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:51
> > +        : securityOrigin(coreSecurityOrigin)
> 
> There was a bug in my patch, referenceCount should be initialized to 1 here.

Um, good catch. I guess my tests passed because g_atomic_int_dec_and_test brings the refcount down to -1 in the typical case webkit_security_origin_ref is never called, then g_atomic_int_dec_and_test always returns FALSE and it's never freed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:56
> > +        : securityOrigin(WTFMove(coreSecurityOrigin))
> 
> And here.

I'll initialize it in the declaration, it doesn't need to be in any constructor.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:60
> > +    RefPtr<WebCore::SecurityOrigin> securityOrigin;
> 
> I think this could be Ref<>

Absolutely.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:74
> > +WebKitSecurityOrigin* webkitSecurityOriginCreate(WebCore::SecurityOrigin* coreSecurityOrigin)
> > +{
> > +    WebKitSecurityOrigin* origin = static_cast<WebKitSecurityOrigin*>(fastMalloc(sizeof(WebKitSecurityOrigin)));
> > +    new (origin) WebKitSecurityOrigin(coreSecurityOrigin);
> > +    return origin;
> > +}
> 
> This is unused in this patch, let's remove it for now, we can add it
> eventually if actually needed.

Yeah, I wondered about that but decided to leave it in. Probably a bad choice.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:110
> > +    if (port && WebCore::defaultPortForProtocol(protocol) != static_cast<uint16_t>(port))
> 
> Use guint16 instead of guint, then you don't need any cast here and it
> matches WebCore::SecurityOrigin. Here you are checking port != 0, so I
> assume you are considering that passing 0 as port means using the default.
> That should be document.

Believe it or not, I actually didn't know about guint16.

I'll document that.

> There's an interesting mix here. The method is called from_string, and it
> indeed uses the createFromString() method, but the parameter is uri, and
> documentation says you provide a URI indeed. String is supposed to be a
> string representation of a security origin (so, it doesn't make sense the
> doc saying that other uri components will be ignored, for example). I think
> what we want here is the URL constructor, I would use for instead of from
> too, webkit_security_origin_new_for_uri(). Instead of saying that components
> will be ignored we should document that a security origin represents, so
> it's not that the path/query/fragment is ignored, it's that we are creating
> the security origin corresponding to the given URI. And of course we should
> use create() method that receives a URL. We could create a SoupURI and
> g_return_val_if_fail if it's invalid instead of just checking it's not null.

Um, OK. End result will be the same either way. I agree that _for_uri is a better name than _from_string.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:223
> > + * Get the port of @origin. This function will always return 0 if the
> > + * port is the default port for the given protocol. For example,
> > + * http://example.com has the same security origin as
> > + * http://example.com:80, and this function will return 0 for a
> > + * #WebKitSecurityOrigin constructed from either string.
> 
> Exactly, that's what we should document in webkit_security_origin_new(),
> since we also allow to pass 0 there meaning the default. Maybe we could add
> a macro to make it more explicit
> 
> #define WEBKIT_SECURITY_ORIGIN_PORT_DEFAULT 0
> 
> Or something similar.

I don't think a macro is needed. I'll improve the documentation.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:244
> > + * Get a string representation of @origin. Be aware that this function
> > + * may return %NULL if the origin is unique. (For example, this will
> > + * occur if @origin represents a sandboxed iframe.)
> 
> So, we are introducing here the concept of unique origins. We should explain
> what a unique origin is (maybe not here, but in the section documention),
> and probably also add webkit_security_origin_is_unique(). We should also
> explain what the string representation is (like the url without the
> path/query/fragment parts).

Hm, maybe. It's not currently possible to have a unique origin because we never create WebKitSecurityOrigin objects inside WebKit, they're only used currently to pass in notification permissions, so they have to be created externally by the client and we don't expose API for creating a unique origin. So it feels odd adding a function that just always returns FALSE. But if WebKit ever returns WebKitSecurityOrigin objects that it creates itself, it could be useful. Maybe I should just add it now? Probably if exposing that, I should expose a comparison function webkit_security_origin_is_same_origin() as well? Even though these are not currently useful, they are easy to add could be in the future.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:246
> > + * Returns: (allow-none): a string representation of @origin.
> 
> , or %NULL if @origin is unique
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:250
> > +const gchar* webkit_security_origin_to_string(WebKitSecurityOrigin* origin)
> 
> This is probably my personal opinion, but to_string sounds to me like a
> conversion, and conversion methods normally return a temp value and the
> transfer is ownership. Here we are caching and returning a const char*,
> which I think is very convenient, but then I think as_string or simply
> get_string_representation like any other getter.

That's what I thought too, but I opened devhelp and searched for to_string and found that half the functions are transfer full and the other half transfer null, so GNOME at least has no such convention. I think I prefer the to_string() name, so maybe I should just make it transfer full. The copy will be often unnecessary but I agree it's probably better to transfer ownership.

> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.h:65
> > +WEBKIT_API guint
> 
> guin16

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:114
> > +    SET_INITIAL_NOTIFICATION_PERMISSIONS,
> 
> what about INITIALIZE_NOTIFICATION_PERMISSIONS for consistency with the web
> extensions initialization signal?

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
> > +     * This signal is emitted when a new web process is about to be
> > +     * launched. It signals the most appropriate moment to use
> > +     * webkit_web_context_set_initial_notification_permissions().
> 
> In the case of web extensions it makes sense to explain that this happens
> before the new web process is spawned, in this case I wouldn't say anything
> about processes, just that this is emitted when the web context needs to
> initialize notification permissions, and that it can be emitted multiple
> times.

I wondered about whether to mention this detail in the comments. I understand your argument that getting the permissions once per process launch is an implementation detail that could change. But it seems worthwhile to mention it here, because otherwise you might assume the signal is only emitted once, and do expensive work in it. For example, my (not very good yet) Epiphany implementation of this opens a file and synchronously reads permissions from it each time the signal is emitted. (Twice, I need to improve that. ;) But it could be emitted 50 or 100 or however many times when the application is started, once for each tab. So we probably want to mention that as a hint to the user to not do needlessly expensive stuff inside the signal handler.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1245
> > +    String string = webkitSecurityOriginGetSecurityOrigin(origin).toString();
> > +    if (string != "null")
> 
> It would be better to use webkit_security_origin_as_string() instead, since
> the value is cached and you would only need to check if it's nullptr. Also
> we don't need webkitSecurityOriginGetSecurityOrigin at all in such case.

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1260
> > + * webkit_web_context_set_initial_notification_permissions:
> 
> initialize_notification_permissions?

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1288
> > +    context->priv->notificationProvider->setNotificationPermissions(WTFMove(map));
> 
> Ok, I see now that we need to keep the permissions somewhere, either here in
> the context or in the provider, so it's ok to keep them in the provider. But
> then we should not transfer the ownership, because the user can call this at
> any moment and not handle the signal for example.

What do you mean "we should not transfer the ownership"? The map is never used again in this function, so it's moved into the Provider, which holds the ownership. What's wrong with that?

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:760
> > +}
> 
> I would also add tests for non HTTP uris, like file://

Probably a good idea, yes.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:775
> > +    Test::add("WebKitSecurityOrigin", "basic", testSecurityOriginBasic);
> 
> Why is this in TestWebKitWebContext.cpp?

Because you put it there. ;) I will move it to a new file.

> > Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebViewTest.cpp:-38
> > -    assertObjectIsDeletedWhenTestFinishes(G_OBJECT(m_userContentManager.get()));
> 
> You can leave this here, the m_userContentManager is still initialized here.

Good catch.

[Michael Catanzaro]

(In reply to comment #32)
> Comment on attachment 297861 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297861&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:235
> > +    if (auto port = origin->securityOrigin->port())
> > +        return *port;
> > +    return 0;
> 
> We can simplify this by using value_or()

Yes, good to know.

[Michael Catanzaro]

Should webkitSecurityOriginGetSecurityOrigin return a WebCore::SecurityOrigin& or a Ref<WebCore::SecurityOrigin>?

[Michael Catanzaro]

(In reply to comment #30)
> So, we are introducing here the concept of unique origins. We should explain
> what a unique origin is (maybe not here, but in the section documention),
> and probably also add webkit_security_origin_is_unique(). We should also
> explain what the string representation is (like the url without the
> path/query/fragment parts).

This is a hard decision. Eventually I decided we should not expose this yet as it's not currently possible to have a unique WebKitSecurityOrigin. If that were to be exposed, we would also want to start exposing more functions like is_same_origin and also the domain property.

[Michael Catanzaro]

(In reply to comment #36)
> This is a hard decision. Eventually I decided we should not expose this yet
> as it's not currently possible to have a unique WebKitSecurityOrigin.

Actually you can do it by passing a data URI to the URI constructor, but IMO that still doesn't justify exposing isUnique.

[Michael Catanzaro]

Patch in bug #166632.

[Carlos Garcia Campos]

(In reply to comment #33)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:244
> > > + * Get a string representation of @origin. Be aware that this function
> > > + * may return %NULL if the origin is unique. (For example, this will
> > > + * occur if @origin represents a sandboxed iframe.)
> > 
> > So, we are introducing here the concept of unique origins. We should explain
> > what a unique origin is (maybe not here, but in the section documention),
> > and probably also add webkit_security_origin_is_unique(). We should also
> > explain what the string representation is (like the url without the
> > path/query/fragment parts).
> 
> Hm, maybe. It's not currently possible to have a unique origin because we
> never create WebKitSecurityOrigin objects inside WebKit, they're only used
> currently to pass in notification permissions, so they have to be created
> externally by the client and we don't expose API for creating a unique
> origin. So it feels odd adding a function that just always returns FALSE.
> But if WebKit ever returns WebKitSecurityOrigin objects that it creates
> itself, it could be useful. Maybe I should just add it now? Probably if
> exposing that, I should expose a comparison function
> webkit_security_origin_is_same_origin() as well? Even though these are not
> currently useful, they are easy to add could be in the future.

If we don't want to expose is_unique(), then we shouldn't mention unique origins in the documentation. Actually, the string reprepsentation can null for non unique origins too, for example for local files when m_enforceFilePathSeparation is true. But again, that's not possible for security origins created by the user. With the current API creating a security origin for a URL that is considered unique, will create a unique origin, so it's indeed possible. Note that the user can register any scheme as no-access with webkit_security_manager_register_uri_scheme_as_no_access().

> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:246
> > > + * Returns: (allow-none): a string representation of @origin.
> > 
> > , or %NULL if @origin is unique
> > 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.cpp:250
> > > +const gchar* webkit_security_origin_to_string(WebKitSecurityOrigin* origin)
> > 
> > This is probably my personal opinion, but to_string sounds to me like a
> > conversion, and conversion methods normally return a temp value and the
> > transfer is ownership. Here we are caching and returning a const char*,
> > which I think is very convenient, but then I think as_string or simply
> > get_string_representation like any other getter.
> 
> That's what I thought too, but I opened devhelp and searched for to_string
> and found that half the functions are transfer full and the other half
> transfer null, so GNOME at least has no such convention. I think I prefer
> the to_string() name, so maybe I should just make it transfer full. The copy
> will be often unnecessary but I agree it's probably better to transfer
> ownership.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitSecurityOrigin.h:65
> > > +WEBKIT_API guint
> > 
> > guin16
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:114
> > > +    SET_INITIAL_NOTIFICATION_PERMISSIONS,
> > 
> > what about INITIALIZE_NOTIFICATION_PERMISSIONS for consistency with the web
> > extensions initialization signal?
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
> > > +     * This signal is emitted when a new web process is about to be
> > > +     * launched. It signals the most appropriate moment to use
> > > +     * webkit_web_context_set_initial_notification_permissions().
> > 
> > In the case of web extensions it makes sense to explain that this happens
> > before the new web process is spawned, in this case I wouldn't say anything
> > about processes, just that this is emitted when the web context needs to
> > initialize notification permissions, and that it can be emitted multiple
> > times.
> 
> I wondered about whether to mention this detail in the comments. I
> understand your argument that getting the permissions once per process
> launch is an implementation detail that could change. But it seems
> worthwhile to mention it here, because otherwise you might assume the signal
> is only emitted once, and do expensive work in it. For example, my (not very
> good yet) Epiphany implementation of this opens a file and synchronously
> reads permissions from it each time the signal is emitted. (Twice, I need to
> improve that. ;) But it could be emitted 50 or 100 or however many times
> when the application is started, once for each tab. So we probably want to
> mention that as a hint to the user to not do needlessly expensive stuff
> inside the signal handler.

Good point. Also the signal can't be handled asynchronously, you are supposed to call the initialize_method in the handler.

> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1245
> > > +    String string = webkitSecurityOriginGetSecurityOrigin(origin).toString();
> > > +    if (string != "null")
> > 
> > It would be better to use webkit_security_origin_as_string() instead, since
> > the value is cached and you would only need to check if it's nullptr. Also
> > we don't need webkitSecurityOriginGetSecurityOrigin at all in such case.
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1260
> > > + * webkit_web_context_set_initial_notification_permissions:
> > 
> > initialize_notification_permissions?
> 
> OK.
> 
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1288
> > > +    context->priv->notificationProvider->setNotificationPermissions(WTFMove(map));
> > 
> > Ok, I see now that we need to keep the permissions somewhere, either here in
> > the context or in the provider, so it's ok to keep them in the provider. But
> > then we should not transfer the ownership, because the user can call this at
> > any moment and not handle the signal for example.
> 
> What do you mean "we should not transfer the ownership"? The map is never
> used again in this function, so it's moved into the Provider, which holds
> the ownership. What's wrong with that?

I meant in the provider, the leakReaf() thing.

> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:760
> > > +}
> > 
> > I would also add tests for non HTTP uris, like file://
> 
> Probably a good idea, yes.
> 
> > > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebContext.cpp:775
> > > +    Test::add("WebKitSecurityOrigin", "basic", testSecurityOriginBasic);
> > 
> > Why is this in TestWebKitWebContext.cpp?
> 
> Because you put it there. ;) I will move it to a new file.

:-D

[Michael Catanzaro]

(In reply to comment #30)
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1245
> > +    String string = webkitSecurityOriginGetSecurityOrigin(origin).toString();
> > +    if (string != "null")
> 
> It would be better to use webkit_security_origin_as_string() instead, since
> the value is cached and you would only need to check if it's nullptr. Also
> we don't need webkitSecurityOriginGetSecurityOrigin at all in such case.

In the end, I didn't add webkitSecurityOriginGetSecurityOrigin in the previous patch, but I still want to add it here to avoid using webkit_security_origin_to_string. One disadvantage of using the external API function is the unnecessary roundtrip encoding conversion from WTF::String (UTF-16) -> API UTF-8 -> back to :String (UTF-16). Also, since it's no longer transfer none, String can avoid heap memory allocation if the string is sufficiently-small. I imagine the performance characteristics are irrelevant, though: it's just nicer to use String instead of GUniquePtr<char>.

[Michael Catanzaro]

(In reply to comment #30) 
> Why leakRef()? If we are leaking our ref why do we keep a
> m_notificationPermissions member?

Chatted about this. Agreed it's a returned temporary RefPtr and the goal is to transfer ownership, so that's right. It might even be what I was thinking when I wrote it!

[Michael Catanzaro]

Created attachment 297957 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 297957 [details]
Patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:45
> +    RefPtr<API::Dictionary> notificationPermissions();

I think this could be const.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
> +     * This signal is emitted when a new web process is about to be
> +     * launched. It signals the most appropriate moment to use
> +     * webkit_web_context_initialize_notification_permissions().

I would mention here that if you don't call webkit_web_context_initialize_notification_permissions() again, the permissions set previously will be used. That way apps can optimize and do nothing if nothing changed in their side. And as I said in previous review, I would start saying this signal is emitted when the web context needs to initialize the permissions, and then you can say this happens when a new process is about to be launched.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1257
> +static void addAllowedOriginToMap(WebKitSecurityOrigin* origin, HashMap<String, RefPtr<API::Object>>* map)
> +{
> +    addOriginToMap(origin, map, true);
> +}
> +
> +static void addDisallowedOriginToMap(WebKitSecurityOrigin* origin, HashMap<String, RefPtr<API::Object>>* map)
> +{
> +    addOriginToMap(origin, map, false);
> +}

These can be lambdas

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1283
> +void webkit_web_context_initialize_notification_permissions(WebKitWebContext* context, GList* allowed_origins, GList* disallowed_origins)

allowed_origins -> allowedOrigins
disallowed_origins -> disallowedOrigins

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1287
> +    g_list_foreach(allowed_origins, (GFunc)addAllowedOriginToMap, &map);
> +    g_list_foreach(disallowed_origins, (GFunc)addDisallowedOriginToMap, &map);

Do not use C style casts here. You can use lambdas instead and cast the userData in the body.

g_list_foreach(allowedOrigins, [](gpointer data, gpointer userData) {
    addOriginToMap(static_cast<WebKitSecurityOrigin*>(origin), static_cast<HashMap<String, RefPtr<API::Object>>*>(map), true);
}, &map);

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:848
> +    GList* allowedOrigins = nullptr;

GList* allowedOrigins = g_list_prepend(nullptr, webkit_security_origin_new_for_uri());

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:849
> +    allowedOrigins = g_list_append(allowedOrigins, webkit_security_origin_new_for_uri(gServer->securityOrigin().data()));

Why do you need gServer->securityOrigin()? You can pass the base uri and webkit_security_origin_new_for_uri will do the magic

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:851
> +    g_list_free_full(allowedOrigins, (GDestroyNotify)webkit_security_origin_unref);

Do not use C style casts

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:856
> +    GList* disallowedOrigins = nullptr;

GList* disallowedOrigins = g_list_prepend(nullptr, webkit_security_origin_new_for_uri());

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:857
> +    disallowedOrigins = g_list_append(disallowedOrigins, webkit_security_origin_new_for_uri(gServer->securityOrigin().data()));

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:859
> +    g_list_free_full(disallowedOrigins, (GDestroyNotify)webkit_security_origin_unref);

Do not use C style casts

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:873
> +    static const char* title = "This is a notification";
> +    static const char* body = "This is the body.";
> +    test->requestNotificationAndWaitUntilShown(title, body);

I guess we can probably pass the string literals directly, we don't really need the variables, no?

> Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebKitTestServer.cpp:76
> +    // The security origin is the base URI minus the trailing slash.
> +    GUniquePtr<gchar> uriString(soup_uri_to_string(m_baseURI, FALSE));
> +    uriString.get()[strlen(uriString.get()) - 1] = '\0';
> +    return uriString.get();

Why are we reinventing the security origin here? I think we can either remove this and pass the base uri to webkit_security_origin_new_for_uri(), or make this return a WebKitSecurityOrigin and simply call webkit_security_origin_new_for_uri() with the base uri.

[Michael Catanzaro]

(In reply to comment #43)
> Comment on attachment 297957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297957&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:45
> > +    RefPtr<API::Dictionary> notificationPermissions();
> 
> I think this could be const.

No, because the return value is non-const, because that's what the C API requires. I would make it const if we were returning a RefPtr<const API::Dictionary>, but we can't do that.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:398
> > +     * This signal is emitted when a new web process is about to be
> > +     * launched. It signals the most appropriate moment to use
> > +     * webkit_web_context_initialize_notification_permissions().
> 
> I would mention here that if you don't call
> webkit_web_context_initialize_notification_permissions() again, the
> permissions set previously will be used. That way apps can optimize and do
> nothing if nothing changed in their side. And as I said in previous review,
> I would start saying this signal is emitted when the web context needs to
> initialize the permissions, and then you can say this happens when a new
> process is about to be launched.

OK.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1257
> > +static void addAllowedOriginToMap(WebKitSecurityOrigin* origin, HashMap<String, RefPtr<API::Object>>* map)
> > +{
> > +    addOriginToMap(origin, map, true);
> > +}
> > +
> > +static void addDisallowedOriginToMap(WebKitSecurityOrigin* origin, HashMap<String, RefPtr<API::Object>>* map)
> > +{
> > +    addOriginToMap(origin, map, false);
> > +}
> 
> These can be lambdas

Think I tried that first. I'll try harder then.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1283
> > +void webkit_web_context_initialize_notification_permissions(WebKitWebContext* context, GList* allowed_origins, GList* disallowed_origins)
> 
> allowed_origins -> allowedOrigins
> disallowed_origins -> disallowedOrigins

Oops, surprised style checker did not complain.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:1287
> > +    g_list_foreach(allowed_origins, (GFunc)addAllowedOriginToMap, &map);
> > +    g_list_foreach(disallowed_origins, (GFunc)addDisallowedOriginToMap, &map);
> 
> Do not use C style casts here. You can use lambdas instead and cast the
> userData in the body.
> 
> g_list_foreach(allowedOrigins, [](gpointer data, gpointer userData) {
>     addOriginToMap(static_cast<WebKitSecurityOrigin*>(origin),
> static_cast<HashMap<String, RefPtr<API::Object>>*>(map), true);
> }, &map);

Much nicer.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:848
> > +    GList* allowedOrigins = nullptr;
> 
> GList* allowedOrigins = g_list_prepend(nullptr,
> webkit_security_origin_new_for_uri());

OK.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:849
> > +    allowedOrigins = g_list_append(allowedOrigins, webkit_security_origin_new_for_uri(gServer->securityOrigin().data()));
> 
> Why do you need gServer->securityOrigin()? You can pass the base uri and
> webkit_security_origin_new_for_uri will do the magic

Oops, yes, I wrote this before you suggested I add WebKitSecurityOrigin.

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:851
> > +    g_list_free_full(allowedOrigins, (GDestroyNotify)webkit_security_origin_unref);
> 
> Do not use C style casts

OK

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:856
> > +    GList* disallowedOrigins = nullptr;
> 
> GList* disallowedOrigins = g_list_prepend(nullptr,
> webkit_security_origin_new_for_uri());

OK

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:857
> > +    disallowedOrigins = g_list_append(disallowedOrigins, webkit_security_origin_new_for_uri(gServer->securityOrigin().data()));
> 
> Ditto.

OK

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:859
> > +    g_list_free_full(disallowedOrigins, (GDestroyNotify)webkit_security_origin_unref);
> 
> Do not use C style casts

OK

> > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:873
> > +    static const char* title = "This is a notification";
> > +    static const char* body = "This is the body.";
> > +    test->requestNotificationAndWaitUntilShown(title, body);
> 
> I guess we can probably pass the string literals directly, we don't really
> need the variables, no?

We could, yes.

> > Tools/TestWebKitAPI/gtk/WebKit2Gtk/WebKitTestServer.cpp:76
> > +    // The security origin is the base URI minus the trailing slash.
> > +    GUniquePtr<gchar> uriString(soup_uri_to_string(m_baseURI, FALSE));
> > +    uriString.get()[strlen(uriString.get()) - 1] = '\0';
> > +    return uriString.get();
> 
> Why are we reinventing the security origin here? I think we can either
> remove this and pass the base uri to webkit_security_origin_new_for_uri(),
> or make this return a WebKitSecurityOrigin and simply call
> webkit_security_origin_new_for_uri() with the base uri.

Indeed.

[Michael Catanzaro]

Committed r210574: <http://trac.webkit.org/changeset/210574>