ok, finally popped my stack enough. will re-snapshot.
On 2009/07/09 19:18:51, ihab.awad wrote:
A few comments -- will discuss further over chat and report back to
the list.
http://codereview.appspot.com/89073/diff/3037/3040 File src/com/google/caja/plugin/domita.js (right):
http://codereview.appspot.com/89073/diff/3037/3040#newcode773 Line 773: '([^' + XML_SPACE + ']+)([' + XML_SPACE + ']|$)', 'g'); Plase move the above 4 lines down closer to the point of first use.
ok
http://codereview.appspot.com/89073/diff/3037/3040#newcode849 Line 849: function(m0, m1, m2) { Please give these parameters descriptive names. Like m0 -> "_"
(unused); m1 ->
"idref", m2 -> "trailingWhitespace"?
ok
http://codereview.appspot.com/89073/diff/3037/3040#newcode852 Line 852: === m1.substring(m1.length - idSuffix.length)) { Please parcel up the above 3 lines into an "endsWith(aString,
aSuffix)" function
and use it.
wrote something simpler.
http://codereview.appspot.com/89073/diff/3037/3040#newcode893 Line 893: case html4.atype.GLOBAL_NAME: What is the justification for suffixing a GLOBAL_NAME (or any kind of
"name" for
that matter) with an ID suffix? And if you suffix a GLOBAL_NAME, why
not also a
LOCAL_NAME? Do you understand the source of the distinction in our
schemata
between GLOBAL_NAME and LOCAL_NAME? The W3C page http://www.w3.org/TR/html401/index/attributes.html, which is
referenced from our
html4-attributes-defs.json, is not helpful in that regard.
I'm mirroring how TemplateCompiler behaves. GLOBAL_NAMEs are things like <form name=xxx>, and the reason for suffixing those is that IE exposes those names as global vars, if there isn't already a var declaration. however, suffixing them screws up anchors, because <a name=xxx> is also a GLOBAL_NAME. I don't know yet how to fix anchors without opening up the global namespace in IE. I think there's an open bug on this.
http://codereview.appspot.com/89073/diff/3037/3040#newcode2012 Line 2012: switch (atype) { If you are going to add 'idSuffix' to GLOBAL_NAME attributes in rewriteAttribute(), shouldn't you be stripping them off here?
yeah, fixed.
http://codereview.appspot.com/89073/diff/3037/3041 File src/com/google/caja/plugin/templates/TemplateCompiler.java
(right):
http://codereview.appspot.com/89073/diff/3037/3041#newcode292 Line 292: if (!checkIllegalSuffixes(value, pos)) { return; } Why remove illegal suffixes? We append an ID suffix where appropriate,
and that
should be robust enough.
In this case, we're not appending an idSuffix.
http://codereview.appspot.com/89073/diff/3037/3041#newcode297 Line 297: if (!checkIllegalSuffix(value, pos)) { return; } Here and in 2 other places below: Why remove illegal suffixes?
I'm wary of potential for abuse if the mangle/unmangle symmetry ever accidentally gets screwed up. given that I had to fix some cases mangle/unmangle asymmetry, and still missed one, the paranoia seems justified. also it seems confusing to have different restrictions for only some types of ID. http://codereview.appspot.com/89073
