Amend caja.js API to support the four methods of providing cajoled code.

Changes caja.js to support
- telling the cajoling service to fetch and cajole content at a url
- fetching cached, already cajoled code at a url
- posting uncajoled content to the cajoling service
- when the cajoler is part of the web server, running cajoled code
  provided as a string in the page.

[ihab.awad, 2011-03-14 21:31:13]

Partial comments; to discuss via chat with Mike Stay.

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

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/es53.js#n...
src/com/google/caja/es53.js:3136: * Function calls g(args) get translated to
g.f___(___.USELESS, args)
Change comment to reflect actual code. :)

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:264: innerContainer.setAttribute('class',
'caja_innerContainer___');
Long whitespace at the end of this line; line should be unchanged.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:269:
innerContainer.appendChild(div.childNodes[0]);
Does this not *reverse* the order of the nodes as they are added?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:288:
tamingWindow.plugin_dispatchToHandler___;
Hm -- how did our stuff work *before* this change?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:295: *     cajoled content.  The JSON object
has the following properties:
Perhaps outdent this line to make the comment readable?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:302: *         contains the uncajoled html to
be sent to the cajoler.
The original API also supported passing just JavaScript ... we would want to
maintain this ability.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:319: // then invoke run with the result
Throw "unsupported ${something}" here.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:324: callback(moduleFunc(imports));
Should be changed to an "opt_callback" to be consistent with "runNow".

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:329: // fetch the cajoled content and invoke
run with the result.
Throw "unsupported ${something}" here.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:336: * @param descriptor a JSON object
describing the cajoled content.
It seems to me it would be easiest for users of the run* functions if:

* The descriptor schema were precisely the same for both cases; and

* The only difference between "run" and "runNow" is that "runNow" accepts only
the synchronously satisfiable subset of descriptor contents.

