New host page tools for ES5/3.

New host page tools for ES5/3.

[Jasvir, 2010-10-29 17:51:14]

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
File tests/com/google/caja/plugin/es53-test-taming-cajoled.html (right):

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming-cajoled.html:180:
assertFalse(called);
Check that r is callable and calling r() sets called appropriately.

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
File tests/com/google/caja/plugin/es53-test-taming.js (right):

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming.js:42:
functionReturningMyFunction: function(f) { return f; }
nice

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming.js:57: var extraImports =
createStandardImports(frameGroup, frame);
Where does createStandardImports come from?  Would it be useful on frameGroup?

[ihab.awad, 2010-10-29 18:07:39]

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
File tests/com/google/caja/plugin/es53-test-taming-cajoled.html (right):

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming-cajoled.html:180:
assertFalse(called);
On 2010/10/29 17:51:14, jasvir wrote:
> Check that r is callable and calling r() sets called appropriately.

Isn't that what the next 2 lines do?

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
File tests/com/google/caja/plugin/es53-test-taming.js (right):

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming.js:42:
functionReturningMyFunction: function(f) { return f; }
On 2010/10/29 17:51:14, jasvir wrote:
> nice

Done.

http://codereview.appspot.com/2745043/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-taming.js:57: var extraImports =
createStandardImports(frameGroup, frame);
On 2010/10/29 17:51:14, jasvir wrote:
> Where does createStandardImports come from?  Would it be useful on frameGroup?

createStandardImports is defined in browser-test-case.js. It involves a bunch of
jsUnit stuff. At the level of the frameGroup and frame methods, each cajoled
module *is* called with a set of "standard" imports (Function, Array, ...) --
you get to pass in the "extra" ones. createStandardImports creates a standard
set of "extra" ones. Hm. :P

I think I'll rename it to createExtraImportsForTesting().

[ihab.awad, 2010-11-03 18:48:59]

Please re-review the API for invoking the taming. See the following files:

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...

The remainder is simply implementation; we can review it when we decide that the
API is reasonable.

[Jasvir, 2010-11-04 20:09:36]

new api LGTM

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-taming-cajoled.html (right):

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming-cajoled.html:104: <div
id="testFunctionReturningRecord" class="testcontainer">
Ok so these are missing tests for frozeness of the records and what happens if a
frozen record is modified by host code as discussed offline.  I assume you plan
to add this after the implementation cleanup?

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-taming.js (right):

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming.js:66: functionReturningPrimitive:
function (y) {
Good test.

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming.js:105: api.Ctor.CTOR___ = true;
Is CTOR___ and SUPER_CTOR___ a tag that's part of the new taming api?

[ihab.awad, 2010-11-04 20:19:14]

Thanks for the interim review. New implementation for taming machinery, and new
tests for the extra semantics, in the works.

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-taming-cajoled.html (right):

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming-cajoled.html:104: <div
id="testFunctionReturningRecord" class="testcontainer">
On 2010/11/04 20:09:36, jasvir wrote:
> Ok so these are missing tests for frozeness of the records and what happens if
a
> frozen record is modified by host code as discussed offline.  I assume you
plan
> to add this after the implementation cleanup?

Yes -- these tests are against the current implementation which just freezes
snapshots of everything. Next version to come.

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-taming.js (right):

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming.js:66: functionReturningPrimitive:
function (y) {
On 2010/11/04 20:09:36, jasvir wrote:
> Good test.

Done.

http://codereview.appspot.com/2745043/diff/12001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-taming.js:105: api.Ctor.CTOR___ = true;
On 2010/11/04 20:09:36, jasvir wrote:
> Is CTOR___ and SUPER_CTOR___ a tag that's part of the new taming api?

Yes.

As is CONSTRUCTED___, which is only necessary when the host code is returning an
object that *it* constructed from some ctor.

[MarkM, 2010-11-08 03:33:27]

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:256: if (f !== null && f !== undefined &&
f.IS_CAJA_FRAME_OBJECT___) {
This test will pass even if f is an object in another frame that inherits
IS_CAJA_FRAME_OBJECT___ cross frame. Is that safe?

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:264: && f.TAMED_TWIN___.FERAL_TWIN___
Since at this point you've already established that f is an object, and
therefore truthy, you can omit this middle test since the following "=== f" test
makes it redundant.

As with the old code, the new code still deserves a comment explaining that this
backpointing check serves as a cheap hasOwnProp check.

[ihab.awad, 2010-11-09 17:04:00]

I just added some API docs for the new stuff. To read them properly, you will
have to patch the CL into a local client then go to
doc/html/introAndTaming/index.html.

[ihab.awad, 2010-11-11 00:15:30]

Responded to MarkM's latest comments. Fixed array taming and added tests for the
behavior (including its quirks).

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js
File src/com/google/caja/es53.js (right):

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:256: if (f !== null && f !== undefined &&
f.IS_CAJA_FRAME_OBJECT___) {
On 2010/11/08 03:33:27, MarkM wrote:
> This test will pass even if f is an object in another frame that inherits
> IS_CAJA_FRAME_OBJECT___ cross frame. Is that safe?

By this inheritance, I assume you mean it has been deliberately crafted by host
code to have a prototype chain pointing into some Caja frame.

I think this is highly, highly unlikely, since it cannot happen except by pretty
inexplicably deliberate action on the part of a host page. Caja frames no longer
contain *any* host code at all; host code is only in the main frame and whatever
other frames it constructs. If host code injects stuff into the Caja frames, it
is doing something highly nonstandard and -- frankly -- wrong, and should expect
bad things to happen. :)

I am considering this a documentation issue. See last diff in
doc/html/introAndTaming/index.html.

http://codereview.appspot.com/2745043/diff/29001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:264: && f.TAMED_TWIN___.FERAL_TWIN___
On 2010/11/08 03:33:27, MarkM wrote:
> Since at this point you've already established that f is an object, and
> therefore truthy, you can omit this middle test since the following "=== f"
test
> makes it redundant.
> 
> As with the old code, the new code still deserves a comment explaining that
this
> backpointing check serves as a cheap hasOwnProp check.

Done.

[ihab.awad, 2010-11-18 00:23:44]

Noting additional feedback from MarkM:

1: Everywhere u expose wunderbar property names in the API, such as "f.XO4A___ =
true;", please instead expose API such as "something.makeXo4a(f)", which does
the same thing behind the scenes for now. I wanna preserve the option to change
our minds on how these taming decisions are represented in the object graph.

2: Although our internal catchall methods do distinguish method invocation
(x.m___) from getting the value of a property (x.v___), the actual Proxy api
does not.

3: Please look over my http://codereview.appspot.com/2225043/ before committing
and do what you easily can, without unduly slowing urself down.

[ihab.awad, 2010-11-19 06:33:07]

@4329