Re: Remove Cajita/Valija flag as an input to all cajoling services

[ihab...@gmail.com]

Mike Samuel, could you please look at the Domita parts of this?

Mike Stay agreed to look at everything else.

Thanks!

http://codereview.appspot.com/136053

[meta...@gmail.com]

LGTM. Please follow up with a message to the list that the API has
changed, and give examples.

http://codereview.appspot.com/136053/diff/7001/7039
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/136053/diff/7001/7039#newcode270
src/com/google/caja/cajita-module.js:270: // this should not happen
Throw an informative exception.

http://codereview.appspot.com/136053/diff/7001/7059
File src/com/google/caja/service/HtmlHandler.java (right):

http://codereview.appspot.com/136053/diff/7001/7059#newcode67
src/com/google/caja/service/HtmlHandler.java:67:
"http://caja.appspot.com/cajoler";
Good catch

http://codereview.appspot.com/136053/diff/7001/7007
File tests/com/google/caja/a.js (right):

http://codereview.appspot.com/136053/diff/7001/7007#newcode23
tests/com/google/caja/a.js:23: x + y
semicolon

http://codereview.appspot.com/136053/diff/7001/7022
File tests/com/google/caja/c.js (right):

http://codereview.appspot.com/136053/diff/7001/7022#newcode23
tests/com/google/caja/c.js:23: x + 1
semicolon

http://codereview.appspot.com/136053

[mikes...@gmail.com]

http://codereview.appspot.com/136053/diff/1/12
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/136053/diff/1/12#newcode211
tests/com/google/caja/plugin/domita_test.html:211:
loadScript('valija.co.js');
On 2009/10/30 06:17:42, ihab.awad wrote:
> On 2009/10/22 00:50:28, MikeSamuel wrote:
> > Why are we loading valija.co.js even when it's not valija?

> Because, even though the tests themselves are in a 'use cajita' block,
the
> enclosing HTML is, at the top level, always Valija. And that guff
looks for $v
> and what not, and fails if it's not there.

> The previous way we did this test -- by cajoling HTML as Cajita all
the way --
> was a special case that we achieved through our build rules; it was
not a sort
> of cajoling we expected any Caja users to actually attempt.

But shouldn't we test under cajita mode as a container probably would --
without loading the unnecessary valija stuff?

http://codereview.appspot.com/136053/diff/7001/7020
File tests/com/google/caja/opensocial/DefaultGadgetRewriterTest.java
(right):

http://codereview.appspot.com/136053/diff/7001/7020#newcode99
tests/com/google/caja/opensocial/DefaultGadgetRewriterTest.java:99: }
How is this test different from testSocialHelloWorld on line 101 now?

http://codereview.appspot.com/136053/diff/7001/7023
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/136053/diff/7001/7023#newcode315
tests/com/google/caja/plugin/domita_test.html:315: // TODO(mikesamuel):
rewrite XHR and setTimeout tests to use this scheme.
plz wrap or maybe deindent this entire script block body

http://codereview.appspot.com/136053/diff/7001/7023#newcode377
tests/com/google/caja/plugin/domita_test.html:377: 'assert',
'assertContains', 'assertEquals', 'assertEvaluatesToFalse',
plz wrap

http://codereview.appspot.com/136053/diff/7001/7023#newcode438
tests/com/google/caja/plugin/domita_test.html:438: return
'http://gadget-proxy/?url=' + encodeURIComponent(uri)
plz wrap

http://codereview.appspot.com/136053/diff/7001/7023#newcode448
tests/com/google/caja/plugin/domita_test.html:448: // Create a readonly
mirror of document so that we can test that mutations
plz wrap

http://codereview.appspot.com/136053/diff/7001/7026
File tests/com/google/caja/plugin/jsunit.js (right):

