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

Improve scripts injection flow and logic #334

Open
ACTCD opened this issue Sep 28, 2022 · 6 comments
Open

Improve scripts injection flow and logic #334

ACTCD opened this issue Sep 28, 2022 · 6 comments
Assignees
Labels
enhancement Feature work maintenance Chores and refactoring
Milestone

Comments

@ACTCD
Copy link
Collaborator

ACTCD commented Sep 28, 2022

When we complete the refactoring of #331, we could try to further optimize the script injection flow and speed up the scripts injection.

In content.js, we will accomplish scripts injection with just two browser.storage.local.get asynchronous operations:

  • Get extension settings information
    • Extension active switch
    • Scripts active switch
    • Global exclude-match list
    • match/include/exclude...
  • Get the matched scripts code
    • Prepare wrappers for each script
    • Add the corresponding GM APIs in the wrapper
    • Add script code in wrapper
    • Immediately inject scripts

This will do the fastest document-start scripts injection in content scripts, but cannot guarantee full priority over page scripts as there are still asynchronous operations.

In background.js, which is responsible for updating the relevant information in browser.storage.local, we can check and update the scripts corresponding to the url request as early as possible, we could use the following process:

  • Use webNavigation.onBeforeNavigate to get the request url
  • Get cached scripts list and file modification date via browser.storage.local.get
  • Get the current files list and file modification date through the native layer, and get new or modified scripts code
  • Prioritize updating scripts code matching url to browser.storage.local and related indexes, and then update other scripts.

Since the above process is optimized for injection speed, the background process may be completed later than the content scripts injection, which may cause the latest scripts code to not be applied when the page refresh/load of first time after the scripts is updated, but this should be acceptable, we can also provide an option switch to let the user decide whether to inject first or version first(For example, it is convenient for user script developers to debug).

The above is just an idea, not tested and finalized, it may be rejected or not implemented for practical reasons.

@ACTCD ACTCD changed the title Improve script injection flow and logic Improve scripts injection flow and logic Sep 28, 2022
@quoid
Copy link
Owner

quoid commented Sep 28, 2022

@ACTCD These are good idea, but I should note that webNavigation.onBeforeNavigate is not reliable. Sometimes it does not return a url in Safari (I reported this bug to Apple a while ago). I believe with this method this could lead to a scenario where we are often attempting to deliver defunct userscripts and/or not getting the newest userscripts as you mentioned in your comment.

@ACTCD
Copy link
Collaborator Author

ACTCD commented Sep 28, 2022

@quoid We can choose any trigger early enough, I haven't evaluated them, but I believe we can surely find one or more available from below:

webNavigation.onBeforeNavigate
webRequest.onBeforeRequest
webRequest.onBeforeSendHeaders
webRequest.onSendHeaders
webRequest.onHeadersReceived
webRequest.onResponseStarted
webRequest.onCompleted
tabs.onUpdated
tabs.onZoomChange
webNavigation.onCommitted
...

Worst case is we don't use url and just update all scripts info, it just lacks priority and won't have much impact.

@quoid
Copy link
Owner

quoid commented Sep 28, 2022

@ACTCD All of the webNavigation have the same issue I described.

tabs is not very viable from my testing, see here

We can not use webRequest because we have a non-persistent background page, which is required for iOS and webRequest required a persistent background page.

@ACTCD
Copy link
Collaborator Author

ACTCD commented Sep 28, 2022

@quoid Thank you so much for your investigation and clarification, it looks like we have the worst possible situation.

For the above idea, we mainly need a trigger as early as possible to prepare and check for scripts updates in advance, as I said above, we can also just forgo the use of url and update the scripts cache sequentially, it should not have too much diffrence.

Another solution, maybe we could find an api in the native layer (swift) to monitor file changes in the scripts folder, actively push the updated files to the background and update the cache information in the storage.

@quoid
Copy link
Owner

quoid commented Sep 28, 2022

Could you elaborate more on how this will work?

  • From content.js we still call REQ_USERSCRIPTS by page sender.url?
    • this is now sent from content.js or is still sent to background.js which makes sendNativeMessage call and returns matched scripts?
  • once we get matched scripts from content.js, where and how do we store those scripts? What does that object look like?
  • is the purpose of webNavigation.onBeforeNavigate to update all script data in browser.storage?
    • does that mean every single navigation event we are asking for all scripts?

@ACTCD
Copy link
Collaborator Author

ACTCD commented Sep 28, 2022

@quoid This is still a very early idea and a lot of work needs to be done before it, I will update with more details in due course.

@ACTCD ACTCD added javascript enhancement Feature work maintenance Chores and refactoring labels Nov 2, 2022
@ACTCD ACTCD self-assigned this Nov 2, 2022
@ACTCD ACTCD added this to the macOS 5.0.0, iOS 2.0.0 milestone Apr 2, 2023
@ACTCD ACTCD modified the milestones: v5.0.0, v6.0.0 Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature work maintenance Chores and refactoring
Projects
None yet
Development

No branches or pull requests

2 participants