Issue 30045566: Fixed #4 - Disable rewrite option for all but internal redirect (Closed)
|
Created:
April 15, 2019, 8:23 p.m. by hub Modified:
April 16, 2019, 2:09 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionFixed #4 - Disable rewrite option for all but internal redirect
Patch Set 1 #Patch Set 2 : Update tests. #
Total comments: 6
Patch Set 3 : Reworked patch #
Total comments: 6
Patch Set 4 : Review fixes. #MessagesTotal messages: 19
Still WiP (I need to fix the tests), but this is the almost minimu, patch to disable $rewrite for all but internal resources.
updated patch
Is there a ticket in adblockpluscore for this? For the commit message, we just have to say "Fixed #1" instead of "Issue adblockpluschrome#1"
https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.j... lib/filterClasses.js:1229: return resourceMap.get(this.resourceName) || url; If `resourceName` is `undefined` or `null` then I'm guessing this automatically falls back to returning `url`. In that case, we could reduce this function to just this line. https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.j... lib/filterClasses.js:1063: if (!value.startsWith("abp-resource:")) `value` could still be `null`?
This condition in `lib/filterClasses.js` (two places) will never be `true` now: rewrite != null && !resourceName This check is now unnecessary: if (this.rewrite == null || this.resourceName)
https://codereview.adblockplus.org/30045566/diff/30045569/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045569/test/filterClasses.... test/filterClasses.js:331: compareFilter(test, "||content.server.com/files/*.php$rewrite=$1", ["type=invalid", "reason=filter_invalid_rewrite", "text=||content.server.com/files/*.php$rewrite=$1"]); Let's add a test for just `$rewrite=` (no value) as well.
The `captureAll` parameter of `filterToRegExp()` is now useless.
On 2019/04/15 20:58:01, Manish Jethani wrote: > Is there a ticket in adblockpluscore for this? Let's use this one: https://gitlab.com/eyeo/adblockplus/adblockpluscore/issues/4 (I'll update it later.)
Following code in RegExpFilter.fromText() will be redundant after these changes,
and therefore should be reomved:
if (rewrite != null && !resourceName)
{
if (contentType == null)
({contentType} = RegExpFilter.prototype);
contentType &= ~(RegExpFilter.typeMap.SCRIPT |
RegExpFilter.typeMap.SUBDOCUMENT |
RegExpFilter.typeMap.OBJECT |
RegExpFilter.typeMap.OBJECT_SUBREQUEST);
}
Also, what would you think about moving this into a GitLab merge request? I know
in our official policy still is to either use Rietveld or GitLab merge requests
at the choice of the developer, but now while using GitLab for issue tracking,
it seems to integrate better if the patch would be submitted through GitLab.
On 2019/04/15 20:58:01, Manish Jethani wrote: > Is there a ticket in adblockpluscore for this? > > For the commit message, we just have to say "Fixed #1" instead of "Issue > adblockpluschrome#1" Fixed #3 actually since it got moved
On 2019/04/16 04:11:40, hub wrote: > On 2019/04/15 20:58:01, Manish Jethani wrote: > > Is there a ticket in adblockpluscore for this? > > > > For the commit message, we just have to say "Fixed #1" instead of "Issue > > adblockpluschrome#1" > > Fixed #3 actually since it got moved Nope, this is about GitLab issue #4, not #3.
On 2019/04/15 22:06:59, Sebastian Noack wrote:
> Following code in RegExpFilter.fromText() will be redundant after these
changes,
> and therefore should be reomved:
>
> if (rewrite != null && !resourceName)
> {
> if (contentType == null)
> ({contentType} = RegExpFilter.prototype);
> contentType &= ~(RegExpFilter.typeMap.SCRIPT |
> RegExpFilter.typeMap.SUBDOCUMENT |
> RegExpFilter.typeMap.OBJECT |
> RegExpFilter.typeMap.OBJECT_SUBREQUEST);
> }
This is gone now as well. I have reworked the patch. (will update soon)
updated patch. https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.j... lib/filterClasses.js:1229: return resourceMap.get(this.resourceName) || url; On 2019/04/15 21:26:37, Manish Jethani wrote: > If `resourceName` is `undefined` or `null` then I'm guessing this automatically > falls back to returning `url`. In that case, we could reduce this function to > just this line. Done. https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045569/lib/filterClasses.j... lib/filterClasses.js:1063: if (!value.startsWith("abp-resource:")) On 2019/04/15 21:26:37, Manish Jethani wrote: > `value` could still be `null`? Yeah. I'll simplify this now that we are going the removal route. https://codereview.adblockplus.org/30045566/diff/30045569/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045569/test/filterClasses.... test/filterClasses.js:331: compareFilter(test, "||content.server.com/files/*.php$rewrite=$1", ["type=invalid", "reason=filter_invalid_rewrite", "text=||content.server.com/files/*.php$rewrite=$1"]); On 2019/04/15 21:48:49, Manish Jethani wrote: > Let's add a test for just `$rewrite=` (no value) as well. Done.
https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:745: if (this.rewrite) This actually has a somewhat opposite of the intended effect. The intention was: If either there is no `$rewrite` or `$rewrite` is `abp-resource:`, then do this. After the changes here, if there is `$rewrite` it will always be `abp-resource:`, consequently the check here is redundant now. We can simply remove the `if` condition. https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:838: * URL. e.g. if the value of the <code>rewrite</code> property is I think we can replace `<code>rewrite</code> property` with `<code>$rewrite</code> option` here because otherwise it would be wrong. https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:1047: if (!value || !value.startsWith("abp-resource:")) It doesn't matter _that_ much, but I'd just like to point out that this is an additional change in behavior. A filter like `foo$rewrite` would given an "unknown option" error previously, while now it would give a "invalid rewrite" error. Maybe that's fine. If we want to stick with the old behavior, then we can leave the check for `value == null` as it is and check for `!value.startsWith(...)` as an additional check.
Updated patch https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:745: if (this.rewrite) On 2019/04/16 05:52:58, Manish Jethani wrote: > This actually has a somewhat opposite of the intended effect. > > The intention was: If either there is no `$rewrite` or `$rewrite` is > `abp-resource:`, then do this. After the changes here, if there is `$rewrite` it > will always be `abp-resource:`, consequently the check here is redundant now. We > can simply remove the `if` condition. Good catch. https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:838: * URL. e.g. if the value of the <code>rewrite</code> property is On 2019/04/16 05:52:58, Manish Jethani wrote: > I think we can replace `<code>rewrite</code> property` with > `<code>$rewrite</code> option` here because otherwise it would be wrong. Done. https://codereview.adblockplus.org/30045566/diff/30045572/lib/filterClasses.j... lib/filterClasses.js:1047: if (!value || !value.startsWith("abp-resource:")) On 2019/04/16 05:52:58, Manish Jethani wrote: > It doesn't matter _that_ much, but I'd just like to point out that this is an > additional change in behavior. A filter like `foo$rewrite` would given an > "unknown option" error previously, while now it would give a "invalid rewrite" > error. Maybe that's fine. > > If we want to stick with the old behavior, then we can leave the check for > `value == null` as it is and check for `!value.startsWith(...)` as an additional > check. I think I'm gonna go the safe route. Change was intentional but maybe that's not a good idea for this fix right now.
What branch/bookmark is this based on?
On 2019/04/16 12:26:10, Manish Jethani wrote: > What branch/bookmark is this based on? I see, this is based on master. LGTM PS: Please land this directly in master.
On 2019/04/16 12:28:19, Manish Jethani wrote: > On 2019/04/16 12:26:10, Manish Jethani wrote: > > What branch/bookmark is this based on? > > I see, this is based on master. > > LGTM > > PS: Please land this directly in master. To be clear, we will make this change (either by grafting or a separate patch) in the next branch also, but first let's get it into master. 
|
