An emulated function should have the same name and length as the original.

An emulated function should have the same name and length as the original.
Fixes https://code.google.com/p/google-caja/issues/detail?id=1928 .

[MarkM, 2014-08-03 02:12:30]

Intended to fix
Doesn't work yet.

[kpreid2, 2014-08-03 02:28:29]

LGTM, please note request for performance measurement

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:664: var makeStandin =
unsafeEval(makeStandinSrc);
I am concerned about the performance of this wrapper, especially as it is
heavily used in the taming membrane. I'd like to see a rough measurement of the
performance impact on the membrane.

If there is significant impact, consider reducing it by memoizing makeStandin
functions (indexed by parameter count, and, in a future version, name (per the
comment)).

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:674: if (Object.isFrozen(newFunc)) {
At first I was thinking this was redundant, because frozenness and sealedness
are about extensibility + property flags, and the latter is copied by the above.

However, it does end up applying to any exempt not-already-immutable properties.
It is also useful if a future spec edition (or nonstandard extension) introduces
objects such that freeze does more than setting non-extensible and modifying
properties (e.g. I believe proxies are a near miss at being such a thing).

Might be worth a comment giving the rationale.

[MarkM, 2014-08-03 16:42:56]

Intended to fix
Doesn't work yet.

[MarkM, 2014-08-03 16:46:58]

PTAL

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:664: var makeStandin =
unsafeEval(makeStandinSrc);
On 2014/08/03 02:28:28, kpreid2 wrote:
> I am concerned about the performance of this wrapper, especially as it is
> heavily used in the taming membrane. I'd like to see a rough measurement of
the
> performance impact on the membrane.
> 
> If there is significant impact, consider reducing it by memoizing makeStandin
> functions (indexed by parameter count, and, in a future version, name (per the
> comment)).

The cache seems like an adequately compelling idea that I went ahead and
implemented it without first measuring performance. PTAL, and let me know
whether you think perf measure is still needed. If so, please suggest what to
measure in order to measure impact on membrane usage.

https://codereview.appspot.com/121970043/diff/20001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:674: if (Object.isFrozen(newFunc)) {
On 2014/08/03 02:28:28, kpreid2 wrote:
> At first I was thinking this was redundant, because frozenness and sealedness
> are about extensibility + property flags, and the latter is copied by the
above.
> 
> However, it does end up applying to any exempt not-already-immutable
properties.
> It is also useful if a future spec edition (or nonstandard extension)
introduces
> objects such that freeze does more than setting non-extensible and modifying
> properties (e.g. I believe proxies are a near miss at being such a thing).
> 
> Might be worth a comment giving the rationale.

Done.

[MarkM, 2014-08-03 18:18:34]

Intended to fix
Doesn't work yet.

[MarkM, 2014-08-03 18:19:44]

Took case or the .name TODO. It now does both .name and .length

[MarkM, 2014-08-03 18:20:30]

So PTAL++

[MarkM, 2014-08-03 18:23:04]

On 2014/08/03 18:19:44, MarkM wrote:
> Took case or the .name TODO. It now does both .name and .length

Oops. Should be "Took care of the ..."

[MarkM, 2014-08-03 19:14:42]

Intended to fix
Doesn't work yet.

[kpreid2, 2014-08-04 16:54:26]

LGTM

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:704: cacheLine =
standinMakerCache.get((standinName = 'standin'));
Please put the assignment on the previous line; this is obscurity to little
benefit.

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:707: standinMakerCache.set(standinName,
((cacheLine = [])));
Same: please separate the assignment.

I would have instead made up a key by concatenating the function name and arity,
so the cache is a single-level data structure, but this seems OK too.

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:721: cacheLine[arity] = ((result =
unsafeEval(makeStandinSrc)));
separate assignment

[MarkM, 2014-08-04 17:23:31]

Intended to fix
Doesn't work yet.

[MarkM, 2014-08-04 17:25:25]

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:704: cacheLine =
standinMakerCache.get((standinName = 'standin'));
On 2014/08/04 16:54:26, kpreid2 wrote:
> Please put the assignment on the previous line; this is obscurity to little
> benefit.

Done.

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:707: standinMakerCache.set(standinName,
((cacheLine = [])));
On 2014/08/04 16:54:26, kpreid2 wrote:
> Same: please separate the assignment.

Done

> I would have instead made up a key by concatenating the function name and
arity,
> so the cache is a single-level data structure, but this seems OK too.

This way, I get to reuse the first level for the blacklist check.

https://codereview.appspot.com/121970043/diff/80001/src/com/google/caja/ses/r...
src/com/google/caja/ses/repairES5.js:721: cacheLine[arity] = ((result =
unsafeEval(makeStandinSrc)));
On 2014/08/04 16:54:26, kpreid2 wrote:
> separate assignment

Done.

[kpreid2, 2014-08-04 17:33:17]

Oops. I forgot to mention: the change description needed to be updated!

[MarkM, 2014-08-04 17:36:56]

On 2014/08/04 17:33:17, kpreid2 wrote:
> Oops. I forgot to mention: the change description needed to be updated!

I am surprised that it let me edit after the issue was closed, but it did. Done.

[kpreid_google, 2014-08-04 17:49:49]

On Mon, Aug 4, 2014 at 10:36 AM, <erights@gmail.com> wrote:

> On 2014/08/04 17:33:17, kpreid2 wrote:
>
>> Oops. I forgot to mention: the change description needed to be
>>
> updated!
>
> I am surprised that it let me edit after the issue was closed, but it
> did. Done.


That doesn't modify the SVN commit message, which is approximately
immutable. If you're going to write draft descriptions like "doesn't work",
please make sure to check that the description is accurate before
committing.

[Mark S. Miller, 2014-08-04 17:51:00]

understood


On Mon, Aug 4, 2014 at 10:49 AM, 'Kevin Reid' via Google Caja Discuss <
google-caja-discuss@googlegroups.com> wrote:

> On Mon, Aug 4, 2014 at 10:36 AM, <erights@gmail.com> wrote:
>
>> On 2014/08/04 17:33:17, kpreid2 wrote:
>>
>>> Oops. I forgot to mention: the change description needed to be
>>>
>> updated!
>>
>> I am surprised that it let me edit after the issue was closed, but it
>> did. Done.
>
>
> That doesn't modify the SVN commit message, which is approximately
> immutable. If you're going to write draft descriptions like "doesn't work",
> please make sure to check that the description is accurate before
> committing.
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Google Caja Discuss" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to google-caja-discuss+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>



-- 
    Cheers,
    --MarkM