Closed Bug 1379560 Opened 7 years ago Closed 7 years ago

Allow users to deny notification permissions globally

Categories

(Core :: Permission Manager, enhancement, P1)

Product:
Core
Component:
Permission Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(2 files)

Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.
59 bytes, text/x-review-board-request
nika
: review+
Paolo
: review+
Details
Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm.
59 bytes, text/x-review-board-request
Paolo
: review+
Details 
+++ This bug was initially created as a clone of Bug #1313939 +++

In both bug 1275599 and bug 1363856 we're looking for this kind of functionality. Here's my proposal for it:

Similar to the permissions.default.image pref (that is currently only used by DOM code) we could change the permission manager to fall back to a permissions.default.* pref instead of simply returning UNKNOWN when no permission for a certain principal/uri has been found. This pref could be set by the add-on API (they will lock it down to only allow globally blocking permissions) and by the permissions management interface (in bug 1368744 to block notification prompts).

There are a couple of advantages to this approach vs. toggling the existing prefs that are built into DOM code:

- We can have the user granularly manage permissions for sites in the permission management or the page info window while honoring their global preference by default.

- The user preference will be in sync with the results from the permission manager, there will be no confusing state as described in bug 1313939 comment 5.

- We can display additional UI if it turns out users find this feature confusing (don't remember they turned something off) and suggest adding a site exception.

- It automatically works for any future permissions we add.
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

https://reviewboard.mozilla.org/r/155816/#review161284

I'm good with this so long as we actually look at the consumers of a permission before we start exposing this pref in the UI :).
Attachment #8884998 - Flags: review?(michael) → review+
(In reply to Michael Layzell [:mystor] from comment #4)
> Comment on attachment 8884998 [details]
> Bug 1379560 - Part 1 - Add a default permission pref in the permission
> manager.
> 
> https://reviewboard.mozilla.org/r/155816/#review161284
> 
> I'm good with this so long as we actually look at the consumers of a
> permission before we start exposing this pref in the UI :).

Thank you! Yes, exposing this needs to be handled carefully, also from product perspective.
I'll take a look at this in a couple of days, it requires some focused attention.
(In reply to :Paolo Amadini from comment #6)
> I'll take a look at this in a couple of days, it requires some focused
> attention.

I agree, take your time. Thanks :)
Blocks: 1375683
Sorry if this review is taking some time. I've looked at the linked bugs and it seems this can wait a few days, but feel free to redirect if it's more urgent.

I have a general question in the meantime: why do you need to change nsPermissionManager.cpp? Platform code that is working with permission prompts already has to go through the front-end modules in order to keep tab permissions into account, so my first thought would be that we don't have to change the database back-end.
Flags: needinfo?(jhofmann)
(In reply to :Paolo Amadini from comment #8)
> Sorry if this review is taking some time. I've looked at the linked bugs and
> it seems this can wait a few days, but feel free to redirect if it's more
> urgent.

That's fine, I'll remind you in a bit and otherwise try to find someone else :)

> I have a general question in the meantime: why do you need to change
> nsPermissionManager.cpp? Platform code that is working with permission
> prompts already has to go through the front-end modules in order to keep tab
> permissions into account, so my first thought would be that we don't have to
> change the database back-end.

There are many places that use nsPermissionManager and will probably continue to do so. WebRTC is currently not using SitePermissions, as far as I'm aware, and native code will never be able to use SitePermissions. I think there are use cases for the permission manager beyond simply showing the prompt. As Matt mentioned in bug 1313939 comment 5, you get precedence problems when you disable a feature in one part of the browser but other parts say it's still working perfectly well.

For me, it's important to keep nsPermissionManager and SitePermissions in sync.
Flags: needinfo?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #9)
> native code will never be able to use SitePermissions

This isn't exactly true, as C++ already calls into XPCOM services to get permissions in some cases. But I think this is more about separation of concerns rather than language choice.

> For me, it's important to keep nsPermissionManager and SitePermissions in
> sync.

This doesn't seem to be what these patches are doing, as the getDefault() implementations in SitePermissions use different preferences. If you're pushing the knowledge of the global default to nsPermissionManager, you might want to create a getDefault method there and remove it from the front-end.

I'd note, however, that this adds to nsPermissionManager the responsibility of handling more than the storage and the subdomain inheritance logic. This would be better handled by a separate component, but if you want nsPermissionManager to do it, you should clearly document in the IDL which methods have knowledge of global defaults and which methods just handle raw database contents.

Another potential issue is the conflation of the UNKNOWN and PROMPT states. UNKNOWN should be a raw value reserved for when there is no entry in the database, while methods that have knowledge of defaults should return PROMPT instead, or the appropriate internal global default for that permission when there is no global preference set.

I think this confusion is exemplified by the test for "getAvailableStates", a method that should return which options are available for each permission type, but switches between UNKNOWN and PROMPT based on whether there is a global default set for the permission.
Comment on attachment 8884999 [details]
Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm.

https://reviewboard.mozilla.org/r/155818/#review164566

Setting r- while discussing the architectural issues above.
Attachment #8884999 - Flags: review?(paolo.mozmail) → review-
Thank you for taking the time to look at this. I understand your concerns and your comments made me think about this some more. I'll try to defend my approach, though I hope I can avoid getting too opinionated/ranty about this.

(In reply to :Paolo Amadini from comment #10)
> (In reply to Johann Hofmann [:johannh] from comment #9)
> > native code will never be able to use SitePermissions
> 
> This isn't exactly true, as C++ already calls into XPCOM services to get
> permissions in some cases. But I think this is more about separation of
> concerns rather than language choice.
> 
> > For me, it's important to keep nsPermissionManager and SitePermissions in
> > sync.
> 
> This doesn't seem to be what these patches are doing, as the getDefault()
> implementations in SitePermissions use different preferences.

getDefault is something we should get rid of at the next opportunity. People doing this instead of solving the problem in the permission manager has led to this confusing state in the first place. We can't clean it up that easily, since we'll need a migration, but it would be unfortunate if this was used as an argument to prevent us from solving it, IMO, properly (meaning that the permission manager is the single source of truth).

> If you're pushing the knowledge of the global default to nsPermissionManager, you
> might want to create a getDefault method there and remove it from the
> front-end.

I'd like to avoid supporting these custom prefs in the permission manager and would rather like to think about how we can get rid of them in SitePermissions.jsm.

> 
> I'd note, however, that this adds to nsPermissionManager the responsibility
> of handling more than the storage and the subdomain inheritance logic. This
> would be better handled by a separate component, but if you want
> nsPermissionManager to do it, you should clearly document in the IDL which
> methods have knowledge of global defaults and which methods just handle raw
> database contents.

What would this separate component look like? Is there any way we can make it clear/enforceable that the permission manager shouldn't be used without consulting that component for a default value? Otherwise yeah, I'm happy to add notes about this. I should have done that in the first place.

> Another potential issue is the conflation of the UNKNOWN and PROMPT states.
> UNKNOWN should be a raw value reserved for when there is no entry in the
> database, while methods that have knowledge of defaults should return PROMPT
> instead, or the appropriate internal global default for that permission when
> there is no global preference set.

I'm not introducing the conflation of UNKNOWN and PROMPT, it's been there since API consumers decided to treat UNKNOWN implicitly like PROMPT. My patch just adds a pref that sets a replacement for UNKNOWN to be returned when no object was found in the DB.

Instead, your main concern seems to be that UNKNOWN_ACTION, for consumers, is no longer guaranteed to implicitly mean that there is no object in the database. I totally understand and share that concern. However, in my view, that is not a guarantee the testPermission function should make. The documented definition is:

"Test whether a website has permission to perform the given action."

And that's what it should do. If a consumer wants to test if there is a permission entry in the DB, we should have a separate function for that (or use getPermissionObject). I went through the list of consumers, and virtually all of them seem to use UNKNOWN only as a default initialization mechanism and don't actually rely on the guarantee we're taking away.

There's https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/base/nsObjectLoadingContent.cpp#3466, which tries to update the expire time if the permission is something else than UNKNOWN, but that will only lead to a no-op operation for other default values and not do any harm AFAICS.
 
My point is that testPermission, by definition, should be reliable and not give false information e.g. return UNKNOWN when in reality SitePermissions.jsm will deny the permission prompt because permission.default.whatever has been set to 2. As long as nsPermissionManager can't be exclusively used by SitePermissions.jsm, you will end up with two sources of truth, one for the front-end and one for the back-end.

> I think this confusion is exemplified by the test for "getAvailableStates",
> a method that should return which options are available for each permission
> type, but switches between UNKNOWN and PROMPT based on whether there is a
> global default set for the permission.

I don't think there's any way to implement this without this sort of if-condition for the UI code (we have two capabilities that represent the same UI state). Again, it's not the fault of my patch that PROMPT wasn't used so far, otherwise we would have had to do this before.


Again, I appreciate your feedback. Let me know what you think.
Blocks: 1390782
Johann, just wondering where this stands. We are hoping to land a few things in 58 which depend on this bug.
Flags: needinfo?(jhofmann)
(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> Johann, just wondering where this stands. We are hoping to land a few things
> in 58 which depend on this bug.

Yes, I'm sorry, Photon has me dropping/delaying a few things these weeks already. It's slowly starting to clear and I would really like to push this over the finish line before 58. I'll keep you updated in the next days.
Flags: needinfo?(jhofmann)
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

So, with Photon winding down and us turning the icecc scheduler in the Berlin office on again, I decided to give this another shot.

Paolo and I had a long Vidyo discussion a while back where we said that it would be better to apply this selectively, only to the permissions we really care about. If I remember it correctly I was able to talk myself out of any other issues with the front-end patch, please correct me if I missed anything.

Our original proposition was to add an optional "defaultPref" parameter to the testPermission/testExactPermission functions, but that got messy quickly and doesn't resolve the problem that the permission manager results should be consistent for a single permission. I now did this by hard-coding the permission names in the permission manager. That would be good enough for me personally, but we could also store the permission names in a separate pref, e.g. permissions.default.types.

I like this version because it gets us where we want, scales to at least a couple dozen more permissions and doesn't impact other users of the permission manager.

Nika, Paolo, what do you think?
Attachment #8884998 - Flags: review+ → review?(nika)
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

https://reviewboard.mozilla.org/r/155816/#review193216


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: extensions/cookie/nsPermissionManager.cpp:146
(Diff revision 2)
> +// "false" unconditionally.
> +bool
> +HasDefaultPref(const char* aType)
> +{
> +  if (aType) {
> +    for (uint32_t i = 0; i < mozilla::ArrayLength(kPermissionsWithDefaults); ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

https://reviewboard.mozilla.org/r/155816/#review163534

Thanks for restarting the work on this! This looks good to me given the various points we discussed, but as mentioned earlier, document in the IDL which methods have knowledge of global defaults and which methods just handle raw database contents.

::: extensions/cookie/nsPermissionManager.cpp:2266
(Diff revision 2)
>    }
>  
>    // Set the default.
>    *aPermission = nsIPermissionManager::UNKNOWN_ACTION;
>  
> +  // For some permissions, query the default from a pref.

Add a comment here, and maybe also in the function, that this check exists so we don't get a performance hit for most permissions that may be checked more frequently from platform code.
Attachment #8884998 - Flags: review?(paolo.mozmail)
Comment on attachment 8884999 [details]
Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm.

https://reviewboard.mozilla.org/r/155818/#review193480

Most issues we discussed were related to this front-end patch, so I need some reminders before doing a more detailed review.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:17
(Diff revision 2)
>  state.current.allowed = Allowed
>  state.current.allowedForSession = Allowed for Session
>  state.current.allowedTemporarily = Allowed Temporarily
>  state.current.blockedTemporarily = Blocked Temporarily
>  state.current.blocked = Blocked
> +state.current.prompt = Always Ask

So, I vaguely remember that we discussed the case of setting a global default that is different from always asking, and then overriding it for a specific site so that we would ask every time, as opposed to just always doing the opposite than the global default. Why was this a use case worth supporting? Can you provide an example?

::: browser/modules/test/unit/test_SitePermissions.js:64
(Diff revision 2)
>    Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
>                     [ SitePermissions.UNKNOWN,
>                       SitePermissions.ALLOW,
>                       SitePermissions.BLOCK ]);
>  
> +  // Test available states with a default permission set.
> +  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.ALLOW);
> +  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> +                   [ SitePermissions.PROMPT,
> +                     SitePermissions.ALLOW,
> +                     SitePermissions.BLOCK ]);
> +  Services.prefs.clearUserPref("permissions.default.camera");
> +

Can you remind me why we thought it was OK to change the available states based on the current global default? What happens to the user interface when the global default changes but there is the PROMPT value set explicitly?

::: browser/modules/test/unit/test_SitePermissions.js:131
(Diff revision 2)
> +  // Check that without a pref the default return value is UNKNOWN.
> +  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
> +    state: SitePermissions.UNKNOWN,
> +    scope: SitePermissions.SCOPE_PERSISTENT,
> +  });

Callers just interested in checking the permission may now get SitePermissions.UNKNOWN and SitePermissions.PROMPT indifferently. Why was this fine?
Attachment #8884999 - Flags: review?(paolo.mozmail)
(In reply to :Paolo Amadini from comment #20)
> Comment on attachment 8884999 [details]
> Bug 1379560 - Part 2 - Add support for custom default permissions in
> SitePermissions.jsm.
> 
> https://reviewboard.mozilla.org/r/155818/#review193480
> 
> Most issues we discussed were related to this front-end patch, so I need
> some reminders before doing a more detailed review.

Thanks for remembering so many things we talked about, I had forgotten most of this already.

> ::: browser/locales/en-US/chrome/browser/sitePermissions.properties:17
> (Diff revision 2)
> >  state.current.allowed = Allowed
> >  state.current.allowedForSession = Allowed for Session
> >  state.current.allowedTemporarily = Allowed Temporarily
> >  state.current.blockedTemporarily = Blocked Temporarily
> >  state.current.blocked = Blocked
> > +state.current.prompt = Always Ask
> 
> So, I vaguely remember that we discussed the case of setting a global
> default that is different from always asking, and then overriding it for a
> specific site so that we would ask every time, as opposed to just always
> doing the opposite than the global default. Why was this a use case worth
> supporting? Can you provide an example?

The example I stated was: What if you globally block location access but want Google Maps to be able to _ask_ you for your location (because you don't trust Google enough to give it permanent location access, which I can very well relate to). It's a little far-fetched, but supporting this use case is really inexpensive, so I'm for it.

> ::: browser/modules/test/unit/test_SitePermissions.js:64
> (Diff revision 2)
> >    Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> >                     [ SitePermissions.UNKNOWN,
> >                       SitePermissions.ALLOW,
> >                       SitePermissions.BLOCK ]);
> >  
> > +  // Test available states with a default permission set.
> > +  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.ALLOW);
> > +  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> > +                   [ SitePermissions.PROMPT,
> > +                     SitePermissions.ALLOW,
> > +                     SitePermissions.BLOCK ]);
> > +  Services.prefs.clearUserPref("permissions.default.camera");
> > +
> 
> Can you remind me why we thought it was OK to change the available states
> based on the current global default? What happens to the user interface when
> the global default changes but there is the PROMPT value set explicitly?

