CRX: make disabling extension during run, optional #1604
Conversation
|
Tests are shoppy shop. Shop is the worst. |
| @@ -207,9 +209,11 @@ function initPopup() { | |||
| const okButton = document.getElementById('ok'); | |||
| okButton.addEventListener('click', () => { | |||
| // Save selected aggregation categories on options page close. | |||
There was a problem hiding this comment.
"Save settings on options page close"?
There was a problem hiding this comment.
Done.
// Save settings when options page is closed.
|
|
||
| let installedExtensions = []; | ||
| let DISABLE_EXTENSIONS_DURING_RUN = false; |
There was a problem hiding this comment.
not a constant so shouldn't be all caps
| * @param {!Array<{name: string, audits: !Array<string>}>} selectedAggregations | ||
| * @param {!{selectedAggregations: Array<{name: string, audits: !Array<string>}>, | ||
| * disableExtensions: boolean}} settings | ||
| * @param {!boolean} disableExtensions True if extensions should be disabled for the run. |
There was a problem hiding this comment.
not separate params, just one object
There was a problem hiding this comment.
leftover from tinkering, done.
maybe (in a future PR) we can move it into a separate background script, then, so other clients won't get the extension-specific stuff |
Or we flip it and have something special for devtools. It's funky the devtools using the built background file from the extension. Those should really be separate targets. |
|
PTAL |
|
Tests are green |
| @@ -251,41 +257,56 @@ window.getDefaultAggregations = function() { | |||
|
|
|||
| /** | |||
| * Save currently selected set of aggregation categories to local storage. | |||
| * @param {!Array<{name: string, audits: !Array<string>}>} selectedAggregations | |||
| * @param {!{selectedAggregations: Array<{name: string, audits: !Array<string>}>, | |||
There was a problem hiding this comment.
somewhere along the line this became wrong. I believe its @param {{selectedAggregations: !Array<string>, disableExtensions: boolean}} settings
| chrome.storage.local.set(storage); | ||
| }; | ||
|
|
||
| /** | ||
| * Load selected aggregation categories from local storage. | ||
| * @return {!Promise<!Object<boolean>>} | ||
| * @return {!Promise<{selectedAggregations: Array<{name: string, audits: !Array<string>}>, |
There was a problem hiding this comment.
this one was right before, so @return {!Promise<{selectedAggregations: !Object<boolean>, disableExtensions: boolean}>}
|
Woot! |
R: @brendankenny, all
This makes the enabling/disabling of extension before a LH run a user setting:
My worry was that some users would find the current default behavior (disable all extension before the run) unexpected and frustrating. Any extensions that don't save state when they're killed off will lose data. I suspect there are many that don't follow best practices. Instead, we're making this a setting for people to opt into.
@brendankenny I tried to move some of the bg methods into popup.html like we chatted about but it was getting trickier than it was worth. We allow the user to dismiss the popup while LH is running. It was getting gnarly to make a transient popup communicate state back to the bg page for persistence, then repopulate the UI when the user reopens the popup. I was also finding myself piping more
backgroundpage variables through popup.js methods...which defeated the original purpose of the code refactor (separation of concerns between bg page and popup.js).