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

Remove "tabs" permission #87

Closed
bershanskiy opened this issue Aug 7, 2019 · 20 comments
Closed

Remove "tabs" permission #87

bershanskiy opened this issue Aug 7, 2019 · 20 comments

Comments

@bershanskiy
Copy link
Contributor

Summary

Untitled

Currently, users are warned that extension can "Read your browsing history", but I believe you can remove this warning by using "activeTab" permission instead of "tabs" permission.

Details

Currently, the extension requires "tabs" permission which creates a rather offputting warning "Read your browsing history" (according to this table). This permission is necessary to populate url, title, and favIconUrl properties of Tab objects, which you never use in the scripts. I think, you use it only to infer the current tab to populate the popup interface. Instead, Chrome has "activeTab" permission that provides this information and does not display any warning message on install.

Switching to "activeTab" would require a small patch, not just removing "tabs" from the manifest. If you are interested, I can prepare this patch and make a PR.

@ajayyy
Copy link
Owner

ajayyy commented Aug 7, 2019

Does this interfere with content scripts running in tabs that aren't the active tab. If not, then yes, that's a great idea to switch.

If you are up to it, feel free to make a PR :)

@bershanskiy
Copy link
Contributor Author

Does this interfere with content scripts running in tabs that aren't the active tab.

No, content scripts continue running even if the tab is not active.

However, the patch will be rather large because this extension popup.js function runThePopup() uses a lot of code like this:

  chrome.tabs.query({
    active: true,
    currentWindow: true
  }, tabs => {
    chrome.tabs.sendMessage(
      tabs[0].id,
      {message: '...'},

It all needs to be rewrtten with activeTab instead.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

activeTab only seems to work when certain actions are taken and until the user navigates to a different origin.
This would work if you had a pageAction setup that you could click to invoke the extension
https://developer.chrome.com/extensions/pageAction
https://developer.chrome.com/extensions/activeTab#invoking-activeTab

@bershanskiy
Copy link
Contributor Author

@officialnoob

activeTab can track your history

Yes, but only for pages that you explicitly envoke the extension on via browserAction or via a context menu. It has a much narrower scope than tabs. Currently, you use chrome.tabs.onActivated, which notifies you of all tab changes, even not related to YouTube (so, in theory, you could record all history including navigations and even just tab changes).

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

activeTab tracking would only affect places where its been invoked

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

I think Tab messaging should still work
As long as the extension gets invoked somehow

@bershanskiy
Copy link
Contributor Author

@officialnoob

activeTab only seems to work when certain actions are taken and until the user navigates to a different origin.

Exactly.

This would work if you had a pageAction setup that you could click to invoke the extension

Yes, when you click the extension icon you trigger that pageAction and it can query the currently active tab to display the relevant info.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

So are people going to click a pageAction every time they visit youtube
Also how are embedded videos going to work?

@bershanskiy
Copy link
Contributor Author

So are people going to click a pageAction every time they visit youtube

No, you trigger pageAction when you open the popup by clicking the extension icon. You use pageAction only to figure out what the current tab is to populate the popup with the proper info (if you have multiple pages open with multiple YouTube videos).

Also how are embedded videos going to work?

They are controlled by content scripts. Since videos are either on youtube.com or are iframes of it, they get injected with content scripts.

This way, if you open a page example.com that has an embedded video, the extension does not interact with example.com -- only with embedded YouTube frame. Now, if the user clicks on the extension icon, he/she grants temporary permission (pageAction) and thus the popup can interact with the page to display the relevant information.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

"when you open the popup by clicking the extension icon" so you do have to or it will not be in effect at the moment even submitting a video not just skipping you dont need to use the popup

The content script running on the host website is isolated from youtube.com by the iframe and should not be able to control it

https://discourse.mozilla.org/t/activetab-permission-does-not-include-iframes-in-current-tab/29084

@ajayyy
Copy link
Owner

ajayyy commented Aug 7, 2019

Hmm, this would require a significant rewrite after thinking about it. At the moment background.js is communicating with the content scripts to tell them when a new video is loaded. This would have to be changed to the content scripts using the on hashed changed function (which is probably a better way anyway).

Then, everything can be handled by the content script except for actions invoked by the popup.

The popup could save a variable right after being opened of the current tab and use that to communicate any time a button clicked (unless I'm misunderstanding).

If the plan to make the popup be only for global settings, then the active tab permission won't even be needed.

@ajayyy
Copy link
Owner

ajayyy commented Aug 7, 2019

I think the best way to solve this is just to make the popup not interact with the tabs at all and just act as a status page (showing how much you have contributed) and a global settings page.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 7, 2019

@ajayyy have you seen my message? I think the problem is valid

@bershanskiy
Copy link
Contributor Author

@officialnoob

"when you open the popup by clicking the extension icon" so you do have to or it will not be in effect at the moment even submitting a video not just skipping you dont need to use the popup

I'm not sure what your question is.

@ajayyy

Hmm, this would require a significant rewrite after thinking about it.

Yes, it would because (currently) the extension uses a lot of chrome.tabs. I do not think this is a good solution because it does not work on pages other than YouTube pages with individual videos, where you already have a much better (in my opinion) option of opening popup in the sidebar. As it stands, current popup UI is mostly useless for my use cases:

  • it does not work on any pages other than individual YouTube video page
  • on individual YouTube video page, I would rather use the persistently open sidebar.

I think the best way to solve this is just to make the popup not interact with the tabs at all and just act as a status page (showing how much you have contributed) and a global settings page.

We should be asking a question "what is the best UI for the user" and not "what is easy to implement". The problem is, the first question is much harder than the second one.

@bershanskiy
Copy link
Contributor Author

@ajayyy Implementing this will require significant code reorganization, so there is no point in stating on it before #89 gets merged.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 8, 2019

@bershanskiy I was trying to tell you about a problem if you want to use the URL, title, favicon url that you get with the extra permission you will need a action from the user which in an iframe may not be possible Im not sure why you even need activeTab messaging still works anyway

@NDevTK
Copy link
Contributor

NDevTK commented Aug 8, 2019

Made a PR as this seems simple to add: #90

@bershanskiy
Copy link
Contributor Author

why you even need activeTab

I meant that you do not need tabs and that you can use activeTab in all circumstances you would use tabs. E.g., it is sufficient for figuring out what to display on the popup, even for pages with embeded videos.
This is more of a future-proofing for cases when the user opens popup on pages with embedded videos (not YouTube itself). Currently, the extension simply does not allow the user to fine-tune ad times for embedded videos: the popup does not work and there is no side menu.

@ajayyy
Copy link
Owner

ajayyy commented Aug 8, 2019

Is sponsor submission really necessary for embeds anyway? Only skipping is needed I'd think.

@NDevTK
Copy link
Contributor

NDevTK commented Aug 8, 2019

Well I think activeTab will not be needed as the URL of a tab is not needed only the video id
what do you mean by "even for pages with embedded videos." as I said If the extra stuff from that permission is needed there is no way to click a page action when its in a iframe... the only thing used from the URL is the video id and that can be sheared anyway

@ajayyy ajayyy closed this as completed Aug 12, 2019
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

No branches or pull requests

3 participants