Goktug Gokdogan has posted comments on this change.
Change subject: Added gwt.codeserver.port system property to
RemoteServiceServlet
......................................................................
Patch Set 2:
(13 comments)
....................................................
File
user/src/com/google/gwt/user/server/rpc/DefaultSerializationPolicyClient.java
Line 48: conn.setConnectTimeout(5000);
Please define constants for magic numbers.
Line 51:
((HttpURLConnection)conn).setInstanceFollowRedirects(false);
You may want to document why we don't want to follow redirects.
....................................................
File user/src/com/google/gwt/user/server/rpc/RemoteServiceServlet.java
Line 42: implements SerializationPolicyProvider,
DefaultSerializationPolicyClient.Logger {
DefaultSerializationPolicyClient.Logger is a contract that we don't want to
necessarily maintain for RemoteServiceServlet and it is better hidden from
the public interface.
Line 136: * will be attempted.)
You may want to add that the value is provided from "gwt.codeserver.port"
system property.
Line 175: private int getCodeServerPort(ServletConfig config) throws
ServletException {
ServletConfig parameter is unused
Line 332: * in the same ServletContext as this servlet. If no policy is
found, it then attempts to
no policy is found -> no policy is found or loading of the policy fails
Line 333: * load the policy from the URL returned by {@link
#getCodeServerPolicyUrl}.</p>
nit: <p> tags are not closed in javadoc. It is used like <br>.
Line 349: return policy;
It may be safer to fall through only if the serialization policy is not
found (like you documented)
or
perhaps we should always be loading from code server client if the code
server client URL is not null?
Line 363: * <p>By default, returns null. If the
<i>gwt.codeserver.port</i> servlet parameter is set,
This is no longer a servlet parameter.
Line 364: * returns a URL under <tt>http://localhost:{port}</tt>.</p>
nit: {@code} is preferred style over '<tt>' in javadoc.
Line 367: * @return the policy, or null if not available.
nit: null -> {@code null}
Line 369: protected String getCodeServerPolicyUrl(String strongName) {
Is this really something we want to be overridden?
If that is the case, then it is a good idea to document it so that
developers will know when to override this.
Line 383: protected SerializationPolicyClient getCodeServerClient() {
Instead of introducing new public interfaces and this method we may be
better off putting following 'protected' method that can be overridden for
customization:
protected SerializationPolicy readPolicyFromCodeServer(String strongName) {
String url = getCodeServerPolicyUrl(strongName));
return url == null ? null : return new
DefaultSerializationPolicyClient().loadPolicy(url);
}
(or something similar)
What is the exact use case we are covering here by letting the client
implementation to be replaced?
If use cases are not mature, it is better keep these private and then think
about it again after it gets mature.
--
To view, visit https://gwt-review.googlesource.com/2341
To unsubscribe, visit https://gwt-review.googlesource.com/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf2366cb94297e738cf50bb9d8038fef66b89722
Gerrit-PatchSet: 2
Gerrit-Project: gwt
Gerrit-Branch: master
Gerrit-Owner: Brian Slesinsky <[email protected]>
Gerrit-Reviewer: Goktug Gokdogan <[email protected]>
Gerrit-Reviewer: Matthew Dempsky <[email protected]>
Gerrit-HasComments: Yes
--
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
---
You received this message because you are subscribed to the Google Groups "Google Web Toolkit Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
