Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Regression : session.setPermissionRequestHandler callback is broken #3877

Closed
alexwykoff opened this issue Sep 11, 2016 · 7 comments · Fixed by #3942 or #3965
Closed

Regression : session.setPermissionRequestHandler callback is broken #3877

alexwykoff opened this issue Sep 11, 2016 · 7 comments · Fixed by #3942 or #3965

Comments

@alexwykoff
Copy link
Contributor

Did you search for similar issues before submitting this one?
Yes

Describe the issue you encountered:
When testing http://www.bennish.net/web-notifications.html during a test run, it was noticed that the notifications were not displaying.

Expected behavior:
The notifications should work.

  • Platform (Win7, 8, 10? macOS? Linux distro?):
    Windows 7 ia-32 and OS X
  • Brave Version:
    0.12.1
  • Steps to reproduce:
    1. Visit http://www.bennish.net/web-notifications.html
    2. Trigger a notification
  • Screenshot if needed:
  • Any related issues:
@bbondy
Copy link
Member

bbondy commented Sep 12, 2016

Notifications aren't supported in win7 since the start, so removing this from 0.12.1. The OS doesn't have them so we'd have to build something custom.

@bbondy bbondy removed this from the 0.12.1dev milestone Sep 12, 2016
@alexwykoff
Copy link
Contributor Author

As mentioned in the original ticket, this occurs on OS X as well where they are supported. I would advocate pulling this back into the milestone.

@alexwykoff alexwykoff self-assigned this Sep 12, 2016
@alexwykoff
Copy link
Contributor Author

This is also not working on Windows x64.

@bbondy
Copy link
Member

bbondy commented Sep 12, 2016

which windows version?

@bbondy
Copy link
Member

bbondy commented Sep 12, 2016

if windows 7 it is known and not a regression per above. Just label with Windows only and re-adjust title pls to indicate win7 only

@diracdeltas
Copy link
Member

seems like permissions in general are not working. geolocation str:

  1. go to https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/Using_geolocation
  2. click on the button in the demo
  3. click 'allow' in the notification bar

it says location cannot be found

@diracdeltas diracdeltas changed the title Regression : HTML5 web notifications are not working. Regression : session.setPermissionRequestHandler callback is broken Sep 12, 2016
@diracdeltas diracdeltas added this to the 0.12.1dev milestone Sep 12, 2016
@diracdeltas
Copy link
Member

diracdeltas commented Sep 12, 2016

@bbondy @bridiver i think this is an Electron issue. in session.setPermissionRequestHandler, the callback seems to reject the permission regardless of whether it's called with true or false.

i'm on os x
Brave: 0.12.1
Electron: 1.3.12
libchromiumcontent: 52.0.2743.116
V8: 5.2.361.49
Node.js: 6.3.0
Update Channel: dev

@bridiver bridiver self-assigned this Sep 12, 2016
bridiver referenced this issue Sep 12, 2016
instead of corresponding to the origin requesting the permission.
fix #3579

Auditors: @bbondy

Test Plan: Go to https://twitter.com/settings/web_notifications and confirm
that clicking on the 'Turn on' button pops up a notification bar.
bridiver added a commit that referenced this issue Sep 13, 2016
…the permission dialog

fix #3877
auditors: @bbondy @diracdeltas @mrose17

geoip service must be enabled through 
GOOGLE_API_KEY
GOOGLE_API_ENDPOINT
env vars
requires brave/muon@f890204
@bridiver bridiver reopened this Sep 13, 2016
bridiver added a commit that referenced this issue Sep 13, 2016
…ose tabs to display on based on mainFrameUrl

don't allow Brave to display notifications without permission
remove unnecessary password copied notifications
cc @bbondy @diracdeltas
fix #3877
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.