Issue 29828610: Issue 6791 - Implement abort-current-inline-script snippet (Closed)
|
Created:
July 12, 2018, 5:38 p.m. by Manish Jethani Modified:
July 17, 2018, 9:52 a.m. CC:
Sebastian Noack Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionBased on https://codereview.adblockplus.org/29828604/
Patch Set 1 #Patch Set 2 : Refactor #Patch Set 3 : Return if target not found #Patch Set 4 : Add attribution #Patch Set 5 : Rebase #
Total comments: 4
MessagesTotal messages: 16
I've received a complaint that this code has been copied from another project, without attribution. Please, could you confirm that you wrote all this code yourself, or if not where you took it from?
On 2018/07/13 10:21:15, kzar wrote: > I've received a complaint that this code has been copied from another project, > without attribution. Please, could you confirm that you wrote all this code > yourself, or if not where you took it from? This is based on the same snippet that exists in uBlock Origin. It's not literally copied but I'd say "inspired by". Anyway, we should give attribution to the original code, I'm not sure what is the best way to do it.
Patch Set 2: Refactor Refactored now to split out commonly used functionality into independent functions.
On 2018/07/13 10:56:25, Manish Jethani wrote: > This is based on the same snippet that exists in uBlock Origin. > It's not literally copied but I'd say "inspired by". and in IRC... > 12:08 <manish> We are now literally copying and pasting code from > uBlock Origin... Those two statements seem to be contradict, either that or there's other code which has been literally copied? > Anyway, we should give attribution to the original code, I'm not sure what is the best way to do it. Yes, we should definitely give proper attribution for code that we have not written. We should also mention that in the codereview, and link to where the code came from. I'm not happy about this situation, for now NOT LGTM.
On 2018/07/13 12:18:08, kzar wrote: > On 2018/07/13 10:56:25, Manish Jethani wrote: > > This is based on the same snippet that exists in uBlock Origin. > > It's not literally copied but I'd say "inspired by". > > and in IRC... > > > 12:08 <manish> We are now literally copying and pasting code from > > uBlock Origin... > > Those two statements seem to be contradict, either that or there's other code > which has been literally copied? Yes, the other snippet is literally copied (it's only one line). Also this one is now refactored, but of course it implements the exact technique used by another ad blocker. > > Anyway, we should give attribution to the original code, I'm not sure what is > the best way to do it. > > Yes, we should definitely give proper attribution for code that we have not > written. We should also mention that in the codereview, and link to where the > code came from. Can you look at the latest patch and tell me if it's a copy or a reimplementation of the technique used by another ad blocker? Also, where do I ad the link? I already mentioned in the Trac issue that this is an implementation of uBlock Origin's technique.
On 2018/07/13 12:26:24, Manish Jethani wrote: > Also, where do I ad the link? I already mentioned in the Trac issue that this is > an implementation of uBlock Origin's technique. I can mention in the source comments that the function is an implementation of the same technique used by uBlock Origin and link to the original function in the uAssets repo. Let me know if you think this is the way to go.
On 2018/07/13 12:26:24, Manish Jethani wrote: > Yes, the other snippet is literally copied (it's only one line). Please could you link to that, and where it came from as well?
On 2018/07/13 12:26:24, Manish Jethani wrote: > Can you look at the latest patch and tell me if it's a copy or a > reimplementation of the technique used by another ad blocker? Well, no I can't actually. That's up to you as the author to be clear which code you've written, which is copied, and which was based upon something else. As a reviewer I assume that the code I'm reviewing is original unless it's clearly labelled. Give Raymond the proper credit for his code, even if you refactored it. I write a comment something like "// This code is based upon ... <link>", and would also mention in the code review, but I'm not a lawyer. Do you know if I got it right Sebastian? > Also, where do I ad the link? I already mentioned in the Trac issue that this is > an implementation of uBlock Origin's technique. Attribution belongs with the code, as it stood this file only had our license and copyright header "Copyright (C) 2006-present eyeo GmbH", which would lead people to think that we wrote the code.
Patch Set 3: Return if target not found Patch Set 4: Add attribution
On 2018/07/13 12:33:28, kzar wrote: > On 2018/07/13 12:26:24, Manish Jethani wrote: > > Yes, the other snippet is literally copied (it's only one line). > > Please could you link to that, and where it came from as well? This is the one: https://codereview.adblockplus.org/29828604/ I'll update that patch to add a comment linking to uBlock Origin's implementation.
On 2018/07/13 12:43:11, kzar wrote: > On 2018/07/13 12:26:24, Manish Jethani wrote: > > Can you look at the latest patch and tell me if it's a copy or a > > reimplementation of the technique used by another ad blocker? > > Well, no I can't actually. That's up to you as the author to be clear which code > you've written, which is copied, and which was based upon something else. As a > reviewer I assume that the code I'm reviewing is original unless it's clearly > labelled. OK, so this is not a copy of the source code, it's an implementation of the same technique. > Give Raymond the proper credit for his code, even if you refactored it. Sure, I have added attribution in the comments now. > > Also, where do I ad the link? I already mentioned in the Trac issue that this > is > > an implementation of uBlock Origin's technique. > > Attribution belongs with the code, as it stood this file only had our license > and copyright header "Copyright (C) 2006-present eyeo GmbH", which would lead > people to think that we wrote the code. OK, to clarify, we did write the code (in this patch). This is not literal copy and paste, hence I don't think it's a question of copyright. But IANAL and I'll check with some people to be sure.
Patch Set 5: Rebase
On 2018/07/13 12:18:08, kzar wrote: > On 2018/07/13 10:56:25, Manish Jethani wrote: > > This is based on the same snippet that exists in uBlock Origin. > > It's not literally copied but I'd say "inspired by". > > and in IRC... > > > 12:08 <manish> We are now literally copying and pasting code from > > uBlock Origin... > > Those two statements seem to be contradict, either that or there's other code > which has been literally copied? By the way, the statement on IRC was in reference to the "plan" of how to do things. We have not started doing things this way, and it is impractical, because almost all of the code needs to be modified to fit in ABP's framework and coding style. We might as well implement the same thing from scratch.
Message was sent while issue was closed.
https://codereview.adblockplus.org/29828610/diff/29829567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29828610/diff/29829567/lib/content/snippet... lib/content/snippets.js:111: window.onerror = function(message, ...rest) The content script part of our snippet already runs after the page script, so the injected part definitely does. I wonder what the point of trying to wrap `onerror` is therefore. Furthermore this wrapper seems pretty easy to workaround, for example the website could mess with `String.prototype.includes` so that `message.includes(magic)` returns `false`. https://codereview.adblockplus.org/29828610/diff/29829567/lib/content/snippet... lib/content/snippets.js:144: function abortCurrentInlineScript(target, needle) Could you give me an example of `target` and `needle` with some context? It would help me grok this logic. https://codereview.adblockplus.org/29828610/diff/29829567/lib/content/snippet... lib/content/snippets.js:164: if (descriptor && descriptor.get) What's the idea with this check? https://codereview.adblockplus.org/29828610/diff/29829567/lib/content/snippet... lib/content/snippets.js:175: throw new ReferenceError(magic); How come you've used `ReferenceError` out of interest?
Message was sent while issue was closed.
This review is closed. 
|