I guess what really should happen is: We always show [PROMPT, ALLOW, BLOCK] (except for permissions that do it differently, like popups) and when the user clicks "Use Default" we remove the permission from the permission manager. This, unfortunately, has the problem that we try to check "Use Default" automatically when the user selects the radio box that corresponds to the default value, which means we'd have to do some hackery that assumes that PROMPT == UNKNOWN in the page info code.

I vaguely remember that we said that this might be preferable to hackery in SitePermissions.jsm, though, so let me try that out.

> ::: browser/modules/test/unit/test_SitePermissions.js:131
> (Diff revision 2)
> > +  // Check that without a pref the default return value is UNKNOWN.
> > +  Assert.deepEqual(SitePermissions.get(uri, "camera"), {
> > +    state: SitePermissions.UNKNOWN,
> > +    scope: SitePermissions.SCOPE_PERSISTENT,
> > +  });
> 
> Callers just interested in checking the permission may now get
> SitePermissions.UNKNOWN and SitePermissions.PROMPT indifferently. Why was
> this fine?

I don't think this function was ever particularly qualified to tell if there's a permission object in the database (it goes through too many hoops for that). If you want to know for sure you can use Services.perms.getPermissionObject or just compare with SitePermissions.getDefault.
(In reply to Johann Hofmann [:johannh] from comment #21)
> > Callers just interested in checking the permission may now get
> > SitePermissions.UNKNOWN and SitePermissions.PROMPT indifferently. Why was
> > this fine?
> 
> I don't think this function was ever particularly qualified to tell if
> there's a permission object in the database (it goes through too many hoops
> for that). If you want to know for sure you can use
> Services.perms.getPermissionObject or just compare with
> SitePermissions.getDefault.

