Skip to content
This repository has been archived by the owner. It is now read-only.

Added option to preserve original filter text #98

Merged
merged 1 commit into from Nov 21, 2018

Conversation

@pes10k
Copy link
Collaborator

pes10k commented Apr 15, 2018

added a bool flag to AdBlockClient.prototype.parse to control whether this data should be preserved (since otherwise the text of ~ the entire filter list would be preserved in the uncommon case of wanting to use findMatchingFilters).

If true, then the return value of findMatchingFilters will have matchingOrigRule and matchingExceptionOrigRule properties that have the original filter rule text. If false, then the matchingOrigRule and matchingExceptionOrigRule properties are undefined

@@ -294,6 +304,13 @@ void parseFilter(const char *input, const char *end, Filter *f,
f->data = new char[len];
f->data[len - 1] = '\0';
memcpy(f->data, input + i + 1, len - 1);

if (preserveRules == true) {

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

if (preserveRules) {

filter.cc Outdated
@@ -781,6 +798,15 @@ uint32_t Filter::serialize(char *buffer) {
}
totalSize += 1;

if (ruleDefinition) {

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

I don't think this is serializing correctly, I think we would need to store the ruleDefinitionSize in the above snprintf

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

Pls also add a test to test/js/serializationTest.js for that to ensure it works.

filter.cc Outdated
@@ -826,6 +852,14 @@ uint32_t Filter::deserialize(char *buffer, uint32_t bufferSize) {
}
consumed += hostLen + 1;

uint32_t ruleDefinitionLen = static_cast<uint32_t>(strlen(buffer + consumed));

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

Ditto I don't think this is deserializing correctly.

FilterParseState parseState = FPStart;
const char *p = input;
const char *q = p;

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

Let's call q filterRuleEndPos instead.

char data[kMaxLineLength];
memset(data, 0, sizeof data);
int i = 0;

int ruleTextLength = 0;

This comment has been minimized.

Copy link
@bbondy

bbondy May 14, 2018

Member

I think we can just subtract filterRuleEndPos from the line start pos to get the same length. So we can probably get rid of this.

@pes10k pes10k force-pushed the pes10k:preserve-original-filter-rule branch from 73a1094 to 1dcc7ce May 21, 2018
@pes10k
Copy link
Collaborator Author

pes10k commented May 21, 2018

@bbondy rebased and change to address your comments. I also added a couple new sets of tests.

One thing that came up when writing this up though: i don't see anywhere that the {Filter, CosmeticFilter}::{Serialize, Deserialize} methods are called. This ended up being very confusing, since I couldn't understand why the original filter rules we're being serialized (i missed that its actually done in serializeFilters in ad_block_client.cc).

Unless I'm missing where these methods are used, would you mind if I deleted all four methods, to avoid future possible confusion?

@@ -347,9 +365,12 @@ void parseFilter(const char *input, const char *end, Filter *f,
}
parseState = FPDataOnly;
f->filterType = FTHTMLFiltering;
p+=2;
p += 2;

This comment has been minimized.

Copy link
@bbondy

bbondy Jul 16, 2018

Member

filterRuleEndPos += 2?

if (buffer) {
snprintf(buffer + bufferSize, bufferSizeAvail, "%s", f->ruleDefinition);
}
bufferSize += static_cast<int>(strlen(f->ruleDefinition));

This comment has been minimized.

Copy link
@bbondy

bbondy Jul 16, 2018

Member

any time serialization deserialization is changed, we need to update the adblock version, so I'm ok with this but you'd have to set it to version 5.

filter.cc Outdated
size_t len = strlen(other.ruleDefinition) + 1;
ruleDefinition = new char[len];
snprintf(ruleDefinition, len, "%s", other.ruleDefinition);
ruleDefinitionLen = len - 1;

This comment has been minimized.

Copy link
@bbondy

bbondy Jul 16, 2018

Member

this is different from above, before the len included the 0 terminated character.

@pes10k pes10k force-pushed the pes10k:preserve-original-filter-rule branch 2 times, most recently from 4a238ff to 861ba9e Jul 17, 2018
…ign of adguard filters, linter corrections
@pes10k pes10k force-pushed the pes10k:preserve-original-filter-rule branch from 861ba9e to b84c6d7 Nov 21, 2018
@bbondy
bbondy approved these changes Nov 21, 2018
@bbondy bbondy merged commit dcfc584 into brave:master Nov 21, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.