Issue 4580 - Removed ext.webRequest.onBeforeRequest

Issue 4580 - Removed ext.webRequest.onBeforeRequest

[kzar, 2018-04-05 11:26:27]

Otherwise seems like a nice change and LGTM

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:107: return;
Please could you add the braces here, to be consistent with the below condition
(once my feedback is addressed).

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:116: return;
Please can you add the braces back, since the rule is if the condition or the
body spans multiple lines you have to add them.

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:119: let frame = ext.getFrame(
Seems like previously we didn't call getFrame if details.tabId was -1 but now we
do. I wonder if that matters?

[Sebastian Noack, 2018-04-05 16:04:09]

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:107: return;
On 2018/04/05 11:26:26, kzar wrote:
> Please could you add the braces here, to be consistent with the below
condition
> (once my feedback is addressed).

See below.

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:116: return;
On 2018/04/05 11:26:26, kzar wrote:
> Please can you add the braces back, since the rule is if the condition or the
> body spans multiple lines you have to add them.

Since when is this the rule? This is the first time hear that we insist on
braces if the block is only one line but the condition has multiple lines. It's
not in our coding style guide, nor does our ESLint config complain.

This rule doesn't make any sense to me. Braces group the block, and if it has
more than one line (in particular with nested logic) braces avoid
misinterpretation. I don't see what this has to do with the condition.

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:119: let frame = ext.getFrame(
On 2018/04/05 11:26:26, kzar wrote:
> Seems like previously we didn't call getFrame if details.tabId was -1 but now
we
> do. I wonder if that matters?

getFrame() just returns undefined if there is not tab or frame with the given
ID. If the tabId is -1 it will effectively always return undefined.

[Manish Jethani, 2018-04-06 14:12:48]

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker.js
File lib/requestBlocker.js (right):

https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
lib/requestBlocker.js:116: return;
On 2018/04/05 16:04:09, Sebastian Noack wrote:
> On 2018/04/05 11:26:26, kzar wrote:
> > Please can you add the braces back, since the rule is if the condition or
the
> > body spans multiple lines you have to add them.
> 
> Since when is this the rule? [...]

As far back as I can remember (which is not too far since I've been around for
only a year), Dave has always insisted on this, and I have made it a point to do
it. To be fair a lot of the rules don't make sense, our style guide needs an
upgrade at some point.

For what it's worth, when there's an else block, I also add the braces around
the else block now to "balance it out".

e.g.

  if (foo ||
      bar)
  {
    doSomething();
  }
  else
  {
    doSomethingElse();
  }

If the condition fit on one line then I would drop the braces from both blocks.

Anyway, my two cents. I'm not a big fan of these rules myself.

[kzar, 2018-04-09 14:53:25]

On 2018/04/06 14:12:48, Manish Jethani wrote:
>
https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker.js
> File lib/requestBlocker.js (right):
> 
>
https://codereview.adblockplus.org/29737568/diff/29737569/lib/requestBlocker....
> lib/requestBlocker.js:116: return;
> On 2018/04/05 16:04:09, Sebastian Noack wrote:
> > On 2018/04/05 11:26:26, kzar wrote:
> > > Please can you add the braces back, since the rule is if the condition or
> the
> > > body spans multiple lines you have to add them.
> > 
> > Since when is this the rule? [...]
> 
> As far back as I can remember (which is not too far since I've been around for
> only a year), Dave has always insisted on this, and I have made it a point to
do
> it. To be fair a lot of the rules don't make sense, our style guide needs an
> upgrade at some point.
> 
> For what it's worth, when there's an else block, I also add the braces around
> the else block now to "balance it out".
> 
> e.g.
> 
>   if (foo ||
>       bar)
>   {
>     doSomething();
>   }
>   else
>   {
>     doSomethingElse();
>   }
> 
> If the condition fit on one line then I would drop the braces from both
blocks.
> 
> Anyway, my two cents. I'm not a big fan of these rules myself.

Well I discussed this with Sebastian in IRC, I was just trying to be consistent
with what I've been told. IIRC Wladimir's logic was that sometimes multiple line
clauses followed by a one line body can be easy to misread at a glance, so even
though the braces make things a little uglier it makes mistakes less likely.

That said I don't have the time (or energy TBH) to go back through my old emails
to back this up. Since you guys disagree with the rule I'll just stop mentioning
this in codereviews, if someone wants to make a second push for getting
everybody to agree on the rules for braces + updating the ESLint configuration
then be my guest! I'd warn you that it's quite demotivating however!