Implement non-simple event handler attributes in ES5 mode.

When Domado meets an event handler attribute value which is not
'simple', it now uses cajaVM.compileExpr (if available, i.e. if in ES5
mode) to compile the handler.

Fixes issue 1436, "domado needs better support for onevent attributes".

[metaweta, 2012-09-20 20:50:06]

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1755: if ((match =
value.match(SIMPLE_HANDLER_PATTERN))) {
Is there a reason to keep the simple handler pattern instead of always using
compileExpr?

[felix8a, 2012-09-20 20:58:00]

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1755: if ((match =
value.match(SIMPLE_HANDLER_PATTERN))) {
On 2012/09/20 20:50:06, metaweta wrote:
> Is there a reason to keep the simple handler pattern instead of always using
> compileExpr?

es53 doesn't have a compileExpr

[metaweta, 2012-09-20 21:08:58]

On 2012/09/20 20:58:00, felix8a wrote:
>
https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
> File src/com/google/caja/plugin/domado.js (right):
> 
>
https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
> src/com/google/caja/plugin/domado.js:1755: if ((match =
> value.match(SIMPLE_HANDLER_PATTERN))) {
> On 2012/09/20 20:50:06, metaweta wrote:
> > Is there a reason to keep the simple handler pattern instead of always using
> > compileExpr?
> 
> es53 doesn't have a compileExpr

Oh yeah.  Duuhh, sorry.

[kpreid2, 2012-09-20 21:10:47]

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1755: if ((match =
value.match(SIMPLE_HANDLER_PATTERN))) {
On 2012/09/20 20:50:06, metaweta wrote:
> Is there a reason to keep the simple handler pattern instead of always using
> compileExpr?

1. If we are in ES5/3 mode we still need this case since there is no eval.

2. The eval case has a small resource leak since domicile.handlers is never
cleaned up, whereas the simple handler case does not need any external table. We
could eliminate the table by adding an interface to dispatchEvent and friends to
do string eval, but that would mean SES pre-eval free-variable checking on every
single firing of the event (consider mousemove).

domicile.handlers is the table used to store cajoler-compiled event handlers,
incidentally.

(3. There is also a test which explicitly tests for the
___.plugin_dispatchEvent___ code, but that could be changed.)


There is an alternative strategy to using domicile.handlers: we could, instead
of constructing a string attribute, refactor attribute sanitization so that
onfoo="..." becomes the combination of:
(a) data-caja-onfoo="..." (nonfunctional, just saving the string value)
(b) element.onfoo = <compiled closure>
I'm pretty sure that, if it doesn't have any surprises I missed, this is a
superior implementation all round, even for ES5/3 mode — but it's an extensive
refactoring to make that possible, and I'd rather add this feature now in a
consistent-with-existing-practice way and then try out the other later.

(I should ask Mike Samuel if he had a reason to avoid .onfoo= in the original
cajoler design.)

[felix8a, 2012-09-20 21:26:40]

some minor comments below, otherwise LGTM.  I have vague qualms about not having
a testcase where es53 rejects a too-complex handler, but it's not clear to me
what good that would do, so no point adding that.

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1755: if ((match =
value.match(SIMPLE_HANDLER_PATTERN))) {
please move = assignment out if the if statement

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1768: ) - 1;
this - 1 hanging off here looks odd, how about break up the expression, use a
temp var for the result of compileExpr?

https://codereview.appspot.com/6554048/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-domado-dom-guest.html (right):

https://codereview.appspot.com/6554048/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-domado-dom-guest.html:1180: <input
onclick="window.autoclicky.click(0)" id="autoclicker" type="checkbox">
icky, but oh well

[kpreid2, 2012-09-20 21:53:30]

Patch Set 2 incorporates your comments.

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1755: if ((match =
value.match(SIMPLE_HANDLER_PATTERN))) {
On 2012/09/20 21:26:40, felix8a wrote:
> please move = assignment out if the if statement

Done. Originally this branch was an else-if, in which case I see no way to
*cleanly* achieve the proper control flow with such a constraint.

https://codereview.appspot.com/6554048/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:1768: ) - 1;
On 2012/09/20 21:26:40, felix8a wrote:
> this - 1 hanging off here looks odd, how about break up the expression, use a
> temp var for the result of compileExpr?

Done.

https://codereview.appspot.com/6554048/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-domado-dom-guest.html (right):

https://codereview.appspot.com/6554048/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-domado-dom-guest.html:1180: <input
onclick="window.autoclicky.click(0)" id="autoclicker" type="checkbox">
On 2012/09/20 21:26:40, felix8a wrote:
> icky, but oh well

Also wrong. Only assignments need to be rewritten (as below). Reverted.

[felix8a, 2012-09-20 21:58:46]

lgtm

[kpreid2, 2012-09-20 22:08:27]

@r5059