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

Make Enterprise injection more reliable #2111

Merged
merged 23 commits into from Jun 21, 2019

Conversation

Projects
None yet
4 participants
@bfred-it
Copy link
Collaborator

commented Jun 2, 2019

I changed the way content scripts are injected. They are no longer injected manually with tabs.executeScript but they are declaratively registered via contentScript.register (which is polyfilled to declarativeContent.RequestContentScript).

2 polyfills and the glue for both (which registers the scripts on new origins) will be extracted into their own modules once verified that this all works.

The only minor bug right now is in Firefox: even if you remove the permission, the scripts will still be injected until the next restart. It's Firefox' bug, nothing asynchronous works after request/remove for some reason.

cc @busches

Test

  1. Visit github.com, no "double injection" errors should ever appear, even if you try "Enable Refined GitHub on this domain"

  1. Visit google.com, no injection happening, so no errors in the console or background.js
  2. Visit google.com/images, "Enable Refined GitHub on this domain"
  3. Reload the page and see the errors in the console (obviously, because it's not GitHub)
  4. Search for images
  5. Open a photo
  6. You shouldn't see the same errors repeated, because it's an ajaxed page and it the content scripts shouldn't be injected multiple times.

In no case should there ever be any errors in the console. I dropped a try/catch so the previously-silenced errors should no longer ever happen.

@bfred-it bfred-it added the enterprise label Jun 2, 2019

@webknjaz
Copy link

left a comment

unrelated

Repository owner deleted a comment from webknjaz Jun 5, 2019

Repository owner deleted a comment from webknjaz Jun 5, 2019

@bfred-it bfred-it force-pushed the ghe-injection branch from c6a579a to 202d8d6 Jun 6, 2019

@bfred-it bfred-it marked this pull request as ready for review Jun 6, 2019

bfred-it added some commits Jun 6, 2019

@notlmn

This comment has been minimized.

Copy link
Contributor

commented on f21bcee Jun 7, 2019

Now I know how @bfred-it debugs his code 😄

This comment has been minimized.

Copy link
Collaborator Author

replied Jun 7, 2019

LOLs and NUTS, when for some reason even setTimeout doesn't work

@bfred-it bfred-it merged commit 558f2d3 into master Jun 21, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bfred-it bfred-it deleted the ghe-injection branch Jun 21, 2019

@@ -0,0 +1,36 @@
/* global chrome */

This comment has been minimized.

Copy link
@notlmn

notlmn Jun 22, 2019

Contributor

@bfred-it Can you extract this file out as another npm module?

@bfred-it

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

Yes after this has been tested and works correctly. There are 3 modules to be extracted

@busches

This comment has been minimized.

Copy link
Collaborator

commented Jun 25, 2019

FYI I changed jobs and don't currently have access to GHE to test. 😐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.