Wiring up new wave panel (undercurrent) to client
zdw...@google.com
Please review this at http://codereview.waveprotocol.org/180001
Affected files:
M src/org/waveprotocol/wave/examples/client/webclient/WebClient.gwt.xml
A
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStageTwo.java
A
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStages.java
M src/org/waveprotocol/wave/examples/client/webclient/client/Session.java
M
src/org/waveprotocol/wave/examples/client/webclient/client/WaveView.ui.xml
M
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.java
M
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.ui.xml
M src/org/waveprotocol/wave/examples/common/SessionConstants.java
M src/org/waveprotocol/wave/examples/fedone/rpc/WaveClientServlet.java
ljvd...@google.com
As discussed can we get a bit more explanation on what the stages are?
Seems difficult for an outsider to understand.
David Wang
On 2010/10/06 01:32:50, zdwang wrote:
As discussed can we get a bit more explanation on what the stages are?
Seems difficult for an outsider to understand.
--
David Wang
hear...@google.com
http://codereview.waveprotocol.org/180001/diff/1/2
File
src/org/waveprotocol/wave/examples/client/webclient/WebClient.gwt.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/2#newcode19
Line 19: <inherits
name='org.waveprotocol.wave.model.conversation.Testing'/>
Please add a comment that this dependency is temporary (it is for
creating fake waves), until the fedone client gets it own harness.
http://codereview.waveprotocol.org/180001/diff/1/3
File
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStageTwo.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/3#newcode45
Line 45: public class FedOneStageTwo extends StageTwo.DefaultProvider {
I'd avoid using "FedOne" in the name, because I don't think the name is
used frequently anywhere else in the code, and it is likely to change.
It's more evolution-friendly to name things according to what they do,
rather than where they are intended to be used.
Walkabaout-Undercurrent just names them StageOneProvider,
StageTwoProvider, etc. StageOne and StageTwo would also be good names
(but StageXProvider avoids any name clash). The package they sit in
already tells you the client for which they provide stages.
Also, this probably belongs in a .testing subpackage, since it uses fake
waves and depends on conversation.testing. Another option is to leave
these here, but make them abstract, leaving the fetchWave method
unimplemented. A .testing subpackage can bind the fetchWave with a fake
wave creator.
I guess it depends on how much we care about keeping the core client (on
the public trunk) free of testing/harness concerns. Since it's not
going to last long, I'm not sure I care too much.
http://codereview.waveprotocol.org/180001/diff/1/3#newcode47
Line 47: static class ProfileImpl implements Profile {
Perhaps a note that these classes are placeholders for proper profile
management.
http://codereview.waveprotocol.org/180001/diff/1/4
File
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStages.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/4#newcode21
Line 21: private final Element initialHtml;
While here, please avoid the awful name "initialHtml". "Initial"
implies later change, and "HTML" is a textual/string language. The
object, however, is permanent, and not a string, so neither part of that
name is accurate.
Something like "panelElement", "e", "element", "wave", "waveElement",
"dom", ... would be far more accurate.
http://codereview.waveprotocol.org/180001/diff/1/4#newcode25
Line 25: * @param initialHtml The dom element to attach stage 1 (wave
panel)
s/attach stage 1 (wave panel)/become the wave panel/
"attach" could be misinterpreted as something to which an independent
wave panel DOM gets appended.
http://codereview.waveprotocol.org/180001/diff/1/4#newcode27
Line 27: * is used for attaching to the GWT widget tree.
s/attaching to/adopting into/
It's important to avoid possible misinterpretation that there might be a
physical effect to the DOM.
http://codereview.waveprotocol.org/180001/diff/1/6
File
src/org/waveprotocol/wave/examples/client/webclient/client/WaveView.ui.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/6#newcode30
Line 30: left: 0px;
css style is to have no units on 0s, since 0 is 0 in any units.
http://codereview.waveprotocol.org/180001/diff/1/7
File
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/7#newcode93
Line 93: private final FlowPanel wavePanelHolder = null;
final and null?
http://codereview.waveprotocol.org/180001/diff/1/8
File
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.ui.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/8#newcode12
Line 12: height:100%;
is this required? percentage heights cause many issues.
http://codereview.waveprotocol.org/180001/diff/1/8#newcode15
Line 15: .root2 {
unused
http://codereview.waveprotocol.org/180001/diff/1/8#newcode81
Line 81: <w:ImplPanel ui:field="contentPanel"></w:ImplPanel>
This can be self-closing.
http://codereview.waveprotocol.org/180001/diff/1/9
File src/org/waveprotocol/wave/examples/common/SessionConstants.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/9#newcode40
Line 40: public final static String ID = "id";
Perhaps it would be better to call this "ID_SEED"? Isn't it only used
to give the client a globally unique ID that it can use for id
generation?
Calling it "session id" may get it confused with the http session id or
auth cookie, and the two values have nothing to do with eachother,
right?
If this value gets used for anything other than seeding the client's
generation of globally unique ids, and there is some reason to call it a
session id, then please comment that it has nothing to do with the HTTP
session id or authorization (unless I've misunderstood how we use those
ids). If it does, then the server needs to generate something that is
cryptographically strong, and has some kind of effort to be globally
unique.
http://codereview.waveprotocol.org/180001/diff/1/10
File
src/org/waveprotocol/wave/examples/fedone/rpc/WaveClientServlet.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/10#newcode142
Line 142: // TODO(zdwang): Figure out a proper session id rather than
generating a random number
proper = cryptographically strong?
see comment in SessionConstants.
hear...@google.com
> On 2010/10/06 01:32:50, zdwang wrote:
> >
> As discussed can we get a bit more explanation on what the stages are?
Seems
> difficult for an outsider to understand.
I agree that if an outsider wants a full appreciation of the client
architecture, then the current code and comments are probably
insufficient. Publishing the design doc would be better than adding
code comments though - the latter a bit too low level.
Also, the documentation issue really applies to the contents of
waveprotocol-libraries (waveprotocol/wave/client), rather than fedone.
The simple-web-client is just taking the staged architecture that is
already in the libraries, and binding the appropriate parts of the
stages it to the simple-web-client environment.
I'll add publishing the design doc to the roadmap.
zdw...@google.com
http://codereview.waveprotocol.org/180001/diff/1/2
File
src/org/waveprotocol/wave/examples/client/webclient/WebClient.gwt.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/2#newcode19
Line 19: <inherits
name='org.waveprotocol.wave.model.conversation.Testing'/>
On 2010/10/06 06:17:28, hearnden wrote:
> Please add a comment that this dependency is temporary (it is for
creating fake
> waves), until the fedone client gets it own harness.
Done.
http://codereview.waveprotocol.org/180001/diff/1/3
File
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStageTwo.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/3#newcode45
Line 45: public class FedOneStageTwo extends StageTwo.DefaultProvider {
I think the testing code here will be short lived, prob days, so I'll
leave it here for now.
Renamed the file.
http://codereview.waveprotocol.org/180001/diff/1/3#newcode47
Line 47: static class ProfileImpl implements Profile {
On 2010/10/06 06:17:28, hearnden wrote:
> Perhaps a note that these classes are placeholders for proper profile
> management.
Done.
http://codereview.waveprotocol.org/180001/diff/1/4
File
src/org/waveprotocol/wave/examples/client/webclient/client/FedOneStages.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/4#newcode21
Line 21: private final Element initialHtml;
On 2010/10/06 06:17:28, hearnden wrote:
> While here, please avoid the awful name "initialHtml". "Initial"
implies later
> change, and "HTML" is a textual/string language. The object, however,
is
> permanent, and not a string, so neither part of that name is accurate.
> Something like "panelElement", "e", "element", "wave", "waveElement",
"dom", ...
> would be far more accurate.
Done.
http://codereview.waveprotocol.org/180001/diff/1/4#newcode25
Line 25: * @param initialHtml The dom element to attach stage 1 (wave
panel)
On 2010/10/06 06:17:28, hearnden wrote:
> s/attach stage 1 (wave panel)/become the wave panel/
> "attach" could be misinterpreted as something to which an independent
wave panel
> DOM gets appended.
Done.
http://codereview.waveprotocol.org/180001/diff/1/4#newcode27
Line 27: * is used for attaching to the GWT widget tree.
On 2010/10/06 06:17:28, hearnden wrote:
> s/attaching to/adopting into/
> It's important to avoid possible misinterpretation that there might be
a
> physical effect to the DOM.
Done.
http://codereview.waveprotocol.org/180001/diff/1/6
File
src/org/waveprotocol/wave/examples/client/webclient/client/WaveView.ui.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/6#newcode30
Line 30: left: 0px;
On 2010/10/06 06:17:28, hearnden wrote:
> css style is to have no units on 0s, since 0 is 0 in any units.
Done.
http://codereview.waveprotocol.org/180001/diff/1/7
File
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/7#newcode93
Line 93: private final FlowPanel wavePanelHolder = null;
On 2010/10/06 06:17:28, hearnden wrote:
> final and null?
Removed.
http://codereview.waveprotocol.org/180001/diff/1/8
File
src/org/waveprotocol/wave/examples/client/webclient/client/WebClient.ui.xml
(right):
http://codereview.waveprotocol.org/180001/diff/1/8#newcode12
Line 12: height:100%;
On 2010/10/06 06:17:28, hearnden wrote:
> is this required? percentage heights cause many issues.
Removed. These were just for testing a while back.
http://codereview.waveprotocol.org/180001/diff/1/8#newcode15
Line 15: .root2 {
On 2010/10/06 06:17:28, hearnden wrote:
> unused
Removed
http://codereview.waveprotocol.org/180001/diff/1/8#newcode81
Line 81: <w:ImplPanel ui:field="contentPanel"></w:ImplPanel>
On 2010/10/06 06:17:28, hearnden wrote:
> This can be self-closing.
Done.
http://codereview.waveprotocol.org/180001/diff/1/9
File src/org/waveprotocol/wave/examples/common/SessionConstants.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/9#newcode40
Line 40: public final static String ID = "id";
On 2010/10/06 06:17:28, hearnden wrote:
> Perhaps it would be better to call this "ID_SEED"? Isn't it only used
to give
> the client a globally unique ID that it can use for id generation?
> Calling it "session id" may get it confused with the http session id
or auth
> cookie, and the two values have nothing to do with eachother, right?
> If this value gets used for anything other than seeding the client's
generation
> of globally unique ids, and there is some reason to call it a session
id, then
> please comment that it has nothing to do with the HTTP session id or
> authorization (unless I've misunderstood how we use those ids). If it
does,
> then the server needs to generate something that is cryptographically
strong,
> and has some kind of effort to be globally unique.
Done.
http://codereview.waveprotocol.org/180001/diff/1/10
File
src/org/waveprotocol/wave/examples/fedone/rpc/WaveClientServlet.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/10#newcode142
Line 142: // TODO(zdwang): Figure out a proper session id rather than
generating a random number
On 2010/10/06 06:17:28, hearnden wrote:
> proper = cryptographically strong?
> see comment in SessionConstants.
It's already cryptographically strong, but we might completely replace
the seed in the future... it's wip
zdw...@google.com
hear...@google.com
http://codereview.waveprotocol.org/180001/diff/1/9
File src/org/waveprotocol/wave/examples/common/SessionConstants.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/9#newcode40
Line 40: public final static String ID = "id";
On 2010/10/06 06:51:19, zdwang wrote:
> On 2010/10/06 06:17:28, hearnden wrote:
> > Perhaps it would be better to call this "ID_SEED"? Isn't it only
used to give
> > the client a globally unique ID that it can use for id generation?
> >
> > Calling it "session id" may get it confused with the http session id
or auth
> > cookie, and the two values have nothing to do with eachother, right?
> >
> > If this value gets used for anything other than seeding the client's
> generation
> > of globally unique ids, and there is some reason to call it a
session id, then
> > please comment that it has nothing to do with the HTTP session id or
> > authorization (unless I've misunderstood how we use those ids). If
it does,
> > then the server needs to generate something that is
cryptographically strong,
> > and has some kind of effort to be globally unique.
> Done.
I don't see any change. Do you need to upload a patch again?
hear...@google.com
http://codereview.waveprotocol.org/180001/diff/1/9
File src/org/waveprotocol/wave/examples/common/SessionConstants.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/9#newcode40
Line 40: public final static String ID = "id";
Never mind - I see it now. Please update the javadoc, and say that this
value is just something that is globally unique that the client can use
to seed unique ids. It has no relationship with the application
session, http session, or authentication, and is not guaranteed to be
cryptographically strong.
hear...@google.com
zdw...@google.com
http://codereview.waveprotocol.org/180001/diff/1/9
File src/org/waveprotocol/wave/examples/common/SessionConstants.java
(right):
http://codereview.waveprotocol.org/180001/diff/1/9#newcode40
Line 40: public final static String ID = "id";
Done.