What I was trying to say is that the callers now have to deal with two possible values that would mean the same thing to them, but the difference doesn't provide any other valuable information, exactly because of the reason you describe.
(In reply to :Paolo Amadini from comment #22)
> (In reply to Johann Hofmann [:johannh] from comment #21)
> > > Callers just interested in checking the permission may now get
> > > SitePermissions.UNKNOWN and SitePermissions.PROMPT indifferently. Why was
> > > this fine?
> > 
> > I don't think this function was ever particularly qualified to tell if
> > there's a permission object in the database (it goes through too many hoops
> > for that). If you want to know for sure you can use
> > Services.perms.getPermissionObject or just compare with
> > SitePermissions.getDefault.
> 
> What I was trying to say is that the callers now have to deal with two
> possible values that would mean the same thing to them, but the difference
> doesn't provide any other valuable information, exactly because of the
> reason you describe.

I'm not introducing the PROMPT state to the permission manager, though ;)
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

https://reviewboard.mozilla.org/r/155816/#review193232

I don't hate this, but I'm not a big fan of the hardcoding of this information inside of the permission manager. We do have some precident for this (such as the preload permsissions) but it is fairly gross.
Attachment #8884998 - Flags: review?(nika) → review+
(In reply to Johann Hofmann [:johannh] from comment #21)
> > ::: browser/modules/test/unit/test_SitePermissions.js:64
> > (Diff revision 2)
> > >    Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> > >                     [ SitePermissions.UNKNOWN,
> > >                       SitePermissions.ALLOW,
> > >                       SitePermissions.BLOCK ]);
> > >  
> > > +  // Test available states with a default permission set.
> > > +  Services.prefs.setIntPref("permissions.default.camera", SitePermissions.ALLOW);
> > > +  Assert.deepEqual(SitePermissions.getAvailableStates("camera"),
> > > +                   [ SitePermissions.PROMPT,
> > > +                     SitePermissions.ALLOW,
> > > +                     SitePermissions.BLOCK ]);
> > > +  Services.prefs.clearUserPref("permissions.default.camera");
> > > +
> > 
> > Can you remind me why we thought it was OK to change the available states
> > based on the current global default? What happens to the user interface when
> > the global default changes but there is the PROMPT value set explicitly?
> 
> I guess what really should happen is: We always show [PROMPT, ALLOW, BLOCK]
> (except for permissions that do it differently, like popups) and when the
> user clicks "Use Default" we remove the permission from the permission
> manager. This, unfortunately, has the problem that we try to check "Use
> Default" automatically when the user selects the radio box that corresponds
> to the default value, which means we'd have to do some hackery that assumes
> that PROMPT == UNKNOWN in the page info code.
> 
> I vaguely remember that we said that this might be preferable to hackery in
> SitePermissions.jsm, though, so let me try that out.

Revisiting my earlier comment, I don't believe it's worth working around the PROMPT == UNKNOWN convention outside of SitePermissions.jsm, I added a comment in the getAvailableStates function to that end. Both consumers (pageinfo and preferences permission management) work well with this patch. Ultimately it comes down to de-duplicating one of the two values returned here, no matter where it happens.

> > What happens to the user interface when
> > the global default changes but there is the PROMPT value set explicitly?

That is quite an edge case, but it's something we should address. The page info code falls back to the correct item, while I added code to preferences permission management to work around this.
Comment on attachment 8884998 [details]
Bug 1379560 - Part 1 - Add a default permission pref in the permission manager.

https://reviewboard.mozilla.org/r/155816/#review195378
Attachment #8884998 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8884999 [details]
Bug 1379560 - Part 2 - Add support for custom default permissions in SitePermissions.jsm.

https://reviewboard.mozilla.org/r/155818/#review195380

I haven't tested the latest version locally because it seems you've already tried it extensively and addressed everything we discussed, and the latest changes look good.

Thank you, this is good to go for me!
Attachment #8884999 - Flags: review?(paolo.mozmail) → review+
I didn't use the correct permission name for desktop-notification, which should be corrected now. Try looks green now (I was slightly confused by bug 1352791 for a moment).
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bbe4e693eb02
Part 1 - Add a default permission pref in the permission manager. r=mystor,Paolo
https://hg.mozilla.org/integration/autoland/rev/99b0d3748cda
Part 2 - Add support for custom default permissions in SitePermissions.jsm. r=Paolo
https://hg.mozilla.org/mozilla-central/rev/bbe4e693eb02
https://hg.mozilla.org/mozilla-central/rev/99b0d3748cda
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Johann, thanks for landing this. It will enable us to implement a few WebExtensions APIs that have been blocked. I have a couple of questions/issues.

1. It looks like this only implements this for the camera, microphone, geo and desktop-notification permissions. If we want to be able to set a default for other permissions, such as cookies [1] and popups [2], does some platform work need to be done to enable those as well?

2. I was working on bug 1364942, for desktop notifications and was trying to do some manual testing, but getting strange results. Testing with a page loaded (in this case it was https://www.wikipedia.org/), if I tried to create a notification via the web console, by default my notification didn't appear and I was not prompted, which is not what I expected. I expected to be prompted.

If I set permissions.default.desktop-notification to Services.perms.ALLOW_ACTION then the notification did appear, without prompting, and if I set permissions.default.desktop-notification to Services.perms.DENY_ACTION then the notification did not appear, also without prompting, and those are as I expected.

If I went into the permissions UI for the page, and manually chose "Allow" then the notification did appear, without prompting, and if I manually chose "Block" the notification did not appear, also without prompting, and those are also as I expected. But if I manually chose "Always Ask" then the notification did not appear and I was not prompted, which is not what I would expect. Note that this last behaviour with "Always Ask" chosen reproduces regardless of the value set for permissions.default.desktop-notification. I tried all values from 0 - 4 and none of them caused the browser to prompt me.

Does this represent a regression introduced by these changes, or am I misunderstanding how this is supposed to work?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1363860
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1364971
Flags: needinfo?(jhofmann)
We had a chat over Vidyo and cleared some things, short summary below:

1. Yes, we will have to whitelist cookies and popups and we can probably do that in the respective bugs. Since these two permissions use their own global default prefs (that are, unfortunately, not picked up by the permission manager) we have to tread a little more carefully to sort things out the right way. I'm happy to help here. Mid-term I'd love if we could avoid these permissions having their own special default prefs.

2. For Notifications, you have to explicitly ask for permission via the DOM API, it's not triggered by just executing new Notification() (but it obviously worked when the global or local setting was set to ALLOW or BLOCK). See https://developer.mozilla.org/en-US/docs/Web/API/notification

Thanks!
Flags: needinfo?(jhofmann)
(In reply to Johann Hofmann [:johannh] from comment #41)
> We had a chat over Vidyo and cleared some things, short summary below:

Thanks Johann. Yes, that cleared it up for me and I did some more testing and it all seems to be working as expected.
Depends on: 1411290
No longer depends on: 1411290
Regressions: 1589896
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: