Issue 433 - Made adjustments for $sitekey-related changes to the Matcher API

This review not only includes functional changes to adjust for the changed Matcher API but also appropriate renaming of variables due to the fact that sitekeys are no longer limited to exception rules and to make them more consistent with the names used in Firefox.

[Wladimir Palant, 2014-09-10 18:27:04]

Please note that the module owner here is Sebastian. So while he doesn't need to
review all the code, he has to approve the changes. Please add him to Cc for
this review.

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/incl...
include.preload.js:65: ext.backgroundPage.sendMessage({type: "add-sitekey",
sitekey: attr});
IMHO, the parameter name "token" is better left as is - this isn't the sitekey
you are sending, it's the combination of sitekey and signature.

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
lib/whitelisting.js:59: if (frame === frame.parent)
This isn't the DOM, it's our own structure - frame and frame.parent cannot be
equal, frame.parent can be null however.

The correct iterating solution is right above your code:

  for (; frame != null; frame = frame.parent)

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
lib/whitelisting.js:101: let signature = keydata[3];
Why not use the same approach as in Firefox?

  if (token.indexOf("_") < 0)
    return;

  let [key, signature] = token.split("_", 2);
  key = key.replace(/=/g, "");
 
Implementing two different approaches for the same thing means twice as many
bugs. In particular, the regular expression above doesn't look like it would
always work correctly (I'd definitely include ^ and $) and removing trailing =
signs from the key explicitly is unnecessary as the regexp should have done it
already.

Looked up who introduced this code: apparently, Sebastian has hidden this change
in a much larger and unrelated review
(http://codereview.adblockplus.org/6346177440120832/). For reference: please
never do this...

[Thomas Greiner, 2014-09-15 09:24:59]

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/incl...
File include.preload.js (right):

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/incl...
include.preload.js:65: ext.backgroundPage.sendMessage({type: "add-sitekey",
sitekey: attr});
On 2014/09/10 18:27:04, Wladimir Palant wrote:
> IMHO, the parameter name "token" is better left as is - this isn't the sitekey
> you are sending, it's the combination of sitekey and signature.

Done.

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
lib/whitelisting.js:59: if (frame === frame.parent)
On 2014/09/10 18:27:04, Wladimir Palant wrote:
> This isn't the DOM, it's our own structure - frame and frame.parent cannot be
> equal, frame.parent can be null however.
> 
> The correct iterating solution is right above your code:
> 
>   for (; frame != null; frame = frame.parent)

Done.

http://codereview.adblockplus.org/5644946291818496/diff/5629499534213120/lib/...
lib/whitelisting.js:101: let signature = keydata[3];
On 2014/09/10 18:27:04, Wladimir Palant wrote:
> Why not use the same approach as in Firefox?
> 
>   if (token.indexOf("_") < 0)
>     return;
> 
>   let [key, signature] = token.split("_", 2);
>   key = key.replace(/=/g, "");
>  
> Implementing two different approaches for the same thing means twice as many
> bugs. In particular, the regular expression above doesn't look like it would
> always work correctly (I'd definitely include ^ and $) and removing trailing =
> signs from the key explicitly is unnecessary as the regexp should have done it
> already.
> 
> Looked up who introduced this code: apparently, Sebastian has hidden this
change
> in a much larger and unrelated review
> (http://codereview.adblockplus.org/6346177440120832/). For reference: please
> never do this...

Done.

[Wladimir Palant, 2014-09-15 10:11:15]

LGTM, up to Sebastian now.

[Sebastian Noack, 2014-09-16 15:59:57]

LGTM