http://codereview.appspot.com/116069/diff/2001/2029
File build.xml (right):
http://codereview.appspot.com/116069/diff/2001/2029#newcode58
Line 58: <!-- servlet dir - needs to be called "war" -->
It *needs* to be called "war"? Wow. Please add a little explanation of
why -- i.e., is that a requirement of the <dev_appserver> invoked later
on...?
http://codereview.appspot.com/116069/diff/2001/2029#newcode65
Line 65: <import
file="${ant.file.appengine-ant-macros}/config/user/ant-macros.xml"/>
Do you mean ${macros.appengine}? This looks like an artifact of
development.... And please line wrap.
http://codereview.appspot.com/116069/diff/2001/2029#newcode115
Line 115: <pathelement path="${third_party}/java/gwt/gwt-dev-mac.jar"/>
Will gwt-dev-mac.jar work on (say) Linux?
http://codereview.appspot.com/116069/diff/2001/2029#newcode633
Line 633: <java failonerror="true" fork="true"
classname="com.google.gwt.dev.Compiler">
Please line wrap here & elsewhere to the extent possible.
http://codereview.appspot.com/116069/diff/2001/2029#newcode663
Line 663: todir="${war}/WEB-INF/classes"
Are the third_party jars that the Caja code relies on not also copied
into WEB-INF/lib?
Also, is a WEB-INF/web.xml generated anywhere? How?
http://codereview.appspot.com/116069/diff/2001/2029#newcode675
Line 675: description="Starts the development server.">
Should specify depends="" to build the server?
http://codereview.appspot.com/116069/diff/2001/2018
File src/com/google/caja/demos/playground/client/CajolingService.java
(right):
http://codereview.appspot.com/116069/diff/2001/2018#newcode12
Line 12: public interface CajolingService extends RemoteService {
So this caused a name conflict with The Class Formerly Known As
CajolingService [TM]. Hmm.... This class is really the main facade for
the Playground to talk back to its server, so PlaygroundBackendService
may not be too specific.
http://codereview.appspot.com/116069/diff/2001/2018#newcode17
Line 17: * @return Returns the cajoled html, js and cajoling messages as
an array
s/Returns//g.
Also, not clear from this comment that you mean:
return[0] -> html
return[1] -> js
return[2 .. (return.length - 1)] -> messages
http://codereview.appspot.com/116069/diff/2001/2016
File
src/com/google/caja/demos/playground/client/CajolingServiceAsync.java
(right):
http://codereview.appspot.com/116069/diff/2001/2016#newcode20
Line 20: */
Doc comment format ... and missing @author.
http://codereview.appspot.com/116069/diff/2001/2016#newcode25
Line 25:
Extra blank line.
http://codereview.appspot.com/116069/diff/2001/2017
File
src/com/google/caja/demos/playground/client/CajolingServiceResult.java
(right):
http://codereview.appspot.com/116069/diff/2001/2017#newcode29
Line 29: public CajolingServiceResult(String html, String javascript,
String[] messages) {
Line wrap.
http://codereview.appspot.com/116069/diff/2001/2015
File src/com/google/caja/demos/playground/client/Playground.java
(right):
http://codereview.appspot.com/116069/diff/2001/2015#newcode27
Line 27:
Extra blank line.
http://codereview.appspot.com/116069/diff/2001/2015#newcode28
Line 28: private PlaygroundView gui;
If the view always passed in callbacks (or listened for events, should
you need them), the model would never have to retain an explicit upward
pointer to its view.
http://codereview.appspot.com/116069/diff/2001/2015#newcode34
Line 34: cajolingService.fetch(url, new AsyncCallback<String>() {
Can you push the CPS all the way through, i.e., the view can send the
callback to the model, rather than having the model do direct upcalls to
the view?
For a system this small, this is an optional issue....
http://codereview.appspot.com/116069/diff/2001/2015#newcode60
Line 60: gui.setRenderedResult(result[0]);
Did you mean result[1]?
http://codereview.appspot.com/116069/diff/2001/2015#newcode61
Line 61: for (int i=2; i < result.length - 2; i++) {
Spaces around "=".
http://codereview.appspot.com/116069/diff/2001/2019
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java
(right):
http://codereview.appspot.com/116069/diff/2001/2019#newcode57
Line 57:
Extra blank line.
http://codereview.appspot.com/116069/diff/2001/2019#newcode66
Line 66: public void setVersion(String v) {
Probably unnecessary if you adopt Model/Delegate and pass a callback
directly to the Model.
http://codereview.appspot.com/116069/diff/2001/2019#newcode70
Line 70: public void selectTab(int index) {
At the very least, the indices used here should be named constants
somewhere.
http://codereview.appspot.com/116069/diff/2001/2019#newcode78
Line 78: for (Menu menu: Menu.values()) {
Space around ":".
http://codereview.appspot.com/116069/diff/2001/2019#newcode96
Line 96: logoPanel.add(new
Image("http://cajadores.com/demos/testbed/caja_logo_small.png";));
Line wrap.
http://codereview.appspot.com/116069/diff/2001/2019#newcode108
Line 108: public void onFocus(FocusEvent event) {
Don't you wish Java had concise closures? :)
http://codereview.appspot.com/116069/diff/2001/2019#newcode165
Line 165: private DecoratedTabPanel createEditorPanel() {
You do realize are dangerously close to implementing the Eclipse UI in
GWT, my friend.... };->
http://codereview.appspot.com/116069/diff/2001/2019#newcode234
Line 234: public PlaygroundView(Playground controller) {
Oh I see, so you're doing MVC rather than Model/Delegate:
http://c2.com/cgi/wiki?ModelDelegate
Is MVC more common than Model/Delegate for GWT programming?
In any case, I'm not sure this usage actually fits with classic MVC.
There, the View sends requests to the Controller, the Controller sends
commands to the Model, and the View connects directly to the Model to
update its state. The Controller does not maintain a back-pointer to the
View.
And then -- the realization that the Controller is either logic that
should be in the View, or stuff that belongs in the Model, leads us to
bundle View and Controller together and get Model/Delegate.
http://codereview.appspot.com/116069/diff/2001/2019#newcode262
Line 262: System.out.println("Output:<"+prettyPrint(result)+">\n");
Spaces around "+"
http://codereview.appspot.com/116069/diff/2001/2019#newcode268
Line 268: }-*/;
Wow, native. Cool!
http://codereview.appspot.com/116069/diff/2001/2019#newcode270
Line 270: public void setRenderedResult(String result) {
Again, probably not necessary (at least not as a *public* method) if you
adopt Model/Delegate and pass callbacks to the model.
http://codereview.appspot.com/116069/diff/2001/2019#newcode270
Line 270: public void setRenderedResult(String result) {
So ... how do cajita.js and domita.js and whatever else get actually
included in the GWT-served HTML content? Is there a container HTML page
that did not get included in this CL?
http://codereview.appspot.com/116069/diff/2001/2019#newcode282
Line 282: " var imports = ___.copy(___.sharedImports);\n" +
Continuation indent.
http://codereview.appspot.com/116069/diff/2001/2019#newcode286
Line 286: " attachDocumentStub('-g___', { rewrite: function() {return
null;} }, imports, gadgetRoot);\n" +
Line wrap.
http://codereview.appspot.com/116069/diff/2001/2019#newcode289
Line 289: htmlAndJs[1].substring(0, htmlAndJs[1].length() - 9);
Perhaps pull the removal of the ending </script> into the regex above,
rather than hiding it here. :)
http://codereview.appspot.com/116069/diff/2001/2014
File
src/com/google/caja/demos/playground/server/GWTCajolingServiceImpl.java
(right):
http://codereview.appspot.com/116069/diff/2001/2014#newcode40
Line 40: implements CajolingService {
Indent 2 more spaces.
http://codereview.appspot.com/116069/diff/2001/2014#newcode52
Line 52: + "&mime-type=" + URLEncoder.encode(mimeType, "UTF-8"));
We point URIs to the cajoling service? Can we point them back to the
current GWT server instead?
If that's too much trouble (requiring an entire cajoling service to be
built into the GWT thingey) then please just add a TODO.
http://codereview.appspot.com/116069/diff/2001/2014#newcode119
Line 119: public String fetch(String url) {
Is this used anywhere?
http://codereview.appspot.com/116069/diff/2001/2012
File src/com/google/caja/service/DefaultCajolingService.java (right):
http://codereview.appspot.com/116069/diff/2001/2012#newcode50
Line 50: public class DefaultCajolingService extends HttpServlet {
Not sure about this rename ... see comments on the new CajolingService.
http://codereview.appspot.com/116069/diff/2001/2012#newcode93
Line 93: public void doPost(HttpServletRequest req, HttpServletResponse
resp)
Per our discussion, see the doPost I added.
http://codereview.appspot.com/116069/diff/2001/2002
File src/log4j.properties (right):
http://codereview.appspot.com/116069/diff/2001/2002#newcode1
Line 1: # Configure the console as our one appender
This seems identical to the one in third_party/java/appengine/. Was it
your intention to add it directly to src/ as well, or should this be
copied in via a build rule?
http://codereview.appspot.com/116069/diff/2001/2027
File third_party/java/appengine/config/user/ant-macros.xml (right):
http://codereview.appspot.com/116069/diff/2001/2027#newcode1
Line 1: <!--
The third_party/java/appengine/ and third_party/java/gwt/ dirs are
missing a README and LICENSE.
(Re the weird placement of this comment -- it needs to be attached to
some text file line in order for the code review tool to accept it.)
http://codereview.appspot.com/116069/diff/2001/2027#newcode1
Line 1: <!--
I notice gwt-dev-mac.jar. Should there also be other
gwt-dev-${platform}.jar checked in for multiplatform development?
http://codereview.appspot.com/116069
![http://cajadores.com/demos/testbed/caja_logo_small.png"](http://cajadores.com/demos/testbed/caja_logo_small.png"){kind=link}