Do you agree that it's desirable to arrange things that way?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:346: function runNow(descriptor,
extraImports, opt_callback) {
It seems there's a lot of code duplication between "run" and "runNow" ... it
feels like they should both call some common function.

[metaweta, 2011-03-23 01:25:09]

Please check to see that I'm using rawAtUrl and cajoledAtUrl in the right
places.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:264: innerContainer.setAttribute('class',
'caja_innerContainer___');
On 2011/03/14 21:31:13, ihab.awad wrote:
> Long whitespace at the end of this line; line should be unchanged.

What do you mean?  Leave the whitespace there?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:269:
innerContainer.appendChild(div.childNodes[0]);
On 2011/03/14 21:31:13, ihab.awad wrote:
> Does this not *reverse* the order of the nodes as they are added?

No.  appendChild(node) places the node at the end of the list of childNodes, not
the start.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:288:
tamingWindow.plugin_dispatchToHandler___;
On 2011/03/14 21:31:13, ihab.awad wrote:
> Hm -- how did our stuff work *before* this change?

It didn't (at least, not when domita was attached only to the taming frame). 
That's why I changed it.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:295: *     cajoled content.  The JSON object
has the following properties:
On 2011/03/14 21:31:13, ihab.awad wrote:
> Perhaps outdent this line to make the comment readable?

Obsoleted with builder.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:302: *         contains the uncajoled html to
be sent to the cajoler.
On 2011/03/14 21:31:13, ihab.awad wrote:
> The original API also supported passing just JavaScript ... we would want to
> maintain this ability.

You mean, as though it were passed in a script block?

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:319: // then invoke run with the result
On 2011/03/14 21:31:13, ihab.awad wrote:
> Throw "unsupported ${something}" here.

Done.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:324: callback(moduleFunc(imports));
On 2011/03/14 21:31:13, ihab.awad wrote:
> Should be changed to an "opt_callback" to be consistent with "runNow".

Done.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:329: // fetch the cajoled content and invoke
run with the result.
On 2011/03/14 21:31:13, ihab.awad wrote:
> Throw "unsupported ${something}" here.

Done.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:336: * @param descriptor a JSON object
describing the cajoled content.
On 2011/03/14 21:31:13, ihab.awad wrote:
> It seems to me it would be easiest for users of the run* functions if:
> 
> * The descriptor schema were precisely the same for both cases; and
> 
> * The only difference between "run" and "runNow" is that "runNow" accepts only
> the synchronously satisfiable subset of descriptor contents.
> 
> Do you agree that it's desirable to arrange things that way?

Done.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:346: function runNow(descriptor,
extraImports, opt_callback) {
On 2011/03/14 21:31:13, ihab.awad wrote:
> It seems there's a lot of code duplication between "run" and "runNow" ... it
> feels like they should both call some common function.

Done.

[ihab.awad, 2011-03-23 18:35:24]

Very close.

Is there a way to create test cases for the 'cajoled' case? Since you're remote,
I think it's ok to check in as-is and I'll add a test case myself if you want
right afterwards.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:264: innerContainer.setAttribute('class',
'caja_innerContainer___');
On 2011/03/23 01:25:09, metaweta wrote:
> On 2011/03/14 21:31:13, ihab.awad wrote:
> > Long whitespace at the end of this line; line should be unchanged.
> 
> What do you mean?  Leave the whitespace there?

Oh -- it just seemed like the line was showing up as a diff even though there is
not different in content, so I expected it must have been a whitespace change.
No biggie, but it does seem like there's extra whitespace at the end of the
line.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:269:
innerContainer.appendChild(div.childNodes[0]);
On 2011/03/23 01:25:09, metaweta wrote:
> On 2011/03/14 21:31:13, ihab.awad wrote:
> > Does this not *reverse* the order of the nodes as they are added?
> 
> No.  appendChild(node) places the node at the end of the list of childNodes,
not
> the start.

Geflurpft. You're right.

http://codereview.appspot.com/4211045/diff/9015/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:302: *         contains the uncajoled html to
be sent to the cajoler.
On 2011/03/23 01:25:09, metaweta wrote:
> On 2011/03/14 21:31:13, ihab.awad wrote:
> > The original API also supported passing just JavaScript ... we would want to
> > maintain this ability.
> 
> You mean, as though it were passed in a script block?

Never mind; see comments in next rev.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:295: *     cajoled content.  The JSON object
has the following properties:
So fwiw -- given that we've implemented this, it looks to me like there is no
need for a Builder _per se_ since there is no use case for a fluent interface
(aBuilder.withFoo().andBar().butBaz().exceptBoo()...). The calls are always only
one deep. So maybe cajoledAtUrl(), cajoled(), and friends should just be
functions on the frame. The could return a run()-able, or they could incorporate
the parameters of run() in their own interface -- whichever. Does this make
sense?

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:305: * @param callback a function that is
called providing the completion
What is 'src' for?

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:330: }
What is 'src' for?

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:382: callback({
Isn't this the 'rawAtUrl' case? loader.async(url) will go fetch the (raw)
content via a cajoling service *and cajole* it.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:396: 
What happens if a client calls builder() twice, diddles with both of them, then
calls run() on one builder, then calls run() on the second? By my
interpretation, since the variable 'run___' is state in the builder, not in the
frame itself, caja.js would attempt to run two modules in the same frame.

http://codereview.appspot.com/4211045/diff/25001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/default-test-driver.js (right):

http://codereview.appspot.com/4211045/diff/25001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/default-test-driver.js:27: .cajoledAtUrl(testCase)
Happiness prevaileth.

[metaweta, 2011-03-23 23:58:45]

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:295: Builder.prototype = {
On 2011/03/23 18:35:24, ihab.awad wrote:
> So fwiw -- given that we've implemented this, it looks to me like there is no
> need for a Builder _per se_ since there is no use case for a fluent interface
> (aBuilder.withFoo().andBar().butBaz().exceptBoo()...). The calls are always
only
> one deep. So maybe cajoledAtUrl(), cajoled(), and friends should just be
> functions on the frame. The could return a run()-able, or they could
incorporate
> the parameters of run() in their own interface -- whichever. Does this make
> sense?

Totally, although I can imagine we might want to do something in the future like
"don't allow this thing network access even if it tries to dynamically cajole
stuff that isn't in the cache".  This API seems somewhat friendlier to future
extension.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:305: cajoled: function(src, cajoledJs,
opt_staticHtml) {
On 2011/03/23 18:35:24, ihab.awad wrote:
> What is 'src' for?

Ostensibly for setting the base URL, but I'm not sure how to pass that to the
module.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:330: raw: function(src, html) {
On 2011/03/23 18:35:24, ihab.awad wrote:
> What is 'src' for?

Again, the base URL.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:382: } else if ('cajoledAtUrl' ===
this.case___){
On 2011/03/23 18:35:24, ihab.awad wrote:
> Isn't this the 'rawAtUrl' case? loader.async(url) will go fetch the (raw)
> content via a cajoling service *and cajole* it.

Done.

http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:396: return new Builder();
On 2011/03/23 18:35:24, ihab.awad wrote:
> What happens if a client calls builder() twice, diddles with both of them,
then
> calls run() on one builder, then calls run() on the second? By my
> interpretation, since the variable 'run___' is state in the builder, not in
the
> frame itself, caja.js would attempt to run two modules in the same frame.

Fixed.

http://codereview.appspot.com/4211045/diff/25001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/default-test-driver.js (right):

http://codereview.appspot.com/4211045/diff/25001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/default-test-driver.js:27: .cajoledAtUrl(testCase)
On 2011/03/23 18:35:24, ihab.awad wrote:
> Happiness prevaileth.

Except now, cajoledAtUrl is unimplemented.  Can you point me at a code snippet
using the module loader API for that case?

[metaweta, 2011-03-24 00:14:32]

On 2011/03/23 23:58:45, metaweta wrote:
>
http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
> File src/com/google/caja/plugin/caja.js (right):
> 
>
http://codereview.appspot.com/4211045/diff/25001/src/com/google/caja/plugin/c...
> src/com/google/caja/plugin/caja.js:295: Builder.prototype = {
> On 2011/03/23 18:35:24, ihab.awad wrote:
> > So fwiw -- given that we've implemented this, it looks to me like there is
no
> > need for a Builder _per se_ since there is no use case for a fluent
interface
> > (aBuilder.withFoo().andBar().butBaz().exceptBoo()...). The calls are always
> only
> > one deep. So maybe cajoledAtUrl(), cajoled(), and friends should just be
> > functions on the frame. The could return a run()-able, or they could
> incorporate
> > the parameters of run() in their own interface -- whichever. Does this make
> > sense?
> 
> Totally, although I can imagine we might want to do something in the future
like
> "don't allow this thing network access even if it tries to dynamically cajole
> stuff that isn't in the cache".  This API seems somewhat friendlier to future
> extension.

I could also do something like

frame.url(myUrl).run(...) // defaults to cajoled = false
frame.url(myUrl).cajoled().run(...)
frame.src(myBaseUrl).html(...).run(...)
frame.src(myBaseUrl).cajoled().content(js, opt_html).run(...)

[ihab.awad, 2011-03-24 03:45:20]

lgtm++; looks great!

There is one remaining issue: cajoled() versus raw(). It seems that "cajoled"
means, the stuff I am giving the API is cajoled. "raw" means, the stuff I am
giving the API is un-cajoled. The current usage seems not to fit that.