Bug 191585 – [GTK][MiniBrowser] Handle Device Info permission requests

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

[GTK][MiniBrowser] Handle Device Info permission requests

[Thibault Saunier]

Created attachment 354668 [details]
Patch

[EWS Watchlist]

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

[Thibault Saunier]

Created attachment 354963 [details]
Patch for landing

[Adrian Perez]

Comment on attachment 354668 [details]
Patch

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

Informal review here… Code LGTM, thanks for the patch! I have only
a few small nits.

In particular, please don't worry much about the “g_clear_*” comments:
I have grown fond of them and I like recommending their usage —alongside
“g_auto*” and “g_steal_pointer()”— to make people aware they exist
in GLib. I will leave it to you to choose whether you want to use them
or not ;-)

> Tools/MiniBrowser/gtk/BrowserTab.c:72
> +PermissionRequestData *permissionRequestDataNew(BrowserTab *tab, WebKitPermissionRequest *request)

This function is used only in this file, please add “static” in its prototype:

   static PermissionRequestData *permissionRequestDataNew(...)

> Tools/MiniBrowser/gtk/BrowserTab.c:74
> +    PermissionRequestData *data = g_malloc0(sizeof(PermissionRequestData));

Here I would rather use “g_new0(PermissionRequestData, 1)”, or even
better: “g_slice_new0(PermissionRequestData)”.

> Tools/MiniBrowser/gtk/BrowserTab.c:82
> +void permissionRequestDataFree(PermissionRequestData *data)

Ditto, please add “static” to the function prototype.

> Tools/MiniBrowser/gtk/BrowserTab.c:85
> +    g_object_unref(data->request);

How about using “g_clear_object()” instead of “g_object_unref()” here?
That will also set the pointers to NULL which makes use-after-free easier
to catch.

> Tools/MiniBrowser/gtk/BrowserTab.c:86
> +    g_free(data);

If you end up using “g_slice_new0()” above, please do change this
to use “g_slice_free()” accordingly ;-)

> Tools/MiniBrowser/gtk/BrowserTab.c:216
> +    permissionRequestDataFree(requestData);

If you feel like embracing the “g_clear_*” family of functions, this could
be changed to “g_clear_pointer(&requestData, permissionRequestDataFree)”
as well.

> Tools/MiniBrowser/gtk/BrowserTab.c:250
> +        g_print("%s request not handled\n", G_OBJECT_TYPE_NAME(request));

Is this an error, a warning, or a debugging hint for a developer?
I would consider rather using “g_printerr()”, even “g_warning()”,
“g_error()”, or “g_debug()” depending on whom this message is
intended for.

[Alejandro G. Castro]

Comment on attachment 354668 [details]
Patch

LGTM, but my biggest concern about the patch is if someone copies this code because they think this is the proper way to grant permissions, because we are adding just 1 variable for every origin. Basically if we grant foo.com we are granting bar.com to use our camera and mic information. Maybe some comment saying: Don't do this at home! :-). Michael complained about this in some other patch, I'll let him confirm the patch.

[Thibault Saunier]

(In reply to Alejandro G. Castro from comment #5)
> Comment on attachment 354668 [details]
> Patch
> 
> LGTM, but my biggest concern about the patch is if someone copies this code
> because they think this is the proper way to grant permissions, because we
> are adding just 1 variable for every origin. Basically if we grant foo.com
> we are granting bar.com to use our camera and mic information. Maybe some
> comment saying: Don't do this at home! :-). Michael complained about this in
> some other patch, I'll let him confirm the patch.

Indeed, it is not an example to follow, but I think it is good enough for the MiniBrowser case.

[Carlos Garcia Campos]

Comment on attachment 354668 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/webkit2.h:41
> +#include <webkit2/WebKitDeviceInfoPermissionRequest.h>

This doesn't belong to this patch. You can even land it unreviewed, but use a different patch.

> Tools/MiniBrowser/gtk/BrowserTab.c:204
> +            requestData->tab->userMediaAccessWasGranted = TRUE;

I would use a hash table and store this per origin.

> Tools/MiniBrowser/gtk/BrowserTab.c:248
> +    } else if (WEBKIT_IS_DEVICE_INFO_PERMISSION_REQUEST(request))
> +        return tab->userMediaAccessWasGranted;

