Fix global 'window' for ES53 case.

[metaweta, 2011-03-02 21:27:03]

LGTM modulo some nits.

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

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js#newc...
src/com/google/caja/es53.js:4786: delete extraImports.window;
Why?

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
src/com/google/caja/plugin/domita.js:3651: classUtils.whitelistES53Value(this,
name);
Where is this defined?

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

http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html:52: //
checkWindow();
Why declare it then?

[ihab.awad, 2011-03-02 22:15:47]

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

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js#newc...
src/com/google/caja/es53.js:4786: delete extraImports.window;
On 2011/03/02 21:27:03, metaweta wrote:
> Why?

Because TameWindow does not allow setting the 'window' property on itself; that
(and other boilerplate like 'opener') are hard-coded as non configurable by
domita.js.

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
src/com/google/caja/plugin/domita.js:3651: classUtils.whitelistES53Value(this,
name);
On 2011/03/02 21:27:03, metaweta wrote:
> Where is this defined?

Dead code. Bobbitted.

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

http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html:52: //
checkWindow();
On 2011/03/02 21:27:03, metaweta wrote:
> Why declare it then?

More undeadness.

[metaweta, 2011-03-02 22:30:15]

On 2011/03/02 22:15:47, ihab.awad wrote:
> http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js
> File src/com/google/caja/es53.js (right):
> 
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js#newc...
> src/com/google/caja/es53.js:4786: delete extraImports.window;
> On 2011/03/02 21:27:03, metaweta wrote:
> > Why?
> 
> Because TameWindow does not allow setting the 'window' property on itself;
that
> (and other boilerplate like 'opener') are hard-coded as non configurable by
> domita.js.

Really?  I could understand that perhaps being the case for cajita, but in ES53,
the 'window' property of the global scope should start out pointing to the
global scope and should be able to be reassigned to anything else.  Maybe it
would make sense to have the globals for es53 be an object that inherits from
imports?

> 
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
> File src/com/google/caja/plugin/domita.js (right):
> 
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
> src/com/google/caja/plugin/domita.js:3651: classUtils.whitelistES53Value(this,
> name);
> On 2011/03/02 21:27:03, metaweta wrote:
> > Where is this defined?
> 
> Dead code. Bobbitted.
> 
>
http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
> File tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html
> (right):
> 
>
http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
> tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html:52: //
> checkWindow();
> On 2011/03/02 21:27:03, metaweta wrote:
> > Why declare it then?
> 
> More undeadness.

[ihab.awad, 2011-03-02 22:34:51]

Perhaps ... let's just check this one in for now though. -- I

On Wed, Mar 2, 2011 at 2:30 PM, <metaweta@gmail.com> wrote:

> On 2011/03/02 22:15:47, ihab.awad wrote:
>
> http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js
>
>> File src/com/google/caja/es53.js (right):
>>
>
>
>
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/es53.js#newc...
>
>> src/com/google/caja/es53.js:4786: delete extraImports.window;
>> On 2011/03/02 21:27:03, metaweta wrote:
>> > Why?
>>
>
>  Because TameWindow does not allow setting the 'window' property on
>>
> itself; that
>
>> (and other boilerplate like 'opener') are hard-coded as non
>>
> configurable by
>
>> domita.js.
>>
>
> Really?  I could understand that perhaps being the case for cajita, but
> in ES53, the 'window' property of the global scope should start out
> pointing to the global scope and should be able to be reassigned to
> anything else.  Maybe it would make sense to have the globals for es53
> be an object that inherits from imports?
>
>
>
>
>
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
>
>> File src/com/google/caja/plugin/domita.js (right):
>>
>
>
>
>
http://codereview.appspot.com/4252050/diff/1/src/com/google/caja/plugin/domit...
>
>> src/com/google/caja/plugin/domita.js:3651:
>>
> classUtils.whitelistES53Value(this,
>
>> name);
>> On 2011/03/02 21:27:03, metaweta wrote:
>> > Where is this defined?
>>
>
>  Dead code. Bobbitted.
>>
>
>
>
>
http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
>
>> File
>>
> tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html
>
>> (right):
>>
>
>
>
>
http://codereview.appspot.com/4252050/diff/1/tests/com/google/caja/plugin/es5...
>
> tests/com/google/caja/plugin/es53-test-basic-functions-cajoled.html:52:
> //
>
>> checkWindow();
>> On 2011/03/02 21:27:03, metaweta wrote:
>> > Why declare it then?
>>
>
>  More undeadness.
>>
>
>
>
> http://codereview.appspot.com/4252050/
>



-- 
Ihab A.B. Awad, Palo Alto, CA

[ihab.awad, 2011-03-02 22:36:08]

@4384