Issue 5760 - Use relative require paths

Issue 5760 - Use relative require paths

[Sebastian Noack, 2018-03-06 21:18:17]

I added Dave as reviewer, since he requested this change, and is module owner
peer. (I might have a look myself later.)

[kzar, 2018-03-19 21:50:08]

Sorry for the delay, please can you rebase this on top of the next branch?

Also please can you verify your changes do not change the build? (Do a devbuild
from next branch, initialise that as a Git / Mercurial repostiory, do a devbuild
from this, move the .git (or whatever) folder across and then check for changed
files.)

https://codereview.adblockplus.org/29713631/diff/29713632/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/include.preload.js#...
include.preload.js:21: let {ElemHideEmulation} = require(
Please indent this like 

let {ElemHideEmulation} =
  require("./adblockpluscore/lib/content/elemHideEmulation");

or

let {ElemHideEmulation} = require(
  "./adblockpluscore/lib/content/elemHideEmulation"
);

(Same with the other long ones you wrapped.)

https://codereview.adblockplus.org/29713631/diff/29713632/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/filterComposer....
lib/filterComposer.js:290: if (require(
Since info is already required can't you just use that? Also the way this is
wrapped is really ugly now.

https://codereview.adblockplus.org/29713631/diff/29713632/lib/popupBlocker.js
File lib/popupBlocker.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/popupBlocker.js...
lib/popupBlocker.js:23: const {BlockingFilter, RegExpFilter} = require(
Better to do it like this in this instance:

const {BlockingFilter,
       RegExpFilter} = require("../adblockpluscore/lib/filterClasses");

https://codereview.adblockplus.org/29713631/diff/29713632/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/prefs.js#newcod...
lib/prefs.js:344: "managed" in browser.storage)
Please fix the whitespace here, `"managed"` should be inline with `require`.

https://codereview.adblockplus.org/29713631/diff/29713632/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/subscriptionIni...
lib/subscriptionInit.js:23: SpecialSubscription} = require(
Please fix the way this is wrapped too.

[kzar, 2018-03-20 13:48:18]

Also what about thes test files? Specifically:

 - qunit/common.js
 - qunit/tests/url.js
 - qunit/tests/prefs.js
 - qunit/tests/filterValidation.js
 - qunit/tests/cssEscaping.js

[Jon Sonesen, 2018-03-20 23:25:18]

Thanks for looking at this, no worries on the delay there were alot of reviews
in queue. 

Updated the tests files and addressed comments

Cheers

https://codereview.adblockplus.org/29713631/diff/29713632/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/include.preload.js#...
include.preload.js:21: let {ElemHideEmulation} = require(
On 2018/03/19 21:50:07, kzar wrote:
> Please indent this like 
> 
> let {ElemHideEmulation} =
>   require("./adblockpluscore/lib/content/elemHideEmulation");
> 
> or
> 
> let {ElemHideEmulation} = require(
>   "./adblockpluscore/lib/content/elemHideEmulation"
> );
> 
> (Same with the other long ones you wrapped.)

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29713632/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/filterComposer....
lib/filterComposer.js:290: if (require(
On 2018/03/19 21:50:07, kzar wrote:
> Since info is already required can't you just use that? Also the way this is
> wrapped is really ugly now.

Acknowledged. This was a change done with a bash script, I guess reviewing the
diff I didn't notice that it was already required. Thanks for catching it!

https://codereview.adblockplus.org/29713631/diff/29713632/lib/popupBlocker.js
File lib/popupBlocker.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/popupBlocker.js...
lib/popupBlocker.js:23: const {BlockingFilter, RegExpFilter} = require(
On 2018/03/19 21:50:08, kzar wrote:
> Better to do it like this in this instance:
> 
> const {BlockingFilter,
>        RegExpFilter} = require("../adblockpluscore/lib/filterClasses");

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29713632/lib/prefs.js
File lib/prefs.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/prefs.js#newcod...
lib/prefs.js:344: "managed" in browser.storage)
On 2018/03/19 21:50:08, kzar wrote:
> Please fix the whitespace here, `"managed"` should be inline with `require`.

Acknowledged. Since rebase this require is not needed

https://codereview.adblockplus.org/29713631/diff/29713632/lib/subscriptionIni...
File lib/subscriptionInit.js (right):

https://codereview.adblockplus.org/29713631/diff/29713632/lib/subscriptionIni...
lib/subscriptionInit.js:23: SpecialSubscription} = require(
On 2018/03/19 21:50:08, kzar wrote:
> Please fix the way this is wrapped too.

Acknowledged.

[kzar, 2018-03-28 13:50:33]

Looks much better thanks, please could you rebase again?

https://codereview.adblockplus.org/29713631/diff/29728665/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29713631/diff/29728665/include.preload.js#...
include.preload.js:20: let {splitSelector} = require("common");
What about this one? (The file is in adblockpluscore/lib/common.js.)

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer....
lib/filterComposer.js:290: if (info.application == "firefox" &&
You tested this code (the block element tool) still works right?

[Jon Sonesen, 2018-03-30 03:09:56]

Thanks for looking at this again and catching those mistakes!

https://codereview.adblockplus.org/29713631/diff/29728665/include.preload.js
File include.preload.js (right):

https://codereview.adblockplus.org/29713631/diff/29728665/include.preload.js#...
include.preload.js:20: let {splitSelector} = require("common");
On 2018/03/28 13:50:33, kzar wrote:
> What about this one? (The file is in adblockpluscore/lib/common.js.)

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer....
lib/filterComposer.js:290: if (info.application == "firefox" &&
On 2018/03/28 13:50:33, kzar wrote:
> You tested this code (the block element tool) still works right?

Oh man, well I had tested it but got the block element issue confused with
another ticket. Which explains why the behavior was different when testing the
other ticket on the current release. My bad, I fixy

[kzar, 2018-04-03 12:08:58]

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer....
lib/filterComposer.js:290: if (info.application == "firefox" &&
On 2018/03/30 03:09:55, Jon Sonesen wrote:
> On 2018/03/28 13:50:33, kzar wrote:
> > You tested this code (the block element tool) still works right?
> 
> Oh man, well I had tested it but got the block element issue confused with
> another ticket. Which explains why the behavior was different when testing the
> other ticket on the current release. My bad, I fixy

Doing `info.application` doesn't work then? (Seems kind of surprising.)

[Jon Sonesen, 2018-04-03 17:16:56]

On 2018/04/03 12:08:58, kzar wrote:
>
https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer.js
> File lib/filterComposer.js (right):
> 
>
https://codereview.adblockplus.org/29713631/diff/29728665/lib/filterComposer....
> lib/filterComposer.js:290: if (info.application == "firefox" &&
> On 2018/03/30 03:09:55, Jon Sonesen wrote:
> > On 2018/03/28 13:50:33, kzar wrote:
> > > You tested this code (the block element tool) still works right?
> > 
> > Oh man, well I had tested it but got the block element issue confused with
> > another ticket. Which explains why the behavior was different when testing
the
> > other ticket on the current release. My bad, I fixy
> 
> Doing `info.application` doesn't work then? (Seems kind of surprising.)

Nevermind, it is working :/ I don't know what was going on.

[kzar, 2018-04-05 11:14:57]

What about qunit/common.js?
What about background.js in the root of the repository?

Please could you also update the comment in composer.postload.js to use a
relative path in the example where we mention that we'd rather
require("include.preload")?

https://codereview.adblockplus.org/29713631/diff/29741659/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/lib/csp.js#newcode20
lib/csp.js:20: // The webRequest API doesn't support WebSocket connection
blocking in Microsoft
Looks like this needs rebasing (on top of the next branch) again, since this
file has changed now.

https://codereview.adblockplus.org/29713631/diff/29741659/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/lib/filterComposer....
lib/filterComposer.js:290: if (info.application == "firefox" &&
Nit: Wouldn't this condition fit on one line now?

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/cssEsca...
File qunit/tests/cssEscaping.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/cssEsca...
qunit/tests/cssEscaping.js:6: const {escapeCSS, quoteCSS} =
require("filterComposer");
What about this one?

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/filterV...
File qunit/tests/filterValidation.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/filterV...
qunit/tests/filterValidation.js:4: const {parseFilter, parseFilters} =
require("filterValidation");
What about this one?

[Jon Sonesen, 2018-04-05 23:01:21]

Rebase is done comments addressed, sorry for the silly little things again.
Thanks for being patient with me.

https://codereview.adblockplus.org/29713631/diff/29741659/lib/csp.js
File lib/csp.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/lib/csp.js#newcode20
lib/csp.js:20: // The webRequest API doesn't support WebSocket connection
blocking in Microsoft
On 2018/04/05 11:14:56, kzar wrote:
> Looks like this needs rebasing (on top of the next branch) again, since this
> file has changed now.

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29741659/lib/filterComposer.js
File lib/filterComposer.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/lib/filterComposer....
lib/filterComposer.js:290: if (info.application == "firefox" &&
On 2018/04/05 11:14:56, kzar wrote:
> Nit: Wouldn't this condition fit on one line now?

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/cssEsca...
File qunit/tests/cssEscaping.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/cssEsca...
qunit/tests/cssEscaping.js:6: const {escapeCSS, quoteCSS} =
require("filterComposer");
On 2018/04/05 11:14:57, kzar wrote:
> What about this one?

Acknowledged.

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/filterV...
File qunit/tests/filterValidation.js (right):

https://codereview.adblockplus.org/29713631/diff/29741659/qunit/tests/filterV...
qunit/tests/filterValidation.js:4: const {parseFilter, parseFilters} =
require("filterValidation");
On 2018/04/05 11:14:57, kzar wrote:
> What about this one?

Since this file is names filterValidation as well, I guess I though I didn't
need to, my bad.

[kzar, 2018-04-06 10:25:19]

LGTM assuming you've tested. Please push to the next branch instead of the
master branch though!