Fixes javascript URLs and inline event handlers

Modifies caja.js to copy the taming frame's plugin_dispatchToHandler___
and plugin_dispatchEvent___ functions into the guest frame (which is
necessary for inline event handlers like onclick="...") and into
the window containing the dom elements (which is necessary for
'javascript:' URLs).  Since we always use a single taming frame
even when there are multiple guest frames, this fix should work in
the presence of multiple cajoled pages.

[ihab.awad, 2011-06-02 17:20:58]

We do not *always* use a single taming frame; that is just the typical use case.
How hard would it be to make the functions dispatch to the correct taming frame?

[ihab.awad, 2011-06-02 17:22:57]

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

http://codereview.appspot.com/4530099/diff/1/src/com/google/caja/plugin/caja....
src/com/google/caja/plugin/caja.js:294: guestWindow.plugin_dispatchEvent___ =
Why assign to guest window as well?

[metaweta, 2011-06-02 17:33:35]

On 2011/06/02 17:20:58, ihab.awad wrote:
> We do not *always* use a single taming frame; that is just the typical use
case.
> How hard would it be to make the functions dispatch to the correct taming
frame?

A lot harder than this fix.  In what situation would we use more than one?

[metaweta, 2011-06-02 17:33:50]

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

http://codereview.appspot.com/4530099/diff/1/src/com/google/caja/plugin/caja....
src/com/google/caja/plugin/caja.js:294: guestWindow.plugin_dispatchEvent___ =
On 2011/06/02 17:22:57, ihab.awad wrote:
> Why assign to guest window as well?

It's needed when triggering event handlers.

[ihab.awad, 2011-06-02 17:49:13]

On Thu, Jun 2, 2011 at 10:33 AM, <metaweta@gmail.com> wrote:

> A lot harder than this fix.  In what situation would we use more than
> one?


When we want to run multiple taming frames to establish independent module
contexts. In other words, our system is by design not a per-host-frame
singleton. For example, while using gadgets from distinct sources which may
have a different interpretation of what "jquery.js" should be -- like
different versions of the same library. There are two actual use cases:

(a) It allows Caja to be more of a feature-complete in-page "operating
system" supporting multiple independent contexts; but, more practically:

(b) It provides an "exit hatch" for any thorny unforeseen problem -- in
testing or production -- that can be solved by just creating "two of
everything" and being done with it. :)

The question of whether this is *necessary* is a good one. We've talked
about it before, but I don't think we've come to a conclusion yet. I think
the reason is that, beyond abstract API issues (do you call caja.configure
and return a frameGroup, or do you just configure globally, etc etc), our
hand has yet to be forced.

How hard *is* it really to preserve the non-singleton nature? Could we come
up with a quick little sketch of the simplest possible non-singleton
solution and evaluate it?

Ihab

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

[metaweta, 2011-06-02 18:30:06]

On 2011/06/02 17:49:13, ihab.awad wrote:
> How hard *is* it really to preserve the non-singleton nature? Could we come
> up with a quick little sketch of the simplest possible non-singleton
> solution and evaluate it?

Well, we already have the plugin_id parameter and a lookup table.  The problem
is that each instance of domita creates its own lookup table.  Whatever solution
we come up with needs to define a single lookup table for the hostpage.

[ihab.awad, 2011-06-02 18:32:51]

On Thu, Jun 2, 2011 at 11:30 AM, <metaweta@gmail.com> wrote:

> Well, we already have the plugin_id parameter and a lookup table.  The
> problem is that each instance of domita creates its own lookup table.
> Whatever solution we come up with needs to define a single lookup table
> for the hostpage.


So maybe if we ensure that plugin_id is unique in the entire host page, then
we can dispatch correctly? I guess that just requires some table in caja.js
that the host-page plugin_dispatch* would close over and use to figure out
which guest frame to talk to? Doesn't sound too bad.... Am I missing some
complication?

Ihab

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

[metaweta, 2011-06-02 18:41:57]

On 2011/06/02 18:32:51, ihab.awad wrote:
> On Thu, Jun 2, 2011 at 11:30 AM,
<https://mail.google.com/mail?view=cm&tf=0&ui=1&to=metaweta@gmail.com> wrote:
> 
> > Well, we already have the plugin_id parameter and a lookup table.  The
> > problem is that each instance of domita creates its own lookup table.
> > Whatever solution we come up with needs to define a single lookup table
> > for the hostpage.
> 
> 
> So maybe if we ensure that plugin_id is unique in the entire host page, then
> we can dispatch correctly? I guess that just requires some table in caja.js
> that the host-page plugin_dispatch* would close over and use to figure out
> which guest frame to talk to? Doesn't sound too bad.... Am I missing some
> complication?

So rather than expose plugin_* from the taming frame directly, there'll be two
layers of dispatch: the hostpage looks up the taming frame from the plugin id
and then invokes the plugin_dispatchToHandler___ from that taming frame.  That
sounds rather slow, but straightforward to implement.

[ihab.awad, 2011-06-02 18:44:09]

On Thu, Jun 2, 2011 at 11:41 AM, <metaweta@gmail.com> wrote:

> So rather than expose plugin_* from the taming frame directly, there'll
> be two layers of dispatch: the hostpage looks up the taming frame from
> the plugin id and then invokes the plugin_dispatchToHandler___ from that
> taming frame.  That sounds rather slow, but straightforward to
> implement.


