Fix for issue 1067.

If a property is marked canRead, then canReadPub should only
return true if the property actually exists on the object.

[MarkM, 2009-07-14 00:00:03]

LGTM, but see my question.

http://codereview.appspot.com/93041/diff/2002/2004
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/93041/diff/2002/2004#newcode1373
Line 1373: if (obj[name + '_canRead___']) { return (name in obj); }
Why not do it by leaving the original line alone and putting

    if (!(name in obj)) { return false; }

on the previous line? I think it would be observationally equivalent to what you
wrote, but a bit more in the style of the rest of the access checking functions.

[metaweta, 2009-07-14 00:08:18]

On 2009/07/14 00:00:03, MarkM wrote:
> LGTM, but see my question.
> 
> http://codereview.appspot.com/93041/diff/2002/2004
> File src/com/google/caja/cajita.js (right):
> 
> http://codereview.appspot.com/93041/diff/2002/2004#newcode1373
> Line 1373: if (obj[name + '_canRead___']) { return (name in obj); }
> Why not do it by leaving the original line alone and putting
> 
>     if (!(name in obj)) { return false; }
> 
> on the previous line? I think it would be observationally equivalent to what
you
> wrote, but a bit more in the style of the rest of the access checking
functions.

Well, for some reason it isn't the same.  If I write it as you suggest here,
many tests fail, whereas they don't if I do it as in the CL.  Feel free to look
it over in more depth if you feel it's important.

[metaweta, 2009-07-14 17:06:07]

The expression "name in obj" fails when obj is a string.

[metaweta, 2009-07-14 17:10:33]

Should I add "obj = Object(obj)" to canReadPub along with your suggested change,
or should I just submit the patch?

[metaweta, 2009-07-14 18:56:27]

Snapshotted.  This also fixes issue 1076 by testing in inPub whether obj is of
type object or function (i.e. that it's allowed on the right of an "in"
expression).  Also added tests for the various bugs that turned up during this
exploration.

[MarkM, 2009-07-14 19:14:35]

LGTM modulo comments

http://codereview.appspot.com/93041/diff/2012/3009
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/93041/diff/2012/3009#newcode1394
Line 1394: if (obj === null || obj === void 0 ||
"|| obj === void 0" is unnecessary given the rest of the conditional.

http://codereview.appspot.com/93041/diff/2012/3008
File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java (right):

http://codereview.appspot.com/93041/diff/2012/3008#newcode115
Line 115: public void testIn() throws Exception {
Why not promote this to CommonJsRewriterTestCase?

http://codereview.appspot.com/93041/diff/2012/3008#newcode913
Line 913: assertConsistent(
Why not promote this to CommonJsRewriterTestCase?

http://codereview.appspot.com/93041/diff/2012/3007
File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
(right):

http://codereview.appspot.com/93041/diff/2012/3007#newcode52
Line 52: public void testStringLength() throws Exception {
Why not promote this to CommonJsRewriterTestCase?

[metaweta, 2009-07-14 19:57:10]

On 2009/07/14 19:14:35, MarkM wrote:
> LGTM modulo comments
> 
> http://codereview.appspot.com/93041/diff/2012/3009
> File src/com/google/caja/cajita.js (right):
> 
> http://codereview.appspot.com/93041/diff/2012/3009#newcode1394
> Line 1394: if (obj === null || obj === void 0 ||
> "|| obj === void 0" is unnecessary given the rest of the conditional.

Removed.

> 
> http://codereview.appspot.com/93041/diff/2012/3008
> File tests/com/google/caja/parser/quasiliteral/CajitaRewriterTest.java
(right):
> 
> http://codereview.appspot.com/93041/diff/2012/3008#newcode115
> Line 115: public void testIn() throws Exception {
> Why not promote this to CommonJsRewriterTestCase?
> 
> http://codereview.appspot.com/93041/diff/2012/3008#newcode913
> Line 913: assertConsistent(
> Why not promote this to CommonJsRewriterTestCase?
> 
> http://codereview.appspot.com/93041/diff/2012/3007
> File tests/com/google/caja/parser/quasiliteral/DefaultValijaRewriterTest.java
> (right):
> 
> http://codereview.appspot.com/93041/diff/2012/3007#newcode52
> Line 52: public void testStringLength() throws Exception {
> Why not promote this to CommonJsRewriterTestCase?

Done.

Also changed valija-cajita.js to avoid TypeErrors around the use of "in". 
Please look those over.

[MarkM, 2009-07-14 22:37:44]

LGTM