Adds url whitelist regex to ext options #410
Conversation
| * Adds named callbacks to be executed when logging. | ||
| * @param {Object } newCallbacks An object containing named callback functions. | ||
| */ | ||
| function addCallbacks() { |
There was a problem hiding this comment.
Please add some comments to this function to help describe what's happening here at a business logic level.
| return false; | ||
| } | ||
| browser.storage.local.get(defaultConfig, function (res) { | ||
| addCallbacks({ |
There was a problem hiding this comment.
Again please add brief explanation of why we need add the filterUrl as a callback since in the lines right below we apply the function directly in the browser event handler.
| function dispatchTabMessage(message) { | ||
| browser.tabs.query({}, function (tabs) { | ||
| tabs.forEach(function (tab) { | ||
| browser.tabs.sendMessage(tab.id, message); | ||
| }); | ||
| }); | ||
| } | ||
| function filterUrl(log) { |
There was a problem hiding this comment.
Code smell to have two different return types depending on conditional. Since you're not actually modifying the log, you don't need to return the log.
Better to have function name like isUrlWhitelisted which only returns bool.
There was a problem hiding this comment.
This function is a little wonky because it has to match the pattern expected by addcallbacks(). Callbacks are called by package log functions and modify logs or drop them if given a falsey value. I've already created a ticket to change this callback functionality to match the pattern you used for modifying headers. Once the core api is changed, we can modify how we're using it here.
Here's some background on why addcallbacks is the way it is:
#312
#350
| browser.runtime.onMessage.addListener(function (message) { | ||
| switch (message.type) { | ||
| // Handles logs rerouted from content and option scripts | ||
| case ADD_LOG: | ||
| log(message.payload); | ||
| var log$1 = filterUrl(message.payload); |
There was a problem hiding this comment.
Then here, you just check if it's whitelisted. if not, return early, otherwise proceed.
if !isUrlWhitelisted(message.payload) return
log(message.payload)There was a problem hiding this comment.
This is an artifact of unclear behavior of the current logging callback api. Callbacks are not executed by log(), so I am mimicking the handling of callback results in packagelog() here in the message handler. This is so there can be a single place where URL filtering logic is defined.
| options(config); | ||
| browser.runtime.sendMessage({ | ||
| type: CONFIG_CHANGE, | ||
| payload: config | ||
| payload: payload |
There was a problem hiding this comment.
FYI: JS will automatically infer the key if your value of the same name.
So {type: CONFIG_CHANGE, payload } is perfectly valid. Saves you a few characters of typing.
Adds an extension option to filter urls. Only urls that match the regex will be logged, this includes tab events.
By default the extension will block all urls. Currently all urls are logged by default, but this change is intended to protect privacy.
Tagging @EandrewJones for review