machawk1 / Mink

Chrome extension that uses Memento to indicate that a page a user is viewing on the live web has an archived copy and to give the user access to the copy
MIT License
49 stars 3 forks source link

Firefox reports console errors when loading Mink locally #302

Open machawk1 opened 4 years ago

machawk1 commented 4 years ago

Loading as a local temp extensions from 49d9731 (current master). Errors mostly relate to chrome APIs, which may have not been implemented in Firefox (at one point they supported them) or they have been deprecated in WebExtensions.

Latter tied to the anonymous callback (starting with function () {), which admittedly is probably not the best approach but what was needed at the time with Chrome when it was written to get the data where it needed to be.

function displayMinkUI (tabId) {
  chrome.tabs.executeScript(tabId, { code: 'var tmData = ' + JSON.stringify(tmData) + '; var tabId = ' + tabId + ';' },
    function () {
      chrome.tabs.executeScript(tabId, {
        file: 'js/displayMinkUI.js'
      }, function (res) {
      ...
      })
    })
}
Screen Shot 2020-02-11 at 9 41 45 AM

Firefox 72.0.2

machawk1 commented 4 years ago

The Mozilla docs report that getBytesInUse is supported, albeit using the Promises model and a single parameter keys of type "null, string, or array of strings". In 49d9731, two parameters are used, a string (timemaps on line 629 of mink.js) and the anonymous callback function.

Note that in the above instance, the callback does nothing beyond reporting the "return value" on the console, so it call likely be safely removed. The inclusion of this function as a whole is questionable.

machawk1 commented 4 years ago

I found that the invocation with an empty callback does have a purpose here. Per the Chrome docs runtime.error will be set if the retrieval fails. The subsequent logic at line 629 indicates that this is being checked and mitigated.

This error seems to be associated with the times where localStorage is full. The subsequent check of chrome.runtime.lastError.message.indexOf('QUOTA_BYTES_PER_ITEM') manually determines this based on the contents of an error after identifying there was one.

The nearby comment admits that this approach is "chicken wire and duct tap!", so is ripe to be changed, so long as the check and goal of the logic are still retained.

Broadening the scope, this logic is performed as a callback to setting the localStorage key:

chrome.storage.local.set({ 'timemaps': tms }, function () {
  chrome.storage.local.getBytesInUse('timemaps', function (bytesUsed) {...})
  if (chrome.runtime.lastError) {
     ...

Is there a better, compatible way (with the Promise and Callback API differences in-mind) to set localStorage and react more appropriately (e.g., reallocate, reset) to ensure the value set in storage is actually set. I question if the existing logic actually sets the item in the end, since localStorage is cleared on set failure.