Skip to content
This repository has been archived by the owner on Jan 17, 2023. It is now read-only.

Gracefully handle toolbar button mashing #3222

Closed
wants to merge 2 commits into from
Closed

Gracefully handle toolbar button mashing #3222

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
583cc67
Load scripts by concatting manually, calling executeScript just once
jaredhirsch Jul 26, 2017
92ea432
Double-check loading tabs list before running loadModules
jaredhirsch Jul 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions addon/webextension/background/selectorLoader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...?

return Promise.resolve(null);
}
let promise;
loadingTabs.add(tabId);
if (hasSeenOnboarding) {
Expand All @@ -80,21 +85,43 @@ this.selectorLoader = (function() {
});
};

function readFile(path) {
return fetch(path, { mode: 'same-origin' }).then((response) => {
return response.blob();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be response.text() instead of response.blob(), skipping the FileReader stuff?

}).then(blob => {
let reader = new FileReader();
return new Promise((resolve, reject) => {
reader.readAsText(blob);
reader.onload = (evt) => {
let contents = evt.target.result;
resolve(contents);
};
reader.onerror = (evt) => {
reject(evt.target.error);
};
});
});
}

function executeModules(tabId, scripts) {
let lastPromise = Promise.resolve(null);
let filePromises = [];
scripts.forEach((file) => {
lastPromise = lastPromise.then(() => {
return browser.tabs.executeScript(tabId, {
file,
runAt: "document_end"
}).catch((error) => {
log.error("error in script:", file, error);
error.scriptName = file;
throw error;
});
});
let fileURL = browser.extension.getURL(file);
let fileContents = readFile(fileURL);
filePromises.push(fileContents);
});
return lastPromise.then(() => {
return Promise.all(filePromises).then(fileContents => {
let allScripts = fileContents.reduce((concatted, script) => { return concatted + ' ; ' + script });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be fileContents.join("; ") ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return Promise.resolve(allScripts);
}).then(scripts => {
return browser.tabs.executeScript(tabId, {
code: scripts,
runAt: "document_start"
}).catch((error) => {
log.error("error in script:", error);
throw error;
});
}).then(() => {
log.debug("finished loading scripts:", scripts.join(" "));
},
(error) => {
Expand Down