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

Injection improvements #579

Closed
1 of 3 tasks
fregante opened this issue Jun 22, 2021 · 5 comments
Closed
1 of 3 tasks

Injection improvements #579

fregante opened this issue Jun 22, 2021 · 5 comments
Assignees
Labels
performance priority user experience Improve the user experience (UX)

Comments

@fregante
Copy link
Contributor

fregante commented Jun 22, 2021

A few things we can do to make responses faster:

  • Reduce the content script entry point via import() as suggested in #540: prevent multiple content script injections #541 (comment)
  • Have a single, unified way to "ensure that the content script exists", probably as part of the messaging API. No more explicit injectTemporaryAccess/injectContentScript/isInstalled/waitReady
  • TBD

Related:

@fregante fregante added user experience Improve the user experience (UX) performance labels Jun 22, 2021
@fregante
Copy link
Contributor Author

fregante commented Jun 23, 2021

One minor issue I found in Firefox:

  • executeScript silently fails on meta pages (or maybe others too?)

It's something I noticed in the sidebar error PR since I only get a "unable to connect" error instead of "unable to inject"

Related:

@fregante
Copy link
Contributor Author

tabReady[tabId][frameId] = true;

I'm confused about this part of the code. It's marking tabs and frames as "ready" but… what happens when a frame reloads? Don't we lose that readiness? This is never set to false though.

I haven't verified, but this would mean that parts of the extension that depend on this will fail after a reload.

@twschiller
Copy link
Contributor

twschiller commented Jul 1, 2021

I haven't verified, but this would mean that parts of the extension that depend on this will fail after a reload.

I think you're correct (I haven't verified either). In practice this executor code is used in a flow where the @pixiebrix/browser/open-tab brick is used to open a new tab, and then an action is performed on the URL that is loaded (using window: target) in the brick. So it hasn't been a problem

@twschiller
Copy link
Contributor

@fregante What do you view as remaining on this ticket?

We can create a separate issue for: #579 (comment) that's low priority

@fregante
Copy link
Contributor Author

fregante commented Jul 4, 2021

We can create a separate issue for: #579 (comment) that's low priority

Sounds good. What would the action be though? Should that code be dropped?

@fregante What do you view as remaining on this ticket?

If #661 eliminated duplicate injection, maybe the first task on this issue isn't necessary anymore.

One minor issue I found in Firefox:

I don't know whether this still applies, I'll extract it if I encounter it again.


The next step I wanted to take as part of this was this, but I'll extract this when I hit a problem with it again:

  • the lifters/messengers should just attempt to send the message first and then await/inject the content script automatically

Closing for now, I'll open more specific issues when found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance priority user experience Improve the user experience (UX)
Projects
None yet
Development

No branches or pull requests

2 participants