Issue 1812042: Shift to loading the Cajita runtime into an iframe rather than the host page.

Issue 1812042: Shift to loading the Cajita runtime into an iframe rather than the host page. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 11 months ago by kpreid2

Modified:
15 years, 11 months ago

Reviewers:
Jasvir, ihab.awad

CC:
google-caja-discuss_googlegroups.com

Base URL:
http://google-caja.googlecode.com/svn/trunk/

Visibility:
Public.

Description

Added file caja.js which creates an iframe, then loads everything from Cajita to HostTools into that frame, and calls a callback with all that stuff so the host page can load gadgets and grant access to the host DOM. Changed the container/ demos to use it. Does not update the playground or any other demos.

Patch Set 1 #

Total comments: 43

Created: 15 years, 11 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+495 lines, -265 lines)			Patch
M	build.xml	View	6 chunks	+61 lines, -12 lines	2 comments	Download
D	src/com/google/caja/demos/container/container-dynamic.html	View	1 chunk	+0 lines, -70 lines	0 comments	Download
M	src/com/google/caja/demos/container/container-interaction.html	View	1 chunk	+79 lines, -135 lines	15 comments	Download
M	src/com/google/caja/demos/container/container-trivial.html	View	2 chunks	+21 lines, -40 lines	2 comments	Download
D	src/com/google/caja/demos/container/gadget-library.html	View	1 chunk	+0 lines, -5 lines	0 comments	Download
A +	src/com/google/caja/demos/container/gadget-library.js	View	1 chunk	+0 lines, -2 lines	0 comments	Download
A	src/com/google/caja/plugin/caja.js	View	1 chunk	+136 lines, -0 lines	14 comments	Download
A	src/com/google/caja/plugin/host-iframe-final.js	View	1 chunk	+4 lines, -0 lines	2 comments	Download
M	src/com/google/caja/plugin/host-tools.js	View	1 chunk	+2 lines, -1 line	0 comments	Download
A	tests/com/google/caja/plugin/HostIframeTest.java	View	1 chunk	+51 lines, -0 lines	0 comments	Download
A	tests/com/google/caja/plugin/host-iframe-test.html	View	1 chunk	+39 lines, -0 lines	0 comments	Download
A	tests/com/google/caja/plugin/host-iframe-test.js	View	1 chunk	+102 lines, -0 lines	8 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

ihab.awad

LGTM++ http://codereview.appspot.com/1812042/diff/1/13 File build.xml (right): http://codereview.appspot.com/1812042/diff/1/13#newcode1158 build.xml:1158: <include name="host-iframe.html"/> Is "host-iframe.html" here correct? Where does ...

15 years, 11 months ago (2010-07-14 04:11:44 UTC) #2

kpreid2

http://codereview.appspot.com/1812042/diff/1/13 File build.xml (right): http://codereview.appspot.com/1812042/diff/1/13#newcode1158 build.xml:1158: <include name="host-iframe.html"/> On 2010/07/14 04:11:44, ihab.awad wrote: > Is ...

15 years, 11 months ago (2010-07-14 17:44:50 UTC) #3

http://codereview.appspot.com/1812042/diff/1/13
File build.xml (right):

http://codereview.appspot.com/1812042/diff/1/13#newcode1158
build.xml:1158: <include name="host-iframe.html"/>
On 2010/07/14 04:11:44, ihab.awad wrote:
> Is "host-iframe.html" here correct? Where does this come from?

Good catch. Used to be the file loaded into the iframe, before I started
constructing it from caja.js instead.

http://codereview.appspot.com/1812042/diff/1/9
File src/com/google/caja/demos/container/container-interaction.html (right):

http://codereview.appspot.com/1812042/diff/1/9#newcode27
src/com/google/caja/demos/container/container-interaction.html:27: <link
rel="stylesheet" href="caja-gadget.css" type="text/css">
On 2010/07/14 04:11:44, ihab.awad wrote:
> Can these CSS rules be injected by "caja.js" (perhaps even by being explicitly
> set as style="..." attributes on the gadget <DIV>s)? It would be one less
thing
> for a page author to include....