I don't understand this. Why do we persist user media only? and why user media automatically grants device info requests?

> Tools/MiniBrowser/gtk/BrowserTab.c:428
> +    tab->userMediaAccessWasGranted = FALSE;

This is already FALSE, it's filled with 0 on allocation.

[Thibault Saunier]

Created attachment 355040 [details]
Patch

[Thibault Saunier]

(In reply to Adrian Perez from comment #4)
> Comment on attachment 354668 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> 
> Informal review here… Code LGTM, thanks for the patch! I have only
> a few small nits.
> 
> In particular, please don't worry much about the “g_clear_*” comments:
> I have grown fond of them and I like recommending their usage —alongside
> “g_auto*” and “g_steal_pointer()”— to make people aware they exist
> in GLib. I will leave it to you to choose whether you want to use them
> or not ;-)

Yeah I know about them, `g_auto*` but it is forbidden in Gst because it is incompatible
with msvc is it OK to use it in WebKit? (Also it is Since 2.44) `g_clear_` doesn't make much sense in finalizing
function to me.

> > Tools/MiniBrowser/gtk/BrowserTab.c:72
> > +PermissionRequestData *permissionRequestDataNew(BrowserTab *tab, WebKitPermissionRequest *request)
> 
> This function is used only in this file, please add “static” in its
> prototype:
> 
>    static PermissionRequestData *permissionRequestDataNew(...)

OOps, sorry about that, any reason why I didn't get a compiler warning?

Fixed.

> > Tools/MiniBrowser/gtk/BrowserTab.c:74
> > +    PermissionRequestData *data = g_malloc0(sizeof(PermissionRequestData));
> 
> Here I would rather use “g_new0(PermissionRequestData, 1)”, or even
> better: “g_slice_new0(PermissionRequestData)”.

Well, g_slice is a candidate for deprecation iiuc (we decided to stop using in Gst), I am not
sure what g_new0 would bring compare to a plain g_malloc0?

> > Tools/MiniBrowser/gtk/BrowserTab.c:82
> > +void permissionRequestDataFree(PermissionRequestData *data)
> 
> Ditto, please add “static” to the function prototype.

Fixed.

> > Tools/MiniBrowser/gtk/BrowserTab.c:85
> > +    g_object_unref(data->request);
> 
> How about using “g_clear_object()” instead of “g_object_unref()” here?
> That will also set the pointers to NULL which makes use-after-free easier
> to catch.

Well, I am freeing the whole structure right after, anyway FIXED.
 
> > Tools/MiniBrowser/gtk/BrowserTab.c:86
> > +    g_free(data);
> 
> If you end up using “g_slice_new0()” above, please do change this
> to use “g_slice_free()” accordingly ;-)
> 
> > Tools/MiniBrowser/gtk/BrowserTab.c:216
> > +    permissionRequestDataFree(requestData);
> 
> If you feel like embracing the “g_clear_*” family of functions, this could
> be changed to “g_clear_pointer(&requestData, permissionRequestDataFree)”
> as well.

Done.
  
> > Tools/MiniBrowser/gtk/BrowserTab.c:250
> > +        g_print("%s request not handled\n", G_OBJECT_TYPE_NAME(request));
> 
> Is this an error, a warning, or a debugging hint for a developer?
> I would consider rather using “g_printerr()”, even “g_warning()”,
> “g_error()”, or “g_debug()” depending on whom this message is
> intended for.

It is just an information to the user/developer (I guess those are the same for MiniBrowser
usually), I just did the same as I did in wpe/MiniBrowse. printerr makes me think it is an error,
and `g_debug` would not be visible almost always, and I think it is an interesting piece of
information in many cases, that being said, I will do whatever you prefer.


Thanks for the review!

[Thibault Saunier]

