Refactor ScriptDownloader #1458

Closed
arantius opened this Issue Oct 28, 2011 · 5 comments

Comments

Projects
None yet
3 participants
Collaborator

arantius commented Oct 28, 2011

I've noticed (mostly) recently that ScriptDownloader has grown poorly as we've increased the number of ways it's used:

  • Only viewing a remote script (before installation)
  • Installing an entire, new, remote script (including all dependencies)
  • Updating a locally modified script (possibly loading dependencies)
  • Updating a remotely modified script (thanks to update checking)

Especially, some of the method names are making less sense, now that more than one kind of updating is being done. In light of #1419 and #1156, I believe it's time to tackle this problem.

Collaborator

arantius commented Nov 14, 2011

I've made quite a bit of progress here. I'm not at all done. (Don't check out / base work on this branch. I'm rewriting these commits, and have got them public just to share work between machines, and at this point to report progress.) The core concept here is a "remote script" that knows how to download itself. Most of the operations then rely on passing this remote script object to something that knows how to deal with it (GM_BrowserUI knows how to do the 'view source', install.js knows how to confirm it via dialog, Config will know how to install, etc.). As a result things today scattered throughout the codebase are being rearranged.

arantius/greasemonkey@master...refactor-scriptdownloader

zorked commented Nov 15, 2011

I understand this is incomplete but remoteScript.js#L148 won't work since it has already been tried. Metadata blocks can reside anywhere in a file. If an author places them at or near the end of the file GM will cause a DDoS. If the headers are set on the xXHR to retrieve x amount of lines assuming all server supports this which they don't. this also has the same issue. Cache systems don't always allow those headers to be passed through and/or caching will end up returning the entire file.

Collaborator

arantius commented Nov 15, 2011

I understand this is incomplete but remoteScript.js#L148 won't work

I'm mostly thinking that if we're checking for updates, we could call this method, and it would immediately abort the download once it has found the metadata. Since in the vast majority of cases the script metadata at the top, this would mean downloading just a few kB of data, rather than the entire script every time. In the worst case, we would end up downloading the whole script instead of downloading the whole script: i.e. not any worse.

But as commented, at this point it's a half formed idea that could easily be scrapped.

Contributor

sizzlemctwizzle commented Nov 18, 2011

I'm mostly thinking that if we're checking for updates, we could call this method, and it would immediately abort the download once it has found the metadata.

Not worth it imo. If users want to host a script and save their bandwidth, they can use @updateurl and point it to a file with just the metadata (or even just a header with an @version).

arantius added a commit to arantius/greasemonkey that referenced this issue Nov 19, 2011

arantius added a commit to arantius/greasemonkey that referenced this issue Nov 19, 2011

Collaborator

arantius commented Nov 19, 2011

This branch is finally to the point where the RemoteScript object works for (at least some) installs. Just need to finish the other cases where scriptDownloader is used (mostly updates) and then it can be removed.

arantius added a commit to arantius/greasemonkey that referenced this issue Dec 1, 2011

arantius added a commit to arantius/greasemonkey that referenced this issue Dec 1, 2011

Fix location redirects html detection.
Always pass the "current" request/channel around for checking its headers, rather than currying in the initial channel to the callbacks.

Refs #1458

@arantius arantius closed this in b3f8572 Dec 2, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment