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

Issue 29336787: Issue 3670 - Make rules case sensitive where possible (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 9 months ago by kzar
Modified:
3 years, 9 months ago
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 3670 - Make rules case sensitive where possible (Depends on this review: https://codereview.adblockplus.org/29336753/ )

Patch Set 1 #

Total comments: 8

Patch Set 2 : Rebased and addressed feedback #

Patch Set 3 : Fix edge case for filters ending with "://" #

Total comments: 4

Patch Set 4 : Keep rules with non alpha characters after hostname ends case sensitive #

Patch Set 5 : Handle "/" separately and the fall through #

Total comments: 8

Patch Set 6 : Fixed mistake with logic for * and ^, addressed other feedback #

Total comments: 9

Patch Set 7 : Addressed more feedback #

Total comments: 2

Patch Set 8 : Improved documentation of toRegexp function and switched to JSDoc syntax #

Unified diffs Side-by-side diffs Delta from patch set Stats (+53 lines, -12 lines) Patch
M lib/contentBlockerList.js View 1 2 3 4 5 6 7 7 chunks +53 lines, -12 lines 0 comments Download

Messages

Total messages: 14
kzar
Patch Set 1
3 years, 9 months ago (2016-02-21 13:49:42 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29336788/lib/contentBlockerLists.js File lib/contentBlockerLists.js (right): https://codereview.adblockplus.org/29336787/diff/29336788/lib/contentBlockerLists.js#newcode175 lib/contentBlockerLists.js:175: function caseSensitive(filter) It probably makes sense to integrate the ...
3 years, 9 months ago (2016-02-21 21:42:30 UTC) #2
kzar
Patch Set 2 : Rebased and addressed feedback https://codereview.adblockplus.org/29336787/diff/29336788/lib/contentBlockerLists.js File lib/contentBlockerLists.js (right): https://codereview.adblockplus.org/29336787/diff/29336788/lib/contentBlockerLists.js#newcode175 lib/contentBlockerLists.js:175: function ...
3 years, 9 months ago (2016-02-24 15:43:29 UTC) #3
kzar
Patch Set 3 : Fix edge case for filters ending with "://"
3 years, 9 months ago (2016-02-24 15:53:34 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29337682/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337682/lib/contentBlockerList.js#newcode112 lib/contentBlockerList.js:112: if (containsHostname && i < lastIndex) I think caseSensitive ...
3 years, 9 months ago (2016-02-24 20:31:05 UTC) #5
kzar
Patch Set 4 : Keep rules with non alpha characters after hostname ends case sensitive ...
3 years, 9 months ago (2016-02-24 21:20:48 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29337694/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337694/lib/contentBlockerList.js#newcode113 lib/contentBlockerList.js:113: if (!hostnameStarted && i >= 2 && text[i-2] == ...
3 years, 9 months ago (2016-02-24 21:47:07 UTC) #7
kzar
Patch Set 6 : Fixed mistake with logic for * and ^, addressed other feedback ...
3 years, 9 months ago (2016-02-24 22:19:46 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js#newcode111 lib/contentBlockerList.js:111: hostnameStarted = caseSensitive = true; Nit: Sometimes you have ...
3 years, 9 months ago (2016-02-24 22:53:49 UTC) #9
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js#newcode161 lib/contentBlockerList.js:161: if (result.caseSensitive) On 2016/02/24 22:53:49, Sebastian Noack wrote: > ...
3 years, 9 months ago (2016-02-24 23:07:37 UTC) #10
kzar
Patch Set 7 : Addressed more feedback https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337696/lib/contentBlockerList.js#newcode111 lib/contentBlockerList.js:111: hostnameStarted = ...
3 years, 9 months ago (2016-02-24 23:15:21 UTC) #11
Sebastian Noack
LGTM!
3 years, 9 months ago (2016-02-24 23:18:47 UTC) #12
Sebastian Noack
https://codereview.adblockplus.org/29336787/diff/29337698/lib/contentBlockerList.js File lib/contentBlockerList.js (right): https://codereview.adblockplus.org/29336787/diff/29337698/lib/contentBlockerList.js#newcode68 lib/contentBlockerList.js:68: // Convert the "regexpSource" part of a filter's text ...
3 years, 9 months ago (2016-02-25 02:28:36 UTC) #13
kzar
3 years, 9 months ago (2016-02-25 15:22:29 UTC) #14
Patch Set 8 : Improved documentation of toRegexp function and switched to JSDoc
syntax

https://codereview.adblockplus.org/29336787/diff/29337698/lib/contentBlockerL...
File lib/contentBlockerList.js (right):

https://codereview.adblockplus.org/29336787/diff/29337698/lib/contentBlockerL...
lib/contentBlockerList.js:68: // Convert the "regexpSource" part of a filter's
text to a regular expression,
On 2016/02/25 02:28:35, Sebastian Noack wrote:
> Sorry for commenting after LGTM. Just one more nit: While I don't consider
> documentation for internal APIs mandatory, if you comment private functions,
> please still use JSDoc syntax.

Done.
Sign in to reply to this message.

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