Change sanitizeCssProperty to take virtualization param like other sanitize* fns

We need sanitizeCssProperty to take an ID suffix so it can sanitize
<global-name>s.

sanitizeStylesheet and sanitizeCssSelectors take a virtualization property
with the necessary ID suffix.

This change changes sanitizeCssProperty and all callers to take the same
virtualization property and bundles base URI and the URI rewriter that all
take into the virtualization property.

This CL should not affect the semantics of a cajoled document.

----

Superseded by 11938045

[felix8a, 2013-07-25 23:31:13]

does this supersede the previous CL?

[MikeSamuel, 2013-07-25 23:54:27]

We need sanitizeCssProperty to take an ID suffix so it can sanitize
<global-name>s.

sanitizeStylesheet and sanitizeCssSelectors take a virtualization property
with the necessary ID suffix.

This change changes sanitizeCssProperty and all callers to take the same
virtualization property and bundles base URI and the URI rewriter that all
take into the virtualization property.

This CL should not affect the semantics of a cajoled document.

[MikeSamuel, 2013-07-25 23:59:39]

On 2013/07/25 23:54:27, MikeSamuel wrote:
> We need sanitizeCssProperty to take an ID suffix so it can sanitize
> <global-name>s.
> 
> sanitizeStylesheet and sanitizeCssSelectors take a virtualization property
> with the necessary ID suffix.
> 
> This change changes sanitizeCssProperty and all callers to take the same
> virtualization property and bundles base URI and the URI rewriter that all
> take into the virtualization property.
> 
> This CL should not affect the semantics of a cajoled document.

I closed https://codereview.appspot.com/11871043/ in favor of this.

[kpreid2, 2013-07-26 00:02:32]

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:518: var virtualization =
Object.create(domicile.virtualization);
Why does this exist as a distinct object? What is the difference in ... oh, I
see. The base URI for the stylesheet is different from the base URI for the
document.

I missed that point before.

Perhaps base URI should be _not_ in virtualization, and rewriteCssUri should
take it as a parameter rather than closing over it?

But this is OK for now too — maybe leave a comment.

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/html-sanitizer.js (right):

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-sanitizer.js:905: var virtualization = {
It seems wrong (and potentially bad for performance) that this is being
constructed anew for every property. I suggest changing the interface of
'virtualization.rewriteCssUri' so that it does not have to close over normProp.

[felix8a, 2013-07-26 00:12:34]

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/sanitizecss.js:124: * @param {{
sanitizeCssProperty is a public api. Changing the signature like this will break
some users. this was a minor problem last time it changed. I'm ok with changing
it, but releases are less traumatic if we do it as: add new interface, deprecate
old interface, migrate users, delete old interface.

[kpreid2, 2013-07-26 00:17:48]

On 2013/07/26 00:12:34, felix8a wrote:
>
https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
> File src/com/google/caja/plugin/sanitizecss.js (right):
> 
>
https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
> src/com/google/caja/plugin/sanitizecss.js:124: * @param {{
> sanitizeCssProperty is a public api. Changing the signature like this will
break
> some users. this was a minor problem last time it changed. I'm ok with
changing
> it, but releases are less traumatic if we do it as: add new interface,
deprecate
> old interface, migrate users, delete old interface.

Hm. Based on that, I'm not sure if the whole thing is a good idea — I didn't
intend 'virtualization' to become part of the public interface, and I had
forgotten about the fact that html[-css]-sanitizer is separately exposed. Now,
if 'virtualization' is a good interface, that's OK, but I don't think it is
based on the scope/data-flow issues I previously mentioned.

On the other hand I'm thinking very shallowly about the whole thing.

[MikeSamuel, 2013-07-26 00:21:57]

On 2013/07/26 00:12:34, felix8a wrote:
>
https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
> File src/com/google/caja/plugin/sanitizecss.js (right):
> 
>
https://codereview.appspot.com/11883043/diff/5001/src/com/google/caja/plugin/...
> src/com/google/caja/plugin/sanitizecss.js:124: * @param {{
> sanitizeCssProperty is a public api. Changing the signature like this will
break
> some users. this was a minor problem last time it changed. I'm ok with
changing
> it, but releases are less traumatic if we do it as: add new interface,
deprecate
> old interface, migrate users, delete old interface.

Fair enough.  I'll table this CL, rethink how to do this then, and tackle it
fresh tomorrow.