http://codereview.appspot.com/136053/diff/7001/7026#newcode57
tests/com/google/caja/plugin/jsunit.js:57: console.log('TEST ERROR: pass
without a test ID');
Maybe console.error instead of console.log

http://codereview.appspot.com/136053

[ihab...@gmail.com]

Other issues replied to via codereview; this one is only in email --

On Wed, Nov 11, 2009 at 2:46 PM, <mikes...@gmail.com> wrote:

But shouldn't we test under cajita mode as a container probably would --
without loading the unnecessary valija stuff?

You would think. That's what I tried at first. But it turns out that the Valija-ness of the enclosing HTML makes Valija startup guff get emitted, which fails ("$v undefined") if Valija is not loaded. So: even if all <script>s in an HTML file choose to be Cajita via "use subset" directives, the outermost thingey still requires Valija.

Ihab

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

[ihab...@gmail.com]

Snapshotted. See replies below.

http://codereview.appspot.com/136053/diff/7001/7039
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/136053/diff/7001/7039#newcode270
src/com/google/caja/cajita-module.js:270: // this should not happen

On 2009/11/11 20:25:25, metaweta wrote:
> Throw an informative exception.

Good point, done.

Not sure what an *informative* exception is here, but I threw something.
This will get bubbled up to the top-level <script> onload event, which
will probably drop it on the floor, but it might show up in a debugger
and we can see it if it happens in reality. Which it shouldn't. But....

http://codereview.appspot.com/136053/diff/7001/7059
File src/com/google/caja/service/HtmlHandler.java (right):

http://codereview.appspot.com/136053/diff/7001/7059#newcode67
src/com/google/caja/service/HtmlHandler.java:67:
"http://caja.appspot.com/cajoler";

On 2009/11/11 20:25:25, metaweta wrote:
> Good catch

We are _sporty_!

http://codereview.appspot.com/136053/diff/7001/7007
File tests/com/google/caja/a.js (right):

http://codereview.appspot.com/136053/diff/7001/7007#newcode23
tests/com/google/caja/a.js:23: x + y

On 2009/11/11 20:25:25, metaweta wrote:
> semicolon

Done.

http://codereview.appspot.com/136053/diff/7001/7022
File tests/com/google/caja/c.js (right):

http://codereview.appspot.com/136053/diff/7001/7022#newcode23
tests/com/google/caja/c.js:23: x + 1

On 2009/11/11 20:25:25, metaweta wrote:
> semicolon

Done.

http://codereview.appspot.com/136053/diff/7001/7020
File tests/com/google/caja/opensocial/DefaultGadgetRewriterTest.java
(right):

http://codereview.appspot.com/136053/diff/7001/7020#newcode99
tests/com/google/caja/opensocial/DefaultGadgetRewriterTest.java:99: }

On 2009/11/11 22:46:34, MikeSamuel wrote:
> How is this test different from testSocialHelloWorld on line 101 now?

It's not. Bobbitted.

http://codereview.appspot.com/136053/diff/7001/7023
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/136053/diff/7001/7023#newcode315
tests/com/google/caja/plugin/domita_test.html:315: // TODO(mikesamuel):
rewrite XHR and setTimeout tests to use this scheme.

On 2009/11/11 22:46:34, MikeSamuel wrote:
> plz wrap or maybe deindent this entire script block body

Went with the deindenting.

http://codereview.appspot.com/136053/diff/7001/7023#newcode377
tests/com/google/caja/plugin/domita_test.html:377: 'assert',
'assertContains', 'assertEquals', 'assertEvaluatesToFalse',

On 2009/11/11 22:46:34, MikeSamuel wrote:
> plz wrap

Done.

http://codereview.appspot.com/136053/diff/7001/7023#newcode438
tests/com/google/caja/plugin/domita_test.html:438: return
'http://gadget-proxy/?url=' + encodeURIComponent(uri)

On 2009/11/11 22:46:34, MikeSamuel wrote:
> plz wrap

Done.

http://codereview.appspot.com/136053/diff/7001/7023#newcode448
tests/com/google/caja/plugin/domita_test.html:448: // Create a readonly
mirror of document so that we can test that mutations

On 2009/11/11 22:46:34, MikeSamuel wrote:
> plz wrap

Done.

http://codereview.appspot.com/136053/diff/7001/7026
File tests/com/google/caja/plugin/jsunit.js (right):

http://codereview.appspot.com/136053/diff/7001/7026#newcode57
tests/com/google/caja/plugin/jsunit.js:57: console.log('TEST ERROR: pass
without a test ID');

On 2009/11/11 22:46:34, MikeSamuel wrote:
> Maybe console.error instead of console.log

Done.

http://codereview.appspot.com/136053

[mikes...@gmail.com]

I reviewed all the parts that Mike Stay didn't. Those parts LGTM

http://codereview.appspot.com/136053

[ihab...@gmail.com]

Per our conversation offline, I added an extra check to class Directive to not render if the directive string requires escaping. I also added a test in parsertest6.js / rendergolden6.txt. Would you like to take one last look? -- I

On Wed, Nov 11, 2009 at 7:29 PM, <mikes...@gmail.com> wrote:

I reviewed all the parts that Mike Stay didn't.   Those parts LGTM

http://codereview.appspot.com/136053

[mikes...@gmail.com]

LGTM


http://codereview.appspot.com/136053/diff/9157/9203
File src/com/google/caja/parser/js/Directive.java (right):

http://codereview.appspot.com/136053/diff/9157/9203#newcode103
src/com/google/caja/parser/js/Directive.java:103: // Escaping has
modified the directive. Render nothing.
maybe add a comment pointing to the longer discussion in the bug.

http://codereview.appspot.com/136053/diff/9157/9203#newcode107
src/com/google/caja/parser/js/Directive.java:107:
rc.getOut().consume(";");
cool

http://codereview.appspot.com/136053/diff/9157/9175
File tests/com/google/caja/parser/js/parsertest6.js (right):

http://codereview.appspot.com/136053/diff/9157/9175#newcode3
tests/com/google/caja/parser/js/parsertest6.js:3: "use f\u0008oo"; //
Should not be rendered
cool

http://codereview.appspot.com/136053

[ihab...@gmail.com]

@3846

http://codereview.appspot.com/136053