Skip to content

support Chrome via WebExtension browser API Polyfill#10

Draft
gianpaj wants to merge 3 commits intoa13o:masterfrom
gianpaj:feature/allow-on-chrome
Draft

support Chrome via WebExtension browser API Polyfill#10
gianpaj wants to merge 3 commits intoa13o:masterfrom
gianpaj:feature/allow-on-chrome

Conversation

@gianpaj
Copy link
Copy Markdown

@gianpaj gianpaj commented Feb 17, 2020

I followed the instructions from the Mozilla repo:
https://github.com/mozilla/webextension-polyfill

  • Make the CSS injection work
  • Make the JS execute
  • Test on Hackernews all the CSS and JS
  • Test on Twitter all the CSS (broken/outdated at the moment)
  • Test on YouTube all the CSS and JS
  • Test on Reddit all the CSS and JS
  • Test on YouTube Gaming all the CSS
  • Test on YouTube Music all the CSS and JS
  • Test that the extension does nothing and causes no errors in an unsupported site

Copy link
Copy Markdown
Owner

@a13o a13o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far LGTM. Promisified web-ext API is a benefit itself and if it leads to cross-browser support even better.

Comment thread src/background.js
Comment on lines +131 to +140
contentScript.files
.filter(file => file.match(/.*\.css$/))
.map(file => `${contentScript.folder}/${file}`)
.map(file => browser.tabs.insertCSS(null, { file }));

GLOBAL_SCRIPTS.map(file => browser.tabs.executeScript(null, { file }));
contentScript.files
.filter(file => file.match(/.*\.js$/))
.map(file => `${contentScript.folder}/${file}`)
.map(file => browser.tabs.executeScript(null, { file }))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously done in browser.contentScripts.register because that method uses the matches array to only inject the css/scripts into tabs where the URL matches the pattern.

I think that previous behavior is worth keeping so that as the project grows it doesn't inject more and more unnecessary css/scripts into each tab.

Is this change needed to work with the browser API package?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm new to these browser.* API methods.
The browser.contentScripts.register doesn't seem to work on Chrome, even after the polyfill. I'll do some more testing on to why it's that

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this today. browser.contentScripts.register isn't supported by Chrome, so the polyfill doesn't implement it. It only polyfills Chrome's existing api and nothing else.

It's possible that the script loading strategy needs to change to support Chrome. This extension will have to filter the content scripts before injecting them with the cross-platform browser.tabs.executeScript.

I've been slowly working on a larger refactor of the extension that redoes how it loads content scripts, so I can move all the supported sites to optionalPermissions. I think Chrome support should wait for this refactor to finish since it uses browser.tabs.onActivate to determine which scripts to inject, which is the exact functionality needed to move away from leaning on matches in browser.contentScripts.register.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. makes sense. I tried another polyfill and keeping browser.contentScripts.register but didn't work
https://github.com/fregante/content-scripts-register-polyfill

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