[chrismorgan]

Some code review just in case anyone is interested. (I don’t expect it to make it into the article as it was written six months ago.)

This pattern:

  if (details.url.includes(url)) {
    return true;
  }
  return false;

should be replaced by this:

  return details.url.includes(url);

This pattern:

  array.map(someFunction).reduce(function(a, b) { return a ||b}, false)

should be replaced by this:

  array.some(someFunction)

(Note the semantics are slightly different—`.some` will break early, so it’s more efficient and equivalent provided there are no side-effects in the map function.)

Taking both of these, the following:

  var useTwitter = VIA_TWITTER.map(function(url) {
    if (details.url.includes(url)) {
      return true;
    }
    return false;
  })
  .reduce(function(a, b) { return a || b}, false);

can be rewritten much more simply as:

  var useTwitter = VIA_TWITTER.some(function(url) {
    return details.url.includes(url);
  }

You could even do it thus if you desired:

  var useTwitter = VIA_TWITTER.some(details.url.includes.bind(details.url));

… but that’s probably harder to read. I will mention arrow functions, however, which are pretty:

  var useTwitter = VIA_TWITTER.some(url => details.url.includes(url));

This part:

  details.requestHeaders.filter(function(header) {

    // block cookies by default
    if (header.name !== "Cookie") {
      return header;
    } 

  })

`.filter` only cares about truthiness in its return value—as this code does it, undefined is false and an object is true. But you could simplify it:

  details.requestHeaders.filter(function(header) {
    // block cookies by default
    return header.name !== "Cookie";
  })

Also in the original code’s usage of map, it’s not actually changing the values, only things inside them, so using `map` is wasteful (as it entails allocating a new array). You could just use `forEach`:

  var reqHeaders = …;
  reqHeaders.forEach(function(header) {
    if (header.name === "Referer") {
      header.value = setRefer(useTwitter);
      foundReferer = true;
    }
    if (header.name === "User-Agent") {
      header.value = setUserAgent(useTwitter);
      foundUA = true;
    }
  });

A remark on fine-tuning performance: when you access properties inside an object multiple times, it’s optimal to store it as a local variable to save having to look it up multiple times. (This is especially the case if the property is expensive to access.) Take the `blockCookies` method:

  function blockCookies(details) {
    for (var i = 0; i < details.responseHeaders.length; ++i) {
      if (details.responseHeaders[i].name === "Set-Cookie") {
        details.responseHeaders.splice(i, 1);
      }
    }
    return {responseHeaders: details.responseHeaders};
  }

This is accessing `details.responseHeaders` many times when it only needs to access it once. It is also accessing its `length` member once per iteration, rather than caching the length. Normally for that I’d say “store the length once up front,” but in this case the code is changing the array length in the loop, so that’d actually break things. On that note, the code as published is actually missing some cookies, because it removes an item from the array and then skips past the new element at that index. To fix that, you need to move the `++i` into the loop so it can be skipped if you do splice the array. Also in order to not need to access the length property many times you could iterate in reverse instead of forwards. I might write the whole function like this:

  function blockCookies(details) {
    var headers = details.responseHeaders;
    var i = headers.length - 1;
    while (i > -1) {
      if (headers[i].name === "Set-Cookie") {
        headers.splice(i, 1);
      } else {
        i--;
      }
    }
    return {responseHeaders: headers};
  }

[elaineo]

Awesome code review, even more valuable than the plugin itself. Thanks :)

[mattmanser]

You can actually just do this:

    for (var i = 0, l = details.responseHeaders.length; i < l; ++i) {

[chrismorgan]

I would have done that, but because of the splicing you don’t actually want ++i every time. That’s why I rewrote it to a while loop.

Even then you could do it as a for loop with an empty increment clause, of course. Or even use a one-liner for loop with no body:

  function blockCookies(details) {
    for (var headers = details.responseHeaders, i = headers.length - 1; i > -1; headers[i].name === "Set-Cookie" ? headers.splice(i, 1) : i--);
    return {responseHeaders: headers};
  }

But that’s crazy talk. Leave optimisations that to the minifier if it feels so disposed.