(In reply to Carlos Garcia Campos from comment #7)
> Comment on attachment 354668 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> 
> > Source/WebKit/UIProcess/API/gtk/webkit2.h:41
> > +#include <webkit2/WebKitDeviceInfoPermissionRequest.h>
> 
> This doesn't belong to this patch. You can even land it unreviewed, but use
> a different patch.

Why would that be?

> > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > +            requestData->tab->userMediaAccessWasGranted = TRUE;
> 
> I would use a hash table and store this per origin.

OK, I will have a look

> > Tools/MiniBrowser/gtk/BrowserTab.c:248
> > +    } else if (WEBKIT_IS_DEVICE_INFO_PERMISSION_REQUEST(request))
> > +        return tab->userMediaAccessWasGranted;
> 
> I don't understand this. Why do we persist user media only? and why user
> media automatically grants device info requests?

This is what the standard says.

> > Tools/MiniBrowser/gtk/BrowserTab.c:428
> > +    tab->userMediaAccessWasGranted = FALSE;
> 
> This is already FALSE, it's filled with 0 on allocation.

Well, making sure it is clear it is on purpose?

[Carlos Garcia Campos]

(In reply to Thibault Saunier from comment #10)
> (In reply to Carlos Garcia Campos from comment #7)
> > Comment on attachment 354668 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> > 
> > > Source/WebKit/UIProcess/API/gtk/webkit2.h:41
> > > +#include <webkit2/WebKitDeviceInfoPermissionRequest.h>
> > 
> > This doesn't belong to this patch. You can even land it unreviewed, but use
> > a different patch.
> 
> Why would that be?

This is a patch for MiniBrowser. That header should have been included in the main header when it was added to the API. So the change is a fix for a previous commit, it doesn't belong to this bug.

> > > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > > +            requestData->tab->userMediaAccessWasGranted = TRUE;
> > 
> > I would use a hash table and store this per origin.
> 
> OK, I will have a look
> 
> > > Tools/MiniBrowser/gtk/BrowserTab.c:248
> > > +    } else if (WEBKIT_IS_DEVICE_INFO_PERMISSION_REQUEST(request))
> > > +        return tab->userMediaAccessWasGranted;
> > 
> > I don't understand this. Why do we persist user media only? and why user
> > media automatically grants device info requests?
> 
> This is what the standard says.

If that's part of a standard it should be done by WebKit I would say. API users shouldn't need to know the standards. Why do we expose device info permission request if it depends on user media requests? Why don't we reuse the value to not ask the user the second time user media permission is requested? Is that done by WebKit already?

> > > Tools/MiniBrowser/gtk/BrowserTab.c:428
> > > +    tab->userMediaAccessWasGranted = FALSE;
> > 
> > This is already FALSE, it's filled with 0 on allocation.
> 
> Well, making sure it is clear it is on purpose?

You don't need to make sure, it's FALSE already.

[Thibault Saunier]

(In reply to Carlos Garcia Campos from comment #11)
> (In reply to Thibault Saunier from comment #10)
> > (In reply to Carlos Garcia Campos from comment #7)
> > > Comment on attachment 354668 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/gtk/webkit2.h:41
> > > > +#include <webkit2/WebKitDeviceInfoPermissionRequest.h>
> > > 
> > > This doesn't belong to this patch. You can even land it unreviewed, but use
> > > a different patch.
> > 
> > Why would that be?
> 
> This is a patch for MiniBrowser. That header should have been included in
> the main header when it was added to the API. So the change is a fix for a
> previous commit, it doesn't belong to this bug.

OK, people tend to make big commits touching many things in WebKit from what I can see, anyway... let's do that in a separate commit.

> > > > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > > > +            requestData->tab->userMediaAccessWasGranted = TRUE;
> > > 
> > > I would use a hash table and store this per origin.
> > 
> > OK, I will have a look
> > 
> > > > Tools/MiniBrowser/gtk/BrowserTab.c:248
> > > > +    } else if (WEBKIT_IS_DEVICE_INFO_PERMISSION_REQUEST(request))
> > > > +        return tab->userMediaAccessWasGranted;
> > > 
> > > I don't understand this. Why do we persist user media only? and why user
> > > media automatically grants device info requests?
> > 
> > This is what the standard says.
> 
> If that's part of a standard it should be done by WebKit I would say. API
> users shouldn't need to know the standards. Why do we expose device info
> permission request if it depends on user media requests? Why don't we reuse
> the value to not ask the user the second time user media permission is
> requested? Is that done by WebKit already?

Well afaiu user permission is the browser responsibility since the beginning, and that is a design choice that has been made again recently agreed with many people (ask Alex for the details, I didn't follow the conversation so closely tbh).

> > > > Tools/MiniBrowser/gtk/BrowserTab.c:428
> > > > +    tab->userMediaAccessWasGranted = FALSE;
> > > 
> > > This is already FALSE, it's filled with 0 on allocation.
> > 
> > Well, making sure it is clear it is on purpose?
> 
> You don't need to make sure, it's FALSE already.

I am making it clear it is on purpose, I know GObjects are 0 initialized. Now, that code changed anyway.

[Carlos Garcia Campos]

(In reply to Thibault Saunier from comment #12)
> (In reply to Carlos Garcia Campos from comment #11)
> > (In reply to Thibault Saunier from comment #10)
> > > (In reply to Carlos Garcia Campos from comment #7)
> > > > Comment on attachment 354668 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> > > > 
> > > > > Source/WebKit/UIProcess/API/gtk/webkit2.h:41
> > > > > +#include <webkit2/WebKitDeviceInfoPermissionRequest.h>
> > > > 
> > > > This doesn't belong to this patch. You can even land it unreviewed, but use
> > > > a different patch.
> > > 
> > > Why would that be?
> > 
> > This is a patch for MiniBrowser. That header should have been included in
> > the main header when it was added to the API. So the change is a fix for a
> > previous commit, it doesn't belong to this bug.
> 
> OK, people tend to make big commits touching many things in WebKit from what
> I can see, anyway... let's do that in a separate commit.

It's not a matter of touching files, sometimes you need to change several layers for a single commit. But adding a missing header to the main header in the public API doesn't belong to a commit adding support for a feature in MiniBrowser.

> > > > > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > > > > +            requestData->tab->userMediaAccessWasGranted = TRUE;
> > > > 
> > > > I would use a hash table and store this per origin.
> > > 
> > > OK, I will have a look
> > > 
> > > > > Tools/MiniBrowser/gtk/BrowserTab.c:248
> > > > > +    } else if (WEBKIT_IS_DEVICE_INFO_PERMISSION_REQUEST(request))
> > > > > +        return tab->userMediaAccessWasGranted;
> > > > 
> > > > I don't understand this. Why do we persist user media only? and why user
> > > > media automatically grants device info requests?
> > > 
> > > This is what the standard says.
> > 
> > If that's part of a standard it should be done by WebKit I would say. API
> > users shouldn't need to know the standards. Why do we expose device info
> > permission request if it depends on user media requests? Why don't we reuse
> > the value to not ask the user the second time user media permission is
> > requested? Is that done by WebKit already?
> 
> Well afaiu user permission is the browser responsibility since the
> beginning, and that is a design choice that has been made again recently
> agreed with many people (ask Alex for the details, I didn't follow the
> conversation so closely tbh).

I'm not objecting nor complaining, just asking because I find it confusing. User permission is browser responsibility, but if the device permission depends on the media one according to the standard, shouldn't be enough to ask for user media?

> > > > > Tools/MiniBrowser/gtk/BrowserTab.c:428
> > > > > +    tab->userMediaAccessWasGranted = FALSE;
> > > > 
> > > > This is already FALSE, it's filled with 0 on allocation.
> > > 
> > > Well, making sure it is clear it is on purpose?
> > 
> > You don't need to make sure, it's FALSE already.
> 
> I am making it clear it is on purpose, I know GObjects are 0 initialized.
> Now, that code changed anyway.

[Thibault Saunier]

Created attachment 355046 [details]
Patch

[Thibault Saunier]

> It's not a matter of touching files, sometimes you need to change several layers for a single commit. But adding a missing header to the main header in the public API doesn't belong to a commit adding support for a feature in MiniBrowser.

Got it, though that header was missing so that feature was implemented, so I actually needed to touch that other layer for this particular commit </stupidly pedantic>

> I'm not objecting nor complaining, just asking because I find it confusing. User permission is browser responsibility, but if the device permission depends on the media one according to the standard, shouldn't be enough to ask for user media?

Intuitively I agree, now I guess they had good reason to do it the way it is :-)

[Thibault Saunier]

> > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > +            requestData->tab->userMediaAccessWasGranted = TRUE;
> 
> I would use a hash table and store this per origin.

This is now done.

[Thibault Saunier]

Created attachment 355052 [details]
Patch for landing

[Thibault Saunier]

(In reply to Thibault Saunier from comment #17)
> Created attachment 355052 [details]
> Patch for landing

Sorry, wrong invocation, I removed the c+ from it and added r+

[Thibault Saunier]

(In reply to Thibault Saunier from comment #18)
> (In reply to Thibault Saunier from comment #17)
> > Created attachment 355052 [details]
> > Patch for landing
> 
> Sorry, wrong invocation, I removed the c+ from it and added r+

And by r+ I meant r?

[Alejandro G. Castro]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Thibault Saunier from comment #12)
> > Well afaiu user permission is the browser responsibility since the
> > beginning, and that is a design choice that has been made again recently
> > agreed with many people (ask Alex for the details, I didn't follow the
> > conversation so closely tbh).
> 
> I'm not objecting nor complaining, just asking because I find it confusing.
> User permission is browser responsibility, but if the device permission
> depends on the media one according to the standard, shouldn't be enough to
> ask for user media?
> 

In summary we tried to implement it in the engine, Apple agreed, but we failed because it would mean moving the permission managers of safari, epiphany or any other browser inside WebKit, and no one wanted that at this point. All the logic of these specs is very complex, we need to differenciate the operations because the spec says we have to update it when the multimedia is modified, but the user can change it using the permission manager we don't control that, the behaviour is also different when the user still did not ask for the getUserMedia the first time, as you can see in the patch when the engine asks for the device-info the browser does not ask the user. Anyway, it is true all these permissions are confusing.

[Adrian Perez]

Just a couple of follow-up comments — thought there is nothing to
change further I think, specially now that the patch is already landed :)

(In reply to Thibault Saunier from comment #9)
> (In reply to Adrian Perez from comment #4)
> > Comment on attachment 354668 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=354668&action=review
> > 
> > Informal review here… Code LGTM, thanks for the patch! I have only
> > a few small nits.
> > 
> > In particular, please don't worry much about the “g_clear_*” comments:
> > I have grown fond of them and I like recommending their usage —alongside
> > “g_auto*” and “g_steal_pointer()”— to make people aware they exist
> > in GLib. I will leave it to you to choose whether you want to use them
> > or not ;-)
> 
> Yeah I know about them, `g_auto*` but it is forbidden in Gst because it is
> incompatible
> with msvc is it OK to use it in WebKit? (Also it is Since 2.44) `g_clear_`
> doesn't make much sense in finalizing
> function to me.

I am aware that they are not supported in MSVC, that's why I was telling
that I like to recommend them, but didn't ask you to begin using them.
Sorry if I wasn't too clear with that ;-)

About “g_clear_*” in this small patch it is quite clear reading the code
that there won't be use-after-free situations, so feel free to leave it
as-is — but IMHO it is better to be consistently disciplined and always
using them.

> > > Tools/MiniBrowser/gtk/BrowserTab.c:72
> > > +PermissionRequestData *permissionRequestDataNew(BrowserTab *tab, WebKitPermissionRequest *request)
> > 
> > This function is used only in this file, please add “static” in its
> > prototype:
> > 
> >    static PermissionRequestData *permissionRequestDataNew(...)
> 
> OOps, sorry about that, any reason why I didn't get a compiler warning?
>
> Fixed.

The compiler does not warn because it assumes that the functions
have been declared “extern” somewhere else and they might be used
by code built in other compilation units ¯\_(ツ)_/¯
 
> > > Tools/MiniBrowser/gtk/BrowserTab.c:74
> > > +    PermissionRequestData *data = g_malloc0(sizeof(PermissionRequestData));
> > 
> > Here I would rather use “g_new0(PermissionRequestData, 1)”, or even
> > better: “g_slice_new0(PermissionRequestData)”.
> 
> Well, g_slice is a candidate for deprecation iiuc (we decided to stop using
> in Gst), I am not
> sure what g_new0 would bring compare to a plain g_malloc0?

The most concrete discussion I have seen are those:

   https://gitlab.gnome.org/GNOME/glib/issues/1079
   https://gitlab.gnome.org/GNOME/glib/issues/1278

and what I interpret is that the GSlice API will be still around, but
internally GLib may just not use its own allocator depending on the
characteristics of the system. Also, the system allocator can be
painfully slow in Windows (its performance varies wildly between
versions, too!) so there *is* still value in using the API.

> [...]
>
> It is just an information to the user/developer (I guess those are the same
> for MiniBrowser
> usually), I just did the same as I did in wpe/MiniBrowse. printerr makes me
> think it is an error,
> and `g_debug` would not be visible almost always, and I think it is an
> interesting piece of
> information in many cases, that being said, I will do whatever you prefer.

Right, then “g_message()” is always shown — but “g_print()” is okay as well.
No need to change anything now that the patch has been landed :)

[Adrian Perez]

(In reply to Adrian Perez from comment #21)
> Just a couple of follow-up comments — thought there is nothing to
> change further I think, specially now that the patch is already landed :)

I meant: “… is already going to be landed” O:-)

[Thibault Saunier]

> I meant: “… is already going to be landed” O:-)

Is it?

For the GSlice discussion, it is a bit of a mess, afaiu it has been recomanded lately to not use it, tbh I am not sure and in that particular case, well, it should really not matter anyway :D

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #11)
> This is a patch for MiniBrowser. That header should have been included in
> the main header when it was added to the API. So the change is a fix for a
> previous commit, it doesn't belong to this bug.

And make sure to update the WPE header at the same time.

[Alejandro G. Castro]

Comment on attachment 355052 [details]
Patch for landing

LGTM, just one small proposal, could we add the hashtable to some global variable of the browser instead of doing it per tab? It can simplify the patch and I think it is more similar to what a real browser should be doing.

[Thibault Saunier]

(In reply to Alejandro G. Castro from comment #25)
> Comment on attachment 355052 [details]
> Patch for landing
> 
> LGTM, just one small proposal, could we add the hashtable to some global
> variable of the browser instead of doing it per tab? It can simplify the
> patch and I think it is more similar to what a real browser should be doing.

Well, in a real browser I think you would allow for the tab only, and potentially save on disk, which could enable for more tabs, so I think current behaviour is what a real browser would do if it doesn't have a way to save the info on disk, don't you think?

[Thibault Saunier]

OK, what should I do in the end?

[Thibault Saunier]

Created attachment 355466 [details]
Patch

[Thibault Saunier]

Moved the `userMediaAccessPermissions` HashTable to be global.

[Carlos Garcia Campos]

Comment on attachment 355466 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserTab.c:59
> +static GHashTable *userMediaAccessPermissions;

This is a set of origins for which we granted user media permission, right? What about userMediaPermissionGrantedOrigins or something like that? current name sounds like if it's storing permission requests.

> Tools/MiniBrowser/gtk/BrowserTab.c:204
> +

Remove this empty line...

> Tools/MiniBrowser/gtk/BrowserTab.c:210
> +            g_hash_table_remove(userMediaAccessPermissions, requestData->origin);
> +        webkit_permission_request_deny(requestData->request);

... or add one here.

> Tools/MiniBrowser/gtk/BrowserTab.c:215
> -    g_object_unref(request);
> +    g_clear_pointer(&requestData, permissionRequestDataFree);

requestData is a parameter here, setting it to NULL doesn't make sense, so I would just call permissionRequestDataFree()

> Tools/MiniBrowser/gtk/BrowserTab.c:249
> +        if (g_hash_table_contains(userMediaAccessPermissions, webkit_web_view_get_uri(webView))) {
> +            webkit_permission_request_allow(request);
> +            return TRUE;

Should we also use the set for user media requests? to avoid asking for the same permission twice? or is that already cached by WebKit itself? If that's the case, I still don't understand why this isn't done by WebKit automatically.

> Tools/MiniBrowser/gtk/BrowserTab.c:252
> +        webkit_permission_request_deny(request);
>          return FALSE;

This should be TRUE since you handled the request.

> Tools/MiniBrowser/gtk/BrowserTab.c:261
> -    g_signal_connect(dialog, "response", G_CALLBACK(permissionRequestDialogResponse), g_object_ref(request));
> +    g_signal_connect(dialog, "response", G_CALLBACK(permissionRequestDialogResponse), permissionRequestDataNew(request,
> +        webkit_web_view_get_uri(webView)));

I'm confused here, permissionRequestDataNew expects an origin, but we are passing the URI here.

> Tools/MiniBrowser/gtk/BrowserTab.c:442
> +    if (!userMediaAccessPermissions)
> +        userMediaAccessPermissions = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL);

I don't think you need the check here, class init is called once.

[Thibault Saunier]

(In reply to Carlos Garcia Campos from comment #30)
> Comment on attachment 355466 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355466&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserTab.c:59
> > +static GHashTable *userMediaAccessPermissions;
> 
> This is a set of origins for which we granted user media permission, right?
> What about userMediaPermissionGrantedOrigins or something like that? current
> name sounds like if it's storing permission requests.

Done.

> > Tools/MiniBrowser/gtk/BrowserTab.c:204
> > +
> 
> Remove this empty line...
> 
> > Tools/MiniBrowser/gtk/BrowserTab.c:210
> > +            g_hash_table_remove(userMediaAccessPermissions, requestData->origin);
> > +        webkit_permission_request_deny(requestData->request);
> 
> ... or add one here.

Done that.

> > Tools/MiniBrowser/gtk/BrowserTab.c:215
> > -    g_object_unref(request);
> > +    g_clear_pointer(&requestData, permissionRequestDataFree);
> 
> requestData is a parameter here, setting it to NULL doesn't make sense, so I
> would just call permissionRequestDataFree()

It is not my style, I just complied with Adrian request here...

> > Tools/MiniBrowser/gtk/BrowserTab.c:249
> > +        if (g_hash_table_contains(userMediaAccessPermissions, webkit_web_view_get_uri(webView))) {
> > +            webkit_permission_request_allow(request);
> > +            return TRUE;
> 
> Should we also use the set for user media requests? to avoid asking for the
> same permission twice? or is that already cached by WebKit itself? If that's
> the case, I still don't understand why this isn't done by WebKit
> automatically.

Both are valid, we do not have the option to "remember response" and the wording

    "Allow access to %s device?"

doesn't imply it will, so I think we shouldn't.

> > Tools/MiniBrowser/gtk/BrowserTab.c:252
> > +        webkit_permission_request_deny(request);
> >          return FALSE;
> 
> This should be TRUE since you handled the request.

Stopped denying the request letting it "resolve itself".

> > Tools/MiniBrowser/gtk/BrowserTab.c:261
> > -    g_signal_connect(dialog, "response", G_CALLBACK(permissionRequestDialogResponse), g_object_ref(request));
> > +    g_signal_connect(dialog, "response", G_CALLBACK(permissionRequestDialogResponse), permissionRequestDataNew(request,
> > +        webkit_web_view_get_uri(webView)));
> 
> I'm confused here, permissionRequestDataNew expects an origin, but we are
> passing the URI here.

I didn't know about the conceptual differences, fixed now.

> > Tools/MiniBrowser/gtk/BrowserTab.c:442
> > +    if (!userMediaAccessPermissions)
> > +        userMediaAccessPermissions = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL);
> 
> I don't think you need the check here, class init is called once.

Well, until we subclass, better safe fmpov.

[Thibault Saunier]

Created attachment 356004 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 356004 [details]
Patch

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

> Tools/MiniBrowser/gtk/BrowserTab.c:91
> +    gchar *origin_str = webkit_security_origin_to_string(origin);

origin_str -> originStr

> Tools/MiniBrowser/gtk/BrowserTab.c:257
> +        if (g_hash_table_contains(userMediaPermissionGrantedOrigins, webkit_web_view_get_uri(webView))) {

You should get the origin here too, not the URI.

[Thibault Saunier]

Created attachment 356170 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 356170 [details]
Patch for landing

Rejecting attachment 356170 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 356170, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/10213774

[Thibault Saunier]

Created attachment 356171 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 356171 [details]:

inspector/model/remote-object-get-properties.html bug 192225 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 356171 [details]
Patch for landing

Clearing flags on attachment: 356171

Committed r238734: <https://trac.webkit.org/changeset/238734>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46372245>