Good idea for future work.

http://codereview.appspot.com/1812042/diff/1/9#newcode47
src/com/google/caja/demos/container/container-interaction.html:47: };
On 2010/07/14 04:11:44, ihab.awad wrote:
> Outdent above 3 lines 2 spaces.

Done.

http://codereview.appspot.com/1812042/diff/1/9#newcode51
src/com/google/caja/demos/container/container-interaction.html:51: var libP =
caja.hostTools.getLoad().async("gadget-library.js");
On 2010/07/14 04:11:44, ihab.awad wrote:
> How about making gadget-library.js do something (even if it's trivial), and
> naming it based on what it does? When I saw the name "gadget-library.js", I
> was like, oh wait, this is a Caja infrastructure library that someone must 
> load in order to use gadgets? It took me a while to realize that no, this is
> an example library that the *user* (developer) would provide to their
> gadgets....

The interacting modules example was already like this; this is a good idea for a
separate change.

http://codereview.appspot.com/1812042/diff/1/9#newcode72
src/com/google/caja/demos/container/container-interaction.html:72:
caja.cajita.setPub(sandbox.imports.outers, p, extraOuters[p]);
On 2010/07/14 04:11:44, ihab.awad wrote:
> If sandbox.attach(...) is what initializes Domita, then if you add the extra
> outers to "imports" *before* you initialize Domita, then Cajita and Valija
code
> will see the same top-level symbols. This is probably a Good Thing [TM] to
> encourage so as to avoid future confusion.

Done.

This seems like unduly surprising behavior, especially from the perspective of a
HostTools (not attachDocumentStub) user. Can we have Domita do something
ordering-independent? If not, we need to explain this behavior on
<http://code.google.com/p/google-caja/wiki/HostTools>.

http://codereview.appspot.com/1812042/diff/1/9#newcode88
src/com/google/caja/demos/container/container-interaction.html:88: 
On 2010/07/14 04:11:44, ihab.awad wrote:
> Remove blank line.

Done.

http://codereview.appspot.com/1812042/diff/1/9#newcode92
src/com/google/caja/demos/container/container-interaction.html:92: <body
onload="loadCaja(loadGadgets, {debug:true,cajaServer:
'http://localhost:8080/'})">
On 2010/07/14 04:11:44, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/1812042/diff/1/9#newcode96
src/com/google/caja/demos/container/container-interaction.html:96: <p>This host
page loads two gadgets (which should appear in the table below) and provides the
left gadget a function "set", and the right gadget "get", by which they can
communicate.</p>
On 2010/07/14 04:11:44, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/1812042/diff/1/12
File src/com/google/caja/demos/container/container-trivial.html (right):

http://codereview.appspot.com/1812042/diff/1/12#newcode26
src/com/google/caja/demos/container/container-trivial.html:26: <script
src="http://localhost:8080/caja.js"
On 2010/07/14 04:11:44, ihab.awad wrote:
> ...

Necessary for testing, will be fixed before commit and we bother Jasvir to
update the playground.

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

http://codereview.appspot.com/1812042/diff/1/6#newcode33
src/com/google/caja/plugin/caja.js:33: * The loading in the iframe is (almost)
necessarily asynchronouse, so you
On 2010/07/14 04:11:44, ihab.awad wrote:
> typo: asynchronous

Done.

http://codereview.appspot.com/1812042/diff/1/6#newcode36
src/com/google/caja/plugin/caja.js:36: * instance which has been preconfigured
to work with the right base URL.
On 2010/07/14 04:11:44, ihab.awad wrote:
> s/the right base URL/the base URL of the owning document/
> 
> Also, perhaps provide a pointer to where the reader can read about the API of
> HostTools.

Done.

