Issue 5698450620416000: Replaced the Disposer

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.

Patch Set 2 : Review issues fixed #

Created: April 30, 2014, 3:44 p.m.

Download [raw] [tar.bz2]

	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

Expand All Messages | Collapse All Messages

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

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

http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/...
src/org/adblockplus/libadblockplus/Disposable.java:18: package
org.adblockplus.libadblockplus;
On 2014/04/30 15:34:08, Felix H. Dahlke wrote:
> What's with this diff, why does it replace the Subscription class?

Because something's wrong with the diff, I'm not replacing the Subscription
class as it still exists. 

So: just abogus diff.

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:27: final static
HashSet<Disposer> disposerSet = new HashSet<Disposer>();
On 2014/04/30 15:34:08, Felix H. Dahlke wrote:
> Should be "static final".

Done.

http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/...
src/org/adblockplus/libadblockplus/Disposer.java:29: private volatile boolean
isDisposed = false;
On 2014/04/30 15:34:08, Felix H. Dahlke wrote:
> Should be "disposed" according to the bean convention IMO. "isDisposed" would
be
> the name of the getter.

Done.

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: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.

http://codereview.adblockplus.org/5698450620416000/diff/5629499534213120/src/...
src/org/adblockplus/libadblockplus/Disposer.java:41: super(referent,
referenceQueue);
On 2014/04/30 15:34:08, Felix H. Dahlke wrote:
> Shouldn't it be the referentQueue if it stores the referents?

It stores the value from a local variable called 'referent', yep, but it stores
and manages 'WeakReference's, that's why it's a 'ReferenceQueue' and that's also
why the member is called 'referenceQueue'.

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.

Expand All Messages | Collapse All Messages