Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(276)

Issue 29773630: Issue 5761 - Changes require paths to relative

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 1 week ago by Jon Sonesen
Modified:
1 week, 3 days ago
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 5761 - Changes require paths to relative

Patch Set 1 #

Total comments: 2

Patch Set 2 : Address PS1 comment #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+23 lines, -22 lines) Patch
M background.js View 1 chunk +1 line, -1 line 2 comments Download
M lib/antiadblockInit.js View 1 chunk +6 lines, -6 lines 0 comments Download
M messageResponder.js View 1 4 chunks +16 lines, -15 lines 0 comments Download

Messages

Total messages: 7
Jon Sonesen
2 months, 1 week ago (2018-05-08 01:03:37 UTC) #1
Jon Sonesen
https://codereview.adblockplus.org/29773630/diff/29773631/background.js File background.js (right): https://codereview.adblockplus.org/29773630/diff/29773631/background.js#newcode93 background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)]; this is a bit hacky ...
2 months, 1 week ago (2018-05-08 01:04:26 UTC) #2
kzar
https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js File messageResponder.js (right): https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js#newcode40 messageResponder.js:40: const info = require("../buildtools/info"); It turns out using a ...
2 months ago (2018-05-17 13:53:28 UTC) #3
Jon Sonesen
On 2018/05/17 13:53:28, kzar wrote: > https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js > File messageResponder.js (right): > > https://codereview.adblockplus.org/29773630/diff/29773631/messageResponder.js#newcode40 > ...
2 months ago (2018-05-18 17:18:12 UTC) #4
kzar
On 2018/05/18 17:18:12, Jon Sonesen wrote: > Ack Thanks
1 month, 4 weeks ago (2018-05-22 12:43:49 UTC) #5
a.giammarchi
a minor note on the name resolution just because I've read kzar note. If that's ...
2 weeks, 3 days ago (2018-07-02 08:14:26 UTC) #6
Jon Sonesen
1 week, 3 days ago (2018-07-09 19:23:49 UTC) #7
https://codereview.adblockplus.org/29773630/diff/29785577/background.js
File background.js (right):

https://codereview.adblockplus.org/29773630/diff/29785577/background.js#newco...
background.js:93: return modules[module.substr(module.lastIndexOf("/") + 1)];
On 2018/07/02 08:14:26, a.giammarchi wrote:
> this is probably already fine but assumes there is a `/` in the path.
> 
> What always works and produces same result instead is
`module.split('/').pop()`,
> so that even if there's no slash in the path, you'll have a module to load
(i.e.
> the "info" one, whenever it gets involved in here).
> 
> Feel free to ignore this comment if there won't ever be a case without a
slash.

Yeah, previously there were never `/` characters but now i think they will
always appear (perhaps the info module is the exception here) Thomas, Dave what
do you think?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5