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

Issue 5698450620416000: Replaced the Disposer (Closed)

Created:
April 30, 2014, 11:17 a.m. by René Jeschke
Modified:
May 2, 2014, 8:58 a.m.
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
April 30, 2014, 11:18 a.m. (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 ...
April 30, 2014, 3:34 p.m. (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: ...
April 30, 2014, 3:43 p.m. (2014-04-30 15:43:15 UTC) #3
Felix Dahlke
April 30, 2014, 3:50 p.m. (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.

Powered by Google App Engine
This is Rietveld