[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
Upgrade GAE version http://gwt-code-reviews.appspot.com/1829803/diff/4001/samples/mobilewebapp/pom.xml File samples/mobilewebapp/pom.xml (right): http://gwt-code-reviews.appspot.com/1829803/diff/4001/samples/mobilewebapp/pom.xml#newcode21 samples/mobilewebapp/pom.xml:21: gae.version1.5.3/gae.version Upgrade to 1.7.1 to get fix for dev mode startup on Mac http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
I'm sorry I've only started to review the files (over the last few days)
but I have a first question/comment about where this is going:
There are many things that are not needed in the case of MobileWebApp as
the host page is protected behind authentication. Because the user won't
ever see this page when being unauthenticated:
- LoginWidget does not need to have a dual state (there's no need for a
login link)
- the username could be inlined in the script generated by the JSP
(that might not work very well with the ApplicationCache/offline though)
Also, there's probably no need to pass MobileWebApp.jsp as an init-param
to the GaeAuthFilter as it could just use getContextPath()
(MobileWebApp.jsp is in the welcome-file-list in the web.xml), or /
(the context path is always / on GAE)
As far as offline is concerned, it might get in our way here, but I'm no
expert in mobile dev. Anyway, it seems to cause us more harm than
anything, and the linker is known not to be great (it will load all
permutations in the client's cache, only to use one of them)
This would greatly simplify the code, at the expense of removing a bunch
of reusable code (LoginWidget, etc.)
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java
File
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java
(right):
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java#newcode18
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java:18:
import com.google.gwt.event.shared.EventBus;
Why this change?
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java
File
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java
(right):
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java#newcode18
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java:18:
import com.google.gwt.event.shared.EventBus;
Same question: what's the reason behind this change?
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java#newcode64
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java:64:
* @param state a {@link State} instance
This looks like a leftover from an experiment (remark also applies to
the javadoc on the loginUrl field)
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/webapp/MobileWebApp.jsp
File samples/mobilewebapp/src/main/webapp/MobileWebApp.jsp (right):
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/webapp/MobileWebApp.jsp#newcode37
samples/mobilewebapp/src/main/webapp/MobileWebApp.jsp:37:
window['__gwtsample_mobilewebapp__'].loginUrl =
'%=appUrls.getLoginUrl()%';
Wouldn't an object literal be more readable?
window['__gwtsample_mobilewebapp__'] = {
loginUrl: '%= appUrls.getLoginUrl() %',
logoutUrl: '%= appUrls.getLogoutUrl() %'
};
(I'd also use var __gwtsample_mobilewebapp__ = instead of
window['__gwtsample_mobilewebapp__'] = but that's more a matter of
personal taste)
Oh, and BTW, the page is protected (in web.xml), so there's no need for
the loginUrl here.
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/webapp/WEB-INF/web.xml
File samples/mobilewebapp/src/main/webapp/WEB-INF/web.xml (right):
http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/webapp/WEB-INF/web.xml#newcode10
samples/mobilewebapp/src/main/webapp/WEB-INF/web.xml:10: auth filters in
place for all other RPC calls. Thre's no real need to protect the
compiled
authentication auth filters
http://gwt-code-reviews.appspot.com/1829803/diff/9003/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/server/GaeAuthFilter.java
File
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/server/GaeAuthFilter.java
(right):
http://gwt-code-reviews.appspot.com/1829803/diff/9003/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/server/GaeAuthFilter.java#newcode93
samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/server/GaeAuthFilter.java:93:
homePageUrl.append(scheme);
No need for all this; UserService accepts paths, so
/ + homePageFileName + (queryString == null ? : ? +
queryString)
would be enough (even though I'd rather keep only the gwt.codesvr
param from the queryString)
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
Hey Thomas,
Thanks for looking this over. I'll reply to the code-specific comments in
Rietveld itself, but I thought I'd respond to your more pressing concerns
first.
On Fri, Sep 14, 2012 at 10:57 AM, t.bro...@gmail.com wrote:
I'm sorry I've only started to review the files (over the last few days)
but I have a first question/comment about where this is going:
There are many things that are not needed in the case of MobileWebApp as
the host page is protected behind authentication. Because the user won't
ever see this page when being unauthenticated:
- LoginWidget does not need to have a dual state (there's no need for a
login link)
- the username could be inlined in the script generated by the JSP
(that might not work very well with the ApplicationCache/offline though)
That is true. I think that I moved the auth functionality over to this
sample so it wouldn't be lost (since Expenses is deprecated, and doesn't
completely work anymore).
I think what needs to happen is that this functionality should be
abstracted out into a separate useful widget. I just didn't have the time
to do this, so I decided to move it over here. It did not seem useful to
leave it in Expenses.
However, if we think that the best solution is to abstract out the
LoginWidget and leave MobileWebApp as-is (since the LoginWidget
functionality doesn't really apply as directly here), I'm okay with that.
Also, there's probably no need to pass MobileWebApp.jsp as an init-param
to the GaeAuthFilter as it could just use getContextPath()
(MobileWebApp.jsp is in the welcome-file-list in the web.xml), or /
(the context path is always / on GAE)
True, I can fix that up.
As far as offline is concerned, it might get in our way here, but I'm no
expert in mobile dev. Anyway, it seems to cause us more harm than
anything, and the linker is known not to be great (it will load all
permutations in the client's cache, only to use one of them)
Sorry, are you saying that we should make this an online-only sample until
we straighten out the offline story? It's good to know about the
permutations issue with the client's cache; that's not ideal at all. What
other harmful aspects are you referring to?
This would greatly simplify the code, at the expense of removing a bunch
of reusable code (LoginWidget, etc.)
http://gwt-code-reviews.**appspot.com/1829803/diff/1/**
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthRequestTransport.javahttp://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java
File
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthRequestTransport.java
(right):
http://gwt-code-reviews.**appspot.com/1829803/diff/1/**
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthRequestTransport.java#**newcode18http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthRequestTransport.java#newcode18
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthRequestTransport.java:**18:
import com.google.gwt.event.shared.**EventBus;
Why this change?
http://gwt-code-reviews.**appspot.com/1829803/diff/1/**
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**javahttp://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java
File
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**java
(right):
http://gwt-code-reviews.**appspot.com/1829803/diff/1/**
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**java#newcode18http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java#newcode18
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**java:18:
import com.google.gwt.event.shared.**EventBus;
Same question: what's the reason behind this change?
http://gwt-code-reviews.**appspot.com/1829803/diff/1/**
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**java#newcode64http://gwt-code-reviews.appspot.com/1829803/diff/1/samples/mobilewebapp/src/main/java/com/google/gwt/sample/gaerequest/client/GaeAuthenticationFailureEvent.java#newcode64
samples/mobilewebapp/src/main/**java/com/google/gwt/sample/**
gaerequest/client/**GaeAuthenticationFailureEvent.**java:64:
* @param state a {@link State} instance
This looks like a leftover from an experiment (remark also applies to
the javadoc on the
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
On 2012/09/14 18:53:22, rdayal wrote: I think this one is going to be pushed past GWT 2.5; that's ok though. http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
On 2012/09/12 22:50:12, rdayal wrote: Ping...I'd like to get this into 2.5. http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Move GAE Auth functionality from Expenses over the MobileWebApp sample. (issue1829803)
On 2012/09/11 20:36:11, rdayal wrote: Had to re-create this; my client was messed up. Original review was here: http://gwt-code-reviews.appspot.com/1806803/ Note that this review is built on the changes here: http://gwt-code-reviews.appspot.com/1828803/ (hence they are included) http://gwt-code-reviews.appspot.com/1829803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
