Gracefully handle toolbar button mashing #3222
Conversation
| }); | ||
| return lastPromise.then(() => { | ||
| return Promise.all(filePromises).then(fileContents => { | ||
| let allScripts = fileContents.reduce((concatted, script) => { return concatted + ' ; ' + script }); |
There was a problem hiding this comment.
Couldn't this be fileContents.join("; ") ?
There was a problem hiding this comment.
One bummer here is that we no longer get filenames or line positions. This is going to make debugging and Sentry much harder to work with.
I guess we could do a sourcemap? Though if we do, then using something like browserify (and doing this all as a make step) might be advantageous. I did a quick lookup of the sourcemap format, since maybe for pure concatenation it isn't that hard, but it looks pretty complicated. Though maybe it's not that complicated, since it looks like something like {offset: {line: 10}, map: {file: "aScript.js"}}} might work?
| @@ -64,6 +64,11 @@ this.selectorLoader = (function() { | |||
| let loadingTabs = new Set(); | |||
|
|
|||
| exports.loadModules = function(tabId, hasSeenOnboarding) { | |||
| // Sometimes loadModules is called when the tab is already loading. | |||
| // Adding a check here seems to help avoid catastrophe. | |||
| if (loadingTabs.has(tabId)) { | |||
There was a problem hiding this comment.
Won't this get in the way of loading modules on the same tab, when it's a new page? E.g., just reload the page and try to re-invoke screenshots, and then this check will stop it from loading...?
| @@ -80,21 +85,43 @@ this.selectorLoader = (function() { | |||
| }); | |||
| }; | |||
|
|
|||
| function readFile(path) { | |||
| return fetch(path, { mode: 'same-origin' }).then((response) => { | |||
| return response.blob(); | |||
There was a problem hiding this comment.
Can this be response.text() instead of response.blob(), skipping the FileReader stuff?
|
After discussion, some options to explore:
If we want to make minimal logic changes, then we should build two files, one that includes slides.js/etc, and one that does not. Right now slides.js (onboarding) runs based on whether the files are included. For the last option (where we use browserify properly) we probably have to change how onboarding is invoked. Maybe we can just use a query string on the script name? |
|
I'm now thinking this is the wrong approach. Closing |
This patch consistently fixes #3097 for me. Two parts:
When loading selector code into a page, manually concatenate all scripts, then kick off 1
browser.tabs.executeScriptrequest. The old approach kicked off NexecuteScriptpromises sequentially, one for each script.Double-check the list of already-loading tabs just before starting to load code into a tab. Although this check is done elsewhere already, adding a check at the moment of truth seems to fix the remaining few weird timing bugs that lead to 'haywire' notifications.