Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 30007559: Issue 7285 - Abstract caching logic (Closed)

Created:
Feb. 14, 2019, 5:24 a.m. by Manish Jethani
Modified:
Feb. 18, 2019, 1:40 p.m.
Reviewers:
hub
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Add tests and change API and behavior slightly #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+201 lines, -27 lines) Patch
A lib/caching.js View 1 1 chunk +82 lines, -0 lines 2 comments Download
M lib/elemHide.js View 1 3 chunks +3 lines, -12 lines 0 comments Download
M lib/matcher.js View 1 4 chunks +4 lines, -15 lines 0 comments Download
A test/caching.js View 1 1 chunk +112 lines, -0 lines 0 comments Download

Messages

Total messages: 5
Manish Jethani
Feb. 14, 2019, 5:24 a.m. (2019-02-14 05:24:20 UTC) #1
Manish Jethani
Patch Set 1
Feb. 14, 2019, 5:37 a.m. (2019-02-14 05:37:47 UTC) #2
hub
one should add tests for that that cache logic. https://codereview.adblockplus.org/30007559/diff/30007564/lib/caching.js File lib/caching.js (right): https://codereview.adblockplus.org/30007559/diff/30007564/lib/caching.js#newcode45 lib/caching.js:45: ...
Feb. 14, 2019, 8:43 p.m. (2019-02-14 20:43:00 UTC) #3
Manish Jethani
Patch Set 2: Add tests and change API and behavior slightly https://codereview.adblockplus.org/30007559/diff/30007564/lib/caching.js File lib/caching.js (right): ...
Feb. 16, 2019, 12:17 p.m. (2019-02-16 12:17:17 UTC) #4
hub
Feb. 18, 2019, 1:36 p.m. (2019-02-18 13:36:51 UTC) #5
LGTM

https://codereview.adblockplus.org/30007559/diff/30009555/lib/caching.js
File lib/caching.js (right):

https://codereview.adblockplus.org/30007559/diff/30009555/lib/caching.js#newc...
lib/caching.js:67: if (this._storage.size == this._capacity &&
!this._storage.has(key))
On 2019/02/16 12:17:17, Manish Jethani wrote:
> This is slightly less efficient but more correct (i.e. an existing entry
should
> not allow the cache to get cleared). In practice it would not matter, but
since
> we are writing tests and making the object reusable in other places, it should
> have good semantics.

Acknowledged.

Powered by Google App Engine
This is Rietveld