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

Issue 29907586: Issue 6994 - Use shortcut matching for location only filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 1 month ago by Jon Sonesen
Modified:
1 year, 1 month ago
Reviewers:
Manish Jethani
Visibility:
Public.

Description

Issue 6994 - Use shortcut matching for location only filters

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address PS1 Comments #

Total comments: 2

Patch Set 3 : #

Patch Set 4 : Reduce ternary use #

Total comments: 3

Patch Set 5 : Address PS4, and rebase #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -7 lines) Patch
M lib/filterClasses.js View 1 2 3 4 1 chunk +10 lines, -0 lines 2 comments Download
M lib/matcher.js View 1 2 3 4 3 chunks +39 lines, -7 lines 8 comments Download
M test/filterListener.js View 1 2 3 4 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 18
Jon Sonesen
1 year, 1 month ago (2018-10-12 04:56:14 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29907586/diff/29907587/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29907586/diff/29907587/lib/matcher.js#newcode175 lib/matcher.js:175: for (let filter of set) The idea was to ...
1 year, 1 month ago (2018-10-13 13:28:46 UTC) #2
Jon Sonesen
https://codereview.adblockplus.org/29907586/diff/29907587/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29907586/diff/29907587/lib/matcher.js#newcode175 lib/matcher.js:175: for (let filter of set) On 2018/10/13 13:28:45, Manish ...
1 year, 1 month ago (2018-10-20 23:07:56 UTC) #3
Jon Sonesen
https://codereview.adblockplus.org/29907586/diff/29917555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29907586/diff/29917555/lib/filterClasses.js#newcode803 lib/filterClasses.js:803: return RegExp(location).test(this.regexp); My bad, thought I added doc strings ...
1 year, 1 month ago (2018-10-20 23:09:48 UTC) #4
Jon Sonesen
Actually disregard PS2 please pretty much off on that one
1 year, 1 month ago (2018-10-21 00:00:57 UTC) #5
Jon Sonesen
This is not quite there, the findKeyword test is failing
1 year, 1 month ago (2018-10-21 04:27:37 UTC) #6
Jon Sonesen
This is not quite there, the findKeyword test is failing
1 year, 1 month ago (2018-10-21 04:27:39 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29907586/diff/29917628/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29907586/diff/29917628/lib/filterClasses.js#newcode778 lib/filterClasses.js:778: get isLocationOnly() Let's make this a function rather than ...
1 year, 1 month ago (2018-10-21 16:06:53 UTC) #8
Jon Sonesen
https://codereview.adblockplus.org/29907586/diff/29917628/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29907586/diff/29917628/lib/matcher.js#newcode154 lib/matcher.js:154: this.filterByKeyword; In the tests this fails on the last ...
1 year, 1 month ago (2018-10-21 17:29:51 UTC) #9
Manish Jethani
Let's come back to this once we've landed the other two patches in this area.
1 year, 1 month ago (2018-10-23 10:50:54 UTC) #10
Jon Sonesen
Ack
1 year, 1 month ago (2018-10-23 17:34:28 UTC) #11
Jon Sonesen
Ack https://codereview.adblockplus.org/29907586/diff/29917628/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29907586/diff/29917628/lib/filterClasses.js#newcode778 lib/filterClasses.js:778: get isLocationOnly() On 2018/10/21 16:06:52, Manish Jethani wrote: ...
1 year, 1 month ago (2018-10-23 17:36:32 UTC) #12
Manish Jethani
This may need a rebase now.
1 year, 1 month ago (2018-10-24 19:24:35 UTC) #13
Jon Sonesen
Yeah it does, I am doing that now and cleaning the patch up some more ...
1 year, 1 month ago (2018-10-24 19:31:56 UTC) #14
Manish Jethani
https://codereview.adblockplus.org/29907586/diff/29923588/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29907586/diff/29923588/lib/filterClasses.js#newcode816 lib/filterClasses.js:816: return this.contentType == RegExpFilter.prototype.contentType && I haven't run this ...
1 year, 1 month ago (2018-10-24 21:35:28 UTC) #15
Jon Sonesen
https://codereview.adblockplus.org/29907586/diff/29923588/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29907586/diff/29923588/lib/filterClasses.js#newcode816 lib/filterClasses.js:816: return this.contentType == RegExpFilter.prototype.contentType && On 2018/10/24 21:35:28, Manish ...
1 year, 1 month ago (2018-10-24 21:46:52 UTC) #16
Manish Jethani
https://codereview.adblockplus.org/29907586/diff/29923588/lib/matcher.js File lib/matcher.js (right): https://codereview.adblockplus.org/29907586/diff/29923588/lib/matcher.js#newcode174 lib/matcher.js:174: let count = typeof filters != "undefined" ? filters.size ...
1 year, 1 month ago (2018-10-25 00:49:42 UTC) #17
Jon Sonesen
1 year, 1 month ago (2018-10-25 21:45:43 UTC) #18
Closing this.
Sign in to reply to this message.

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