http://codereview.appspot.com/1812042/diff/1/6#newcode124
src/com/google/caja/plugin/caja.js:124: var fd = frame.contentWindow.document;
// .contentDocument not IE-compatible
On 2010/07/14 04:11:44, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/1812042/diff/1/6#newcode127
src/com/google/caja/plugin/caja.js:127: fscript.type = "text/javascript";
On 2010/07/14 04:11:44, ihab.awad wrote:
> Prefer fscript.setAttribute("type", "...");

Done.

Got a handy reference for when to use which?

http://codereview.appspot.com/1812042/diff/1/6#newcode128
src/com/google/caja/plugin/caja.js:128: fscript.src = cajaServer + filename;
On 2010/07/14 04:11:44, ihab.awad wrote:
> Are you relying on the fact that "cajaServer" contains a trailing slash?

Yes, because at this layer I haven't loaded uri.js to do something cleverer, and
double-slashes are up to the server to interpret.

> Should you?

Ideally, no. Suggestions? Should we do some cheap string fiddling, or load
uri.js, or...?

http://codereview.appspot.com/1812042/diff/1/6#newcode135
src/com/google/caja/plugin/caja.js:135: }
On 2010/07/14 04:11:44, ihab.awad wrote:
> Missing a semicolon here?

Done.

http://codereview.appspot.com/1812042/diff/1/7
File src/com/google/caja/plugin/host-iframe-final.js (right):

http://codereview.appspot.com/1812042/diff/1/7#newcode1
src/com/google/caja/plugin/host-iframe-final.js:1: // This file exists to be
concatenated into the single file that caja.js (the
On 2010/07/14 04:11:44, ihab.awad wrote:
> Needs copyright header (!!)

Done.

http://codereview.appspot.com/1812042/diff/1/4
File tests/com/google/caja/plugin/host-iframe-test.js (right):

http://codereview.appspot.com/1812042/diff/1/4#newcode32
tests/com/google/caja/plugin/host-iframe-test.js:32: Unfortunately, this test
doesn't work because other tests running asynchronously also change the layout
of the page.
On 2010/07/14 04:11:44, ihab.awad wrote:
> Long line.

Done.

http://codereview.appspot.com/1812042/diff/1/4#newcode53
tests/com/google/caja/plugin/host-iframe-test.js:53: 
On 2010/07/14 04:11:44, ihab.awad wrote:
> Extra blank line.

Done.

http://codereview.appspot.com/1812042/diff/1/4#newcode83
tests/com/google/caja/plugin/host-iframe-test.js:83: 
On 2010/07/14 04:11:44, ihab.awad wrote:
> Extra blank line.

Done.

http://codereview.appspot.com/1812042/diff/1/4#newcode93
tests/com/google/caja/plugin/host-iframe-test.js:93:
"../../../../../src/com/google/caja/demos/container/gadget-trivial.html"),
On 2010/07/14 04:11:44, ihab.awad wrote:
> Long line.

Done.

ihab.awad

Still LGTM++ http://codereview.appspot.com/1812042/diff/1/6 File src/com/google/caja/plugin/caja.js (right): http://codereview.appspot.com/1812042/diff/1/6#newcode127 src/com/google/caja/plugin/caja.js:127: fscript.type = "text/javascript"; On 2010/07/14 17:44:50, kpreid2 ...

15 years, 11 months ago (2010-07-14 17:48:45 UTC) #4

kpreid1

I am currently working on testing after the requested changes and will commit shortly. On ...

15 years, 11 months ago (2010-07-14 18:35:39 UTC) #5

Jasvir

Using createAttribute and setAttributeNode instead of setAttribute work around the problems in IE and is ...

15 years, 11 months ago (2010-07-14 18:49:49 UTC) #6

ihab.awad

15 years, 11 months ago (2010-07-14 19:57:45 UTC) #7

On 2010/07/14 18:49:49, jasvir wrote:
> Using createAttribute and setAttributeNode instead of setAttribute work around
> the problems in IE and is cross-browser compatible.

In that case, the right thing would be to use setAttribute from here:

http://code.google.com/p/google-caja/source/browse/trunk/src/com/google/caja/...

which, fwiw, also helpfully lists all the hazards.

Expand All Messages | Collapse All Messages