[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