[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rchandia]

LGTM. With doc-nits.


http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java#newcode51
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java:51:
* @return the local {@link Storage} object, or null unsupporting
browsers
...null on unsupporting...

http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/desktop/MobileWebAppShellDesktop.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/desktop/MobileWebAppShellDesktop.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/desktop/MobileWebAppShellDesktop.java#newcode205
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/desktop/MobileWebAppShellDesktop.java:205:
* @param clientFactory the {@link ClientFactory} of shared resources
No longer a single param

http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/mobile/MobileWebAppShellMobile.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/mobile/MobileWebAppShellMobile.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/4003/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/mobile/MobileWebAppShellMobile.java#newcode109
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/mobile/MobileWebAppShellMobile.java:109:
* @param clientFactory the {@link ClientFactory} of shared resources
No longer a single param

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

Ready for another look.

OrientationMonitor is replaced with OrientationHelper. John, I think
I've stumbled onto a pretty nice widget plugin pattern there. I was able
to delete the superclass for the shells without adding much boilerplate
or copy/paste. What do you think?

Notice that I've snuck a method onto the HasAttachHandlers interface. I
think we can get away with that since it's meant to be implemented by
Widget — I'm willing to break a few tests to make it more useful.

Hmm. I suppose I could instead introduce an interface into the demo
code, IsAttachable extends HasAttachHandlers. The change sure feels
right, though.


http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java#newcode50
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java:50:
* @return the local {@link Storage} object, or null unsupporting
browsers
On 2011/05/04 20:29:44, jlabanca wrote:

/r/null unsupporting/null in unsupporting


Done.

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[Ray Ryan]

Rodrigo, it looks like John is on vacation for the rest of the week. Can you
finish this review?

On Thu, May 5, 2011 at 2:24 PM, rj...@google.com wrote:

 Ready for another look.

 OrientationMonitor is replaced with OrientationHelper. John, I think
 I've stumbled onto a pretty nice widget plugin pattern there. I was able
 to delete the superclass for the shells without adding much boilerplate
 or copy/paste. What do you think?

 Notice that I've snuck a method onto the HasAttachHandlers interface. I
 think we can get away with that since it's meant to be implemented by
 Widget — I'm willing to break a few tests to make it more useful.

 Hmm. I suppose I could instead introduce an interface into the demo
 code, IsAttachable extends HasAttachHandlers. The change sure feels
 right, though.




 http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
 File

 samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
 (right):


 http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java#newcode50

 samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java:50:
 * @return the local {@link Storage} object, or null unsupporting
 browsers
 On 2011/05/04 20:29:44, jlabanca wrote:

 /r/null unsupporting/null in unsupporting


 Done.


 http://gwt-code-reviews.appspot.com/1433801/


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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

Sorry for the noise, not yet ready for review. Soon.

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[rjrjr]

Ready for review.

http://gwt-code-reviews.appspot.com/1433801/

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

[gwt-contrib] Re: Step one in making mobilewebapp more DI friendly. The goal is to (issue1433801)

[jlabanca]

LGTM if you make OrientationMonitor.provider() reference a singleton
boolean instead of recalculating.


http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java#newcode50
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ClientFactory.java:50:
* @return the local {@link Storage} object, or null unsupporting
browsers
/r/null unsupporting/null in unsupporting

http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ui/OrientationMonitor.java
File
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ui/OrientationMonitor.java
(right):

http://gwt-code-reviews.appspot.com/1433801/diff/3001/samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ui/OrientationMonitor.java#newcode43
samples/mobilewebapp/src/main/com/google/gwt/sample/mobilewebapp/client/ui/OrientationMonitor.java:43:
return calculate();
Calling calculate() every time we access the getter could lead to
performance degradation because the browser has to perform layout to
calculate window.clientHeight/Width.  Instead, can we use a singleton
instance of OrientationMonitor to maintain an orientation bit and check
it in the getter?

http://gwt-code-reviews.appspot.com/1433801/

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