Encapsulates users and subscriptions management into WaveletInfo.

[veg...@gmail.com]

Reviewers: Soren Lassen,

Description:
Extracts the logic of users and subscriptions management from
ClientFrontendImpl.

Please review this at http://codereview.waveprotocol.org/614002

Affected files:
M src/org/waveprotocol/box/server/ServerMain.java
M src/org/waveprotocol/box/server/frontend/ClientFrontendImpl.java
A src/org/waveprotocol/box/server/frontend/WaveletInfo.java
M src/org/waveprotocol/wave/client/testing/UndercurrentHarness.java
M test/org/waveprotocol/box/server/frontend/ClientFrontendImplTest.java

[soren...@gmail.com]

http://codereview.waveprotocol.org/614002/diff/1006/3003
File src/org/waveprotocol/box/server/frontend/WaveletInfo.java (right):

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode57
Line 57: this.explicitParticipants =
Collections.synchronizedSet(Sets.<ParticipantId> newHashSet());
why synchronized if you all accesses are controlled by the waveletInfo
lock?

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode195
Line 195: remainingParticipants =
Collections.unmodifiableSet(waveletInfo.explicitParticipants);
I think you need to make a copy here, it's not enough with an
unmodifiable wrapper (because if the underlying set changes then you
cannot access it safely from another thread, even via the
unmodifiableSet wrapper)

same in line 211 below

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode222
Line 222: public void notifyAddedExplicitWaveletParticipant(WaveletName
waveletName, ParticipantId participant) {
line too long (same in a few other places in this file)

http://codereview.waveprotocol.org/614002

[veg...@gmail.com]

http://codereview.waveprotocol.org/614002/diff/1006/3003
File src/org/waveprotocol/box/server/frontend/WaveletInfo.java (right):

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode57
Line 57: this.explicitParticipants =
Collections.synchronizedSet(Sets.<ParticipantId> newHashSet());

On 2011/09/01 23:43:54, Soren Lassen wrote:
> why synchronized if you all accesses are controlled by the waveletInfo
lock?

Done.

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode195
Line 195: remainingParticipants =
Collections.unmodifiableSet(waveletInfo.explicitParticipants);

On 2011/09/01 23:43:54, Soren Lassen wrote:
> I think you need to make a copy here, it's not enough with an
unmodifiable
> wrapper (because if the underlying set changes then you cannot access
it safely
> from another thread, even via the unmodifiableSet wrapper)

> same in line 211 below

Done.

http://codereview.waveprotocol.org/614002/diff/1006/3003#newcode222
Line 222: public void notifyAddedExplicitWaveletParticipant(WaveletName
waveletName, ParticipantId participant) {

On 2011/09/01 23:43:54, Soren Lassen wrote:
> line too long (same in a few other places in this file)

Formatted the whole file.

http://codereview.waveprotocol.org/614002

[soren...@gmail.com]

LGTM

http://codereview.waveprotocol.org/614002/diff/5001/6003
File src/org/waveprotocol/box/server/frontend/WaveletInfo.java (right):

http://codereview.waveprotocol.org/614002/diff/5001/6003#newcode58
Line 58: this.explicitParticipants = Sets.<ParticipantId> newHashSet();
I believe <ParticipantId> is redundant.

http://codereview.waveprotocol.org/614002/diff/5001/6003#newcode90
Line 90: final MapMaker mapMaker = new MapMaker();
normally, you just create a new MapMaker instance whenever you create a
computing map

new MapMaker().makeComputingMap(....);

I don't think there are advantages to sharing the MapMaker instance but,
on the other hand, what you have should work

http://codereview.waveprotocol.org/614002/diff/5001/6003#newcode183
Line 183: version = waveletInfo.getCurrentVersion();
FWIW, it will be fewer lines of code to just say

synchronize (waveletInfo) {
return waveletInfo.getCurrentVersion();
}

same in lines 197, 214 below, but what you have works fine, too

http://codereview.waveprotocol.org/614002/diff/5001/6003#newcode197
Line 197:
Collections.unmodifiableSet(Sets.newHashSet(waveletInfo.explicitParticipants));
it will be more direct to use ImmutableSet.copyOf

same in line 214 below

http://codereview.waveprotocol.org/614002