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

Issue 5698450620416000: Replaced the Disposer (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 2 months ago by René Jeschke
Modified:
5 years, 2 months ago
Reviewers:
Felix Dahlke
CC:
Wladimir Palant
Visibility:
Public.

Description

Replaced the Disposer

Patch Set 1 #

Total comments: 11

Patch Set 2 : Review issues fixed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -319 lines) Patch
D src/com/github/rjeschke/neetutils/dispose/Disposable.java View 1 chunk +0 lines, -26 lines 0 comments Download
D src/com/github/rjeschke/neetutils/dispose/Disposer.java View 1 chunk +0 lines, -99 lines 0 comments Download
D src/com/github/rjeschke/neetutils/dispose/ReferenceList.java View 1 chunk +0 lines, -162 lines 0 comments Download
A + src/org/adblockplus/libadblockplus/Disposable.java View 1 chunk +3 lines, -8 lines 0 comments Download
A src/org/adblockplus/libadblockplus/Disposer.java View 1 1 chunk +94 lines, -0 lines 0 comments Download
M src/org/adblockplus/libadblockplus/EventCallback.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/FilterChangeCallback.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/FilterEngine.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/JsEngine.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/JsValue.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/LogSystem.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/UpdaterCallback.java View 1 chunk +0 lines, -3 lines 0 comments Download
M src/org/adblockplus/libadblockplus/WebRequest.java View 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 4
René Jeschke
5 years, 2 months ago (2014-04-30 11:18:16 UTC) #1
Felix Dahlke
Looks good, only some understanding/readability comments. http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/org/adblockplus/libadblockplus/Disposable.java File src/org/adblockplus/libadblockplus/Disposable.java (right): http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/org/adblockplus/libadblockplus/Disposable.java#newcode18 src/org/adblockplus/libadblockplus/Disposable.java:18: package org.adblockplus.libadblockplus; What's ...
5 years, 2 months ago (2014-04-30 15:34:08 UTC) #2
René Jeschke
http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/org/adblockplus/libadblockplus/Disposable.java File src/org/adblockplus/libadblockplus/Disposable.java (right): http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/org/adblockplus/libadblockplus/Disposable.java#newcode18 src/org/adblockplus/libadblockplus/Disposable.java:18: package org.adblockplus.libadblockplus; On 2014/04/30 15:34:08, Felix H. Dahlke wrote: ...
5 years, 2 months ago (2014-04-30 15:43:15 UTC) #3
Felix Dahlke
5 years, 2 months ago (2014-04-30 15:50:43 UTC) #4
LGTM

http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/...
File src/org/adblockplus/libadblockplus/Disposer.java (right):

http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/...
src/org/adblockplus/libadblockplus/Disposer.java:39: public Disposer(final
Disposable referent, final Disposable disposable)
On 2014/04/30 15:43:16, René Jeschke wrote:
> On 2014/04/30 15:34:08, Felix H. Dahlke wrote:
> > How about renaming "disposable" to "reference"? That'd be a bit more clear
> IMO.
> 
> 'referent' is the reference that get stored in the ReferenceQueue to track
when
> 'referent' is only weakly reachable.
> 
> 'disposable' is the object that actually does the disposing.
> 
> So, IMO: both names are correct.

Isn't disposable the object that _gets_ disposed once the referent is GCed?
Anyway, yeah, "reference" is probably too ambiguous here. We could call
"referent" "reference" and it'd still make sense.
Sign in to reply to this message.

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