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

Issue 5150730151264256: Issue 354 - Avoid a string copy that is unnecessary most of the time (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 1 month ago by tschuster
Modified:
4 years ago
Visibility:
Public.

Description

Avoid a string copy that is unnecessary most of the time

Patch Set 1 #

Total comments: 1

Patch Set 2 : Avoid toUpperCase for RegexpFilter #

Total comments: 1

Patch Set 3 : Added domainSourceIsUpperCase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -3 lines) Patch
M lib/filterClasses.js View 1 2 4 chunks +20 lines, -3 lines 0 comments Download

Messages

Total messages: 13
tschuster
4 years, 1 month ago (2014-04-04 15:13:08 UTC) #1
Wladimir Palant
Unnecessary or not - without this code the isActiveOnDomain() and isActiveOnlyOnDomain() methods are broken.
4 years, 1 month ago (2014-04-08 11:28:18 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/filterClasses.js File lib/filterClasses.js (right): http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/filterClasses.js#newcode315 lib/filterClasses.js:315: let list = this.domainSource.toUpperCase().split(this.domainSeparator); I see, I didn't realize ...
4 years, 1 month ago (2014-04-08 12:10:05 UTC) #3
tschuster
On 2014/04/08 12:10:05, Wladimir Palant wrote: > http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/filterClasses.js > File lib/filterClasses.js (right): > > http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/filterClasses.js#newcode315 ...
4 years, 1 month ago (2014-04-08 12:40:13 UTC) #4
tschuster
4 years, 1 month ago (2014-04-10 17:41:32 UTC) #5
Wladimir Palant
http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/filterClasses.js File lib/filterClasses.js (right): http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/filterClasses.js#newcode316 lib/filterClasses.js:316: if (!(this instanceof RegExpFilter)) { I'd rather not hardcode ...
4 years, 1 month ago (2014-04-10 18:43:38 UTC) #6
tschuster
On 2014/04/10 18:43:38, Wladimir Palant wrote: > http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/filterClasses.js > File lib/filterClasses.js (right): > > http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/filterClasses.js#newcode316 ...
4 years, 1 month ago (2014-04-10 19:19:53 UTC) #7
Wladimir Palant
On 2014/04/10 19:19:53, tschuster wrote: > Adding a new property is a bad idea, because ...
4 years, 1 month ago (2014-04-11 05:40:16 UTC) #8
tschuster
On 2014/04/11 05:40:16, Wladimir Palant wrote: > On 2014/04/10 19:19:53, tschuster wrote: > > Adding ...
4 years, 1 month ago (2014-04-11 10:15:09 UTC) #9
Wladimir Palant
On 2014/04/11 10:15:09, tschuster wrote: > Nope. What do you think of |isDomainAlreadyUpperCase: function() { ...
4 years, 1 month ago (2014-04-12 07:50:19 UTC) #10
tschuster
Added domainSourceIsUpperCase
4 years, 1 month ago (2014-04-16 16:11:47 UTC) #11
tschuster
Sorry for being unclear. Properties just on the prototype is of course not a problem. ...
4 years, 1 month ago (2014-04-16 16:14:27 UTC) #12
Wladimir Palant
4 years, 1 month ago (2014-04-16 17:13:36 UTC) #13
LGTM
Sign in to reply to this message.

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