Sounds good. But why slow btw?

  var targetFrame = targetFrames[pluginId];

Ihab

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

[metaweta, 2011-06-02 18:57:57]

On 2011/06/02 18:44:09, ihab.awad wrote:
> On Thu, Jun 2, 2011 at 11:41 AM,
<https://mail.google.com/mail?view=cm&tf=0&ui=1&to=metaweta@gmail.com> wrote:
> 
> > So rather than expose plugin_* from the taming frame directly, there'll
> > be two layers of dispatch: the hostpage looks up the taming frame from
> > the plugin id and then invokes the plugin_dispatchToHandler___ from that
> > taming frame.  That sounds rather slow, but straightforward to
> > implement.
> 
> 
> Sounds good. But why slow btw?
> 
>   var targetFrame = targetFrames[pluginId];


Well, more like

function plugin_dispatchEvent___(thisNode, event, pluginId, handler) {
   return targetFrames[pluginId].contentWindow.plugin_dispatchEvent___(thisNode,
event, pluginId, handler);
}

But this is in response to a user action, not in a tight loop, so it probably
doesn't matter.

[MikeSamuel, 2011-06-02 20:13:27]

Is there already something which maps from a module id to a frame for
that module id?

[metaweta, 2011-06-02 20:22:49]

On Thu, Jun 2, 2011 at 1:13 PM, Mike Samuel <mikesamuel@gmail.com> wrote:
> Is there already something which maps from a module id to a frame for
> that module id?

Not that I know of.  The pluginId is currently just a counter per
taming frame, so we'd need to change that to generate a unique id.
-- 
Mike Stay - metaweta@gmail.com
http://www.cs.auckland.ac.nz/~mike
http://reperiendi.wordpress.com

[ihab.awad, 2011-06-02 20:46:16]

On Thu, Jun 2, 2011 at 1:18 PM, Mike Stay <metaweta@gmail.com> wrote:

> Not that I know of.  The pluginId is currently just a counter per
> taming frame, so we'd need to change that to generate a unique id.
>

Right, just hoisting the counter up one level would be fine.

Ihab

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

[MikeSamuel, 2011-06-02 21:03:53]

2011/6/2  <ihab.awad@gmail.com>:
>
> On Thu, Jun 2, 2011 at 1:18 PM, Mike Stay <metaweta@gmail.com> wrote:
>>
>> Not that I know of.  The pluginId is currently just a counter per
>> taming frame, so we'd need to change that to generate a unique id.
>
> Right, just hoisting the counter up one level would be fine.

And we can't do dispatch of events to the proper frame without some
kind of top frame coordination.

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

[ihab.awad, 2011-06-07 01:46:37]

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

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:1949: id = imports.id___ = +('' +
Math.random()).substr(2);
Doesn't this change mean that IDs could collide?

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

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:292: imports.id___ = +('' +
Math.random()).substr(2);
Why not call ___.getId(imports) to cause an ID to be attached? Also, same
question re collisions as with "getId" in es53.js.

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:304: tamingWindow.plugin_dispatchEvent___;
I suspect that guestWindow.plugin_* is unused, in which case they should be
removed entirely.

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:305: divWindow.plugin_dispatchToHandler___ =
Don't we also need a divWindow.plugin_dispatchEvent___ ?

[metaweta, 2011-06-07 02:56:22]

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

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/es53.js#...
src/com/google/caja/es53.js:1949: id = imports.id___ = +('' +
Math.random()).substr(2);
On 2011/06/07 01:46:37, ihab.awad wrote:
> Doesn't this change mean that IDs could collide?

Yes, but with very low probability.

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

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:292: imports.id___ = +('' +
Math.random()).substr(2);
On 2011/06/07 01:46:37, ihab.awad wrote:
> Why not call ___.getId(imports) to cause an ID to be attached? Also, same
> question re collisions as with "getId" in es53.js.

There's no getId on the (fake) ___ object on the divWindow.  If I call it on the
tamingWindow using the old getId definition, then it will return a count of
plugins using *that window*, which will certainly collide if I have two taming
windows.

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:304: tamingWindow.plugin_dispatchEvent___;
On 2011/06/07 01:46:37, ihab.awad wrote:
> I suspect that guestWindow.plugin_* is unused, in which case they should be
> removed entirely.

No, on* event handlers are executed in the context of the guestWindow, so we
need dispatchEvent in the guest window.  javascript URLs are evaluated in the
context of the divWindow, so we need dispatchToHandler there.

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:305: divWindow.plugin_dispatchToHandler___ =
On 2011/06/07 01:46:37, ihab.awad wrote:
> Don't we also need a divWindow.plugin_dispatchEvent___ ?

No, see above.

[ihab.awad, 2011-06-07 03:53:07]

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

http://codereview.appspot.com/4530099/diff/13001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:292: imports.id___ = +('' +
Math.random()).substr(2);
So the problem is that the ___.getId() in any given taming frame does not return
a unique value? Ok, then how about removing ___.getId() and ___.getImports()
*entirely* from es53.js? Then, when caja.js initializes a taming frame, it
*injects* a reference to  a ___.getId() and a ___.getImports() provided by
caja.js, which allocate *globally* unique references.

In that way, caja.js can also maintain a table to remember which frame a given
set of imports came from as well, and use that to advantage.

would _that_ work?

[ihab.awad, 2011-06-10 03:43:35]

lgtm