[meta] Make use of default function parameters

[mjethani]

Background

ES6 supports ​default function parameters.

We have code like this, for example:

function makeSelector(node, selector)
{
  let index = positionInParent(node);

  if (index > 0)
  {
    let newSelector = `${node.tagName}:nth-child(${index})`;
    if (selector)
      newSelector += " > " + selector;

    return makeSelector(node.parentNode, newSelector);
  }

  return selector;
}

Unfortunately everywhere this function is called we are passing an empty string to it as the second argument. The second argument is in fact an implementation detail, the caller shouldn't have to pass a default value here. In such cases the function should already have default values assigned.

What to change

Find instances in the code where it would benefit from the use of default function parameters and make the appropriate changes.

[kzar]

Seems like ​default function arguments are supported by all our supported browsers and I don't see a down side of using them. Unless someone disagrees or I missed something I think we can go ahead with this one.

Please could you open an issue to update our coding style rules page about this? We should also open separate issues if we need to make changes to our other repositories.

[sebastian]

I don't have any objections either, so far. But I generally (not only for this issue), prefer to discuss new suggested coding practices based on a patch showing the changes to our code base.

Replying to mjethani:

Unfortunately everywhere this function is called we are passing an empty string to it as the second argument. The second argument is in fact an implementation detail, the caller shouldn't have to pass a default value here. In such cases the function should already have default values assigned.

For completeness, there are also examples, where the other way around our existing code already emulates default parameters, though boilerplate that is obsolete now, e.g. follwoing example from lib/whitelisting.js in adblockpluschrome:

exports.checkWhitelisted = (page, frame, typeMask) =>
{
  if (typeof typeMask == "undefined")
    typeMask = RegExpFilter.typeMap.DOCUMENT;

  ...
};

If we start using default function parameters, code like this should be changed to:

exports.checkWhitelisted = (page, frame, typeMask = RegExpFilter.typeMap.DOCUMENT) =>
{
  ...
};

[agiammarchi]

Replying to kzar:

Seems like ​default function arguments are supported by all our supported browsers and I don't see a down side of using them.

That very same MDN page states Opera 45 is the one supporting them. I wouldn't be surprised if that'd be just another typo in their JSON but when in doubt, since I'm updating and adding real tests as we go/update code, feel free to ​verify live browsers features.

[agiammarchi]

FYI I've double checked myself and Opera 36 indeed has no issues with default parameters. It looks like there could be some issue with non defaults after defaults (non tested behavior) but I'll write a test myself to be sure it works.

​MDN bug filed

edit tested also regular parameters after default parameters and everything seems to be just fine

[mjethani]

I've made this a meta issue, I'll file separate ones for Core and Platform. Meanwhile let's keep any discussions about this topic here.