All done - thanks, Lex! On 2010/05/26 18:19:26, Lex wrote:
LGTM with nits. No need to rereview if you like the suggested changes.
These changes will make SOYC a lot easier to understand.
http://gwt-code-reviews.appspot.com/566801/diff/1/3 File dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java
(right):
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode179 dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:179: outFile.println("if (element.name == \"packageBreakdown\") {"); Putting ifs here doesn't scale well if we add more popups -- and we
should!
There are multiple ways to avoid an ever increasing chain of ifs here.
A simple
way would be to remove the "element" parameter and add one with the ID
of the
help popup.
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode284 dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:284: "onMouseOut=\"hide(this);\">Package breakdown</a></h2></div>"); If you go with the changed parameter, then show(this) would be changed
to
show(\"packageBreakdownPopup\");, and likewise for hide().
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode359 dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:359: outFile.println("<th>Size <span
class=\"soyc-th-units\">(Bytes)</span></th>");
Nice. It pains me to think how many times I looked at that table
header and
didn't notice the problem.
http://gwt-code-reviews.appspot.com/566801/diff/1/3#newcode728 dev/core/src/com/google/gwt/soyc/MakeTopLevelHtmlForPerm.java:728: TreeMap<Float, Set<String> > sortedCodeTypes = new TreeMap<Float,
Set<String> >(
Very nice, and likewise for the other ones. Could you also change the
keys from
Float to Integer? Comparison on floats is begging for weird corner
cases. http://gwt-code-reviews.appspot.com/566801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
