LGTM

Just a couple of nits.


http://gwt-code-reviews.appspot.com/706802/diff/1/2
File
bikeshed/src/com/google/gwt/sample/expenses/gwt/client/Scaffold.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/2#newcode87
bikeshed/src/com/google/gwt/sample/expenses/gwt/client/Scaffold.java:87:
shell.getLoginPanel().add(login);
Might as well  just instantiate the LoginWidget right in the ui.xml.
Give shell a getLoginWidget method instead of getLoginPanel

http://gwt-code-reviews.appspot.com/706802/diff/1/6
File
bikeshed/src/com/google/gwt/sample/expenses/gwt/request/UserInformationRecord.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/6#newcode47
bikeshed/src/com/google/gwt/sample/expenses/gwt/request/UserInformationRecord.java:47:
String getUniqueId();
Any reason to provide both this and getId(), which (for now) is required
by the Record interface anyway?

http://gwt-code-reviews.appspot.com/706802/diff/1/9
File
bikeshed/src/com/google/gwt/sample/expenses/server/domain/GaeUserInformation.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/9#newcode28
bikeshed/src/com/google/gwt/sample/expenses/server/domain/GaeUserInformation.java:28:
public class GaeUserInformation extends UserInformation{
Nit: need a space before the {

http://gwt-code-reviews.appspot.com/706802/diff/1/10
File bikeshed/war/WEB-INF/web.xml (right):

http://gwt-code-reviews.appspot.com/706802/diff/1/10#newcode13
bikeshed/war/WEB-INF/web.xml:13: <param-name>userInfoClass</param-name>
Please be sure to mention this in the RequestFactoryServlet javadoc (and
get rid of the stale thing there).

http://gwt-code-reviews.appspot.com/706802/diff/1/11
File
user/src/com/google/gwt/requestfactory/client/AuthenticationFailureHandler.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/11#newcode47
user/src/com/google/gwt/requestfactory/client/AuthenticationFailureHandler.java:47:
String loginUrl = response.getHeader("login");
Where is it documented that this header will be set?

http://gwt-code-reviews.appspot.com/706802/diff/1/12
File user/src/com/google/gwt/requestfactory/client/LoginWidget.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/12#newcode44
user/src/com/google/gwt/requestfactory/client/LoginWidget.java:44:
public void updateWithNewUserInformation(UserInformationProvider info) {
Can you make this setUserInformation? That's more idiomatic.

http://gwt-code-reviews.appspot.com/706802/diff/1/19
File
user/src/com/google/gwt/requestfactory/shared/UserInformationProvider.java
(right):

http://gwt-code-reviews.appspot.com/706802/diff/1/19#newcode23
user/src/com/google/gwt/requestfactory/shared/UserInformationProvider.java:23:
public interface UserInformationProvider {
Naming nits. Can you rename this interface to UserInformation, and make
your concrete implementation  UserInformationImpl?

http://gwt-code-reviews.appspot.com/706802/show

--
http://groups.google.com/group/Google-Web-Toolkit-Contributors

Reply via email to