Add more hints to URI policy, and improve "target" attribute handling

* Consistently send more "hints" to the container's URI policy
  upon invocation.
* Modify "target" attribute handling so that the server side
  cajoler always produces markup with target="_blank" then always
  consults a default and whitelist supplied by the container on
  the client side for rewriting.

Assigning felix8a as primary reviewer; jasvir because he had some questions about the API of this change when it was being developed.

[felix8a, 2012-05-06 00:09:28]

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:250: if (typeof
partial.targetAttributePresets.whitelist.length === 0) {
stray 'typeof'

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:258: };
it bothers me a little that the default value is duplicated here and in
domado.js, but I don't feel strongly about it.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/do...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/do...
src/com/google/caja/plugin/domado.js:5039: console.log("#####
rewriteTargetAttribute(" + value + ", " + tagName + ", " + attribName + ")")
delete

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
File src/com/google/caja/plugin/es53-frame-group.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
src/com/google/caja/plugin/es53-frame-group.js:259: for (var i = 0; i <
config.targetAttributePresets.whitelist.length; i++) {
how about use slice to make the copy?

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
src/com/google/caja/plugin/es53-frame-group.js:267: undefined,
targetAttributePresets, true);
"true" is an extraneous arg

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/TemplateCompiler.java:308: /*
this is commented out?

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/TemplateCompiler.java:320:
System.err.println("r is not safe: " + r.result);
is this an error or not?

http://codereview.appspot.com/6182049/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/container.js (right):

http://codereview.appspot.com/6182049/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/container.js:55: return "rewritten-" + value;
this rewrite doesn't show up in any tests.  it looks like container.js is only
used by HtmlCompiledPluginTest.java, and none of those trigger
rewriteTargetAttribute___.

which is odd, because one of the tests has an <a> tag and tests that the result
has target="_self", which should not have happened.  did that test really pass?

[ihab.awad, 2012-05-09 04:43:54]

Snapshotted.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:250: if (typeof
partial.targetAttributePresets.whitelist.length === 0) {
On 2012/05/06 00:09:28, felix8a wrote:
> stray 'typeof'

Done.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:258: };
On 2012/05/06 00:09:28, felix8a wrote:
> it bothers me a little that the default value is duplicated here and in
> domado.js, but I don't feel strongly about it.

You're right; I removed it from here.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/do...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/do...
src/com/google/caja/plugin/domado.js:5039: console.log("#####
rewriteTargetAttribute(" + value + ", " + tagName + ", " + attribName + ")")
On 2012/05/06 00:09:28, felix8a wrote:
> delete

Done.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
File src/com/google/caja/plugin/es53-frame-group.js (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
src/com/google/caja/plugin/es53-frame-group.js:259: for (var i = 0; i <
config.targetAttributePresets.whitelist.length; i++) {
On 2012/05/06 00:09:28, felix8a wrote:
> how about use slice to make the copy?

Done.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/es...
src/com/google/caja/plugin/es53-frame-group.js:267: undefined,
targetAttributePresets, true);
On 2012/05/06 00:09:28, felix8a wrote:
> "true" is an extraneous arg

Done.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
File src/com/google/caja/plugin/templates/TemplateCompiler.java (right):

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/TemplateCompiler.java:308: /*
On 2012/05/06 00:09:28, felix8a wrote:
> this is commented out?

Yes, cruft. Removed.

http://codereview.appspot.com/6182049/diff/3001/src/com/google/caja/plugin/te...
src/com/google/caja/plugin/templates/TemplateCompiler.java:320:
System.err.println("r is not safe: " + r.result);
On 2012/05/06 00:09:28, felix8a wrote:
> is this an error or not?

No, per the below comment, we drop stuff on the floor and rely on previously
executed code to have warned. This was just debugging cruft. Removed.

http://codereview.appspot.com/6182049/diff/3001/tests/com/google/caja/plugin/...
File tests/com/google/caja/plugin/container.js (right):

http://codereview.appspot.com/6182049/diff/3001/tests/com/google/caja/plugin/...
tests/com/google/caja/plugin/container.js:55: return "rewritten-" + value;
On 2012/05/06 00:09:28, felix8a wrote:
> this rewrite doesn't show up in any tests.  it looks like container.js is only
> used by HtmlCompiledPluginTest.java, and none of those trigger
> rewriteTargetAttribute___.
> 
> which is odd, because one of the tests has an <a> tag and tests that the
result
> has target="_self", which should not have happened.  did that test really
pass?

You are right -- I must have forgotten to re-run the test before submitting.

A simple corresponding change to HtmlCompiledPluginTest and all is well.

[ihab.awad, 2012-05-09 04:43:57]

Snapshotted.

[felix8a, 2012-05-09 16:06:34]

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:49: ATTRIBUTETYPES: undefined,
please quote these keys for Closure

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:246: if (partial.targetAttributePresets) {
please use partial['targetAttributePresets'], and below.

when you use o.y and Closure doesn't have a specific type for o, it assumes o is
an Object, and .y is a property of all Objects, and
1, it will complain if there isn't any assignment o2.y = z
2, it will rename all occurrences of .y to something shorter
which screws up when the .y is a name in an api for external linkage.

http://codereview.appspot.com/6182049/diff/12003/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-target-attribute-presets.js (right):

http://codereview.appspot.com/6182049/diff/12003/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-target-attribute-presets.js:73:
'action="foo"');
shouldn't this be target="foo"?  did this pass?

[ihab.awad, 2012-05-09 18:13:11]

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:49: ATTRIBUTETYPES: undefined,
On 2012/05/09 16:06:34, felix8a wrote:
> please quote these keys for Closure

Done.

http://codereview.appspot.com/6182049/diff/12003/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:246: if (partial.targetAttributePresets) {
On 2012/05/09 16:06:34, felix8a wrote:
> please use partial['targetAttributePresets'], and below.
> 
> when you use o.y and Closure doesn't have a specific type for o, it assumes o
is
> an Object, and .y is a property of all Objects, and
> 1, it will complain if there isn't any assignment o2.y = z
> 2, it will rename all occurrences of .y to something shorter
> which screws up when the .y is a name in an api for external linkage.

Done.

http://codereview.appspot.com/6182049/diff/12003/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-target-attribute-presets.js (right):

http://codereview.appspot.com/6182049/diff/12003/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-target-attribute-presets.js:73:
'action="foo"');
On 2012/05/09 16:06:34, felix8a wrote:
> shouldn't this be target="foo"?  did this pass?

No it fails (whew!). I think I ran the test with a hacked-up html4-defs file and
needed to do an 'ant clean'.

[felix8a, 2012-05-09 19:11:09]

LGTM

[felix8a, 2012-05-09 20:58:02]

lgtm again

[Jasvir, 2012-05-09 22:49:43]

LGTM

[ihab.awad, 2012-11-09 22:10:07]

@4874