Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🎉 Add advanced setting to allow all website access permission. #196

Merged
merged 8 commits into from
Jul 26, 2018

Conversation

zmilonas
Copy link
Collaborator

@zmilonas zmilonas commented Jul 22, 2018

Closes #168

Tested with Chrome 69 on macOS

Demo screen recording here: https://gfycat.com/ForthrightDefensiveIberianemeraldlizard

@subdavis
Copy link
Owner

subdavis commented Jul 23, 2018

Woohoo! Great demo gif, also!

Couple of quick questions about how you chose to do this.

  1. Permissions are usually requested in the background when the request comes from the pop-up page because the pop-up might close during the user interaction. There's no danger of the settings window closing, so you can skip that bit and run the permissions request directly from this page if you want.

  2. Could you talk about the choice to not allow users to revert this option? It looks like the user would have to modify permissions from the browser permissions manager rather than unclick this toggle. Would there be any issues with using the chrome.permissions.remove API to let users change their minds later?

BTW, I'm giving a talk about this project on Tuesday, and I have a slide to mention how cool it has been that contributors like you have come along to pick up features and bugs reports. Thanks!

@zmilonas
Copy link
Collaborator Author

zmilonas commented Jul 23, 2018

@subdavis thanks for the feedback

ad 1. Good to know for the future :D
ad 2. I must've been to careless when implementing and didn't know i can programatically remove the permission as well. I changed it up

Great to hear you like the contributions.
Also, probably need a seperate issue for that but there is a need for some more or less centralized logging of errors, unless we want to just console.error to print to chrome extension log.

@subdavis
Copy link
Owner

@zmilonas looks like there are some conflicts here. Once those get sorted out, we can merge.

@zmilonas
Copy link
Collaborator Author

@subdavis conflicts resolved :)

@subdavis
Copy link
Owner

Opened #199 to tackle some of the anomalies in options page styles. Otherwise, LGTM.

@subdavis subdavis merged commit 95ffcd9 into subdavis:develop Jul 26, 2018
@subdavis
Copy link
Owner

subdavis commented Aug 5, 2018

I don't know if I'm losing my mind, but this broke.

options.html:1 Unchecked runtime.lastError while running permissions.request: This function must be called during a user gesture
    at allOriginPermission (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/build/options.build.js:6602:24)
    at Ni.run (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:100:6484)
    at zt (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:74289)
    at Array.<anonymous> (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:68397)
    at MessagePort.at (chrome-extension://fmhmiaejopepamlcjkncpgpdjichnecm/dll/dll.library.js:95:68226)

I remember testing this PR and it working. I'm on Chromium 67.0.3396.99 (Official Build) Built on Ubuntu , running on Ubuntu 17.10 (64-bit)

This is now the same thing that Firefox does as mentioned in the README. Can you reproduce, @zmilonas? I'm running on commit 95ffcd9

@zmilonas
Copy link
Collaborator Author

zmilonas commented Aug 6, 2018

@ubdavis
Can't reproduce :/. What are the steps?

@subdavis
Copy link
Owner

subdavis commented Aug 6, 2018

Just try to toggle on this feature from advanced settings. What version of Chrome are you on, and what platform?

zmilonas added a commit to zmilonas/Tusk that referenced this pull request Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants