Support external stylesheets loaded with @import in ES5

Supports the loading of external stylesheets with @import.

@r5373

[kpreid2, 2013-04-17 14:08:51]

Please excuse me for volunteering when I wasn't asked; I just saw a few
things...

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:449: function
makeCssUriHandler(baseUri, domicileProp, mime) {
suggest 'method' instead of 'domicileProp' to be more specific than generic-meta

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:457: // TODO: Why is this setting
mime type to image/*?
Because the only things CSS used to fetch are background images? Perhaps move
the mime type parameter into sanitizecss.js, so that even if it's still always
image/* it's more explicit.

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/sani...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/sani...
src/com/google/caja/plugin/sanitizecss.js:33: * \@overrides window setTimeout
unused setTimeout

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-css-imports-guest.html (right):

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:2: - Copyright (C)
2011 Google Inc.
update date

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:17: <div
class="testcont1ainer waiting" id="testCssImports">
misspelled 'testcontainer'

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:18: <base
href="/ant-testlib/com/google/caja/plugin/">
Not necessary and interferes with some things such as the new 'run from source
tree' feature.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:19: <link
href="es53-test-css-imports-1.css" rel="stylesheet"></link>
</link> is wrong

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:36:
jsunitRegisterIf(inES5Mode,
Why is this test specific to ES5 mode? Won't ES5/3 also support @import with the
same outcome, in which case we should want our tests to inform us of
discrepancies?

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:38: function
testCssImports () {
no space before (

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:41: var color =
directAccess.getComputedStyle(candidate, 'background-color');
Maybe mention why you're using directAccess?

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:49:
assertExpectedColor({rgb: 0x000000}, "shouldbezero");
single quotes

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:57:
pass('testCssImports');
Please put a TODO here to move this pass() into the main line of the test once
we have fixed assertAsynchronousRequirement.

[Jasvir, 2013-04-17 15:51:59]

Supports the loading of external stylesheets with @import.

[Jasvir, 2013-04-17 15:52:17]

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:449: function
makeCssUriHandler(baseUri, domicileProp, mime) {
On 2013/04/17 14:08:51, kpreid2 wrote:
> suggest 'method' instead of 'domicileProp' to be more specific than
generic-meta

Done.

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:457: // TODO: Why is this setting
mime type to image/*?
There are more of mimetype clean ups I accumulated.  I'll do the changes you
suggest in a later CL.

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/sani...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/8778046/diff/1/src/com/google/caja/plugin/sani...
src/com/google/caja/plugin/sanitizecss.js:33: * \@overrides window setTimeout
On 2013/04/17 14:08:51, kpreid2 wrote:
> unused setTimeout

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
File tests/com/google/caja/plugin/es53-test-css-imports-guest.html (right):

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:2: - Copyright (C)
2011 Google Inc.
On 2013/04/17 14:08:51, kpreid2 wrote:
> update date

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:17: <div
class="testcont1ainer waiting" id="testCssImports">
On 2013/04/17 14:08:51, kpreid2 wrote:
> misspelled 'testcontainer'

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:18: <base
href="/ant-testlib/com/google/caja/plugin/">
On 2013/04/17 14:08:51, kpreid2 wrote:
> Not necessary and interferes with some things such as the new 'run from source
> tree' feature.

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:19: <link
href="es53-test-css-imports-1.css" rel="stylesheet"></link>
On 2013/04/17 14:08:51, kpreid2 wrote:
> </link> is wrong

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:36:
jsunitRegisterIf(inES5Mode,
On 2013/04/17 14:08:51, kpreid2 wrote:
> Why is this test specific to ES5 mode? Won't ES5/3 also support @import with
the
> same outcome, in which case we should want our tests to inform us of
> discrepancies?

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:38: function
testCssImports () {
On 2013/04/17 14:08:51, kpreid2 wrote:
> no space before (

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:41: var color =
directAccess.getComputedStyle(candidate, 'background-color');
On 2013/04/17 14:08:51, kpreid2 wrote:
> Maybe mention why you're using directAccess?

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:49:
assertExpectedColor({rgb: 0x000000}, "shouldbezero");
On 2013/04/17 14:08:51, kpreid2 wrote:
> single quotes

Done.

https://codereview.appspot.com/8778046/diff/1/tests/com/google/caja/plugin/es...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:57:
pass('testCssImports');
On 2013/04/17 14:08:51, kpreid2 wrote:
> Please put a TODO here to move this pass() into the main line of the test once
> we have fixed assertAsynchronousRequirement.

Done.

[Jasvir, 2013-04-22 16:40:16]

ping.

[ihab.awad, 2013-04-22 22:58:45]

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:457: // TODO: Why is this setting
mime type to image/*?
Because, until this CL, that was the only envisioned use case for CSS URIs?

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:458: function
makeCssUriSanitizer(baseUri) { return makeCssUriHandler(baseUri, 'cssUri',
'image/*'); }
Long lines.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:464: function
continuation(sanitizeStyle, moreToCome) {
If you changed the API of "continuation" so it takes a single object with values
"result" and "moreToCome", you could make things so the "add if no more things
to do" logic is done only once.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:465: safeCss.push(sanitizeStyle);
See my comment in the next file: Given arrival order nondeterminism of your
XHRs, it is not clear to me how this simple push() preserves proper ordering.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:473: cssText,
domicile.suffixStr.replace(/^-/, ''),
Nit: Outdent 1 space.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:481: safeInlineCss =
sanitized.result;
It seems safeInlineCss is just the last snippet of CSS, right? There's no
"inline" versus "non-inline" distinction iiuc. In any case, I found the variable
name non-mnemonic.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:121: sanitizeCssProperty = (function
() {
Could probably do away with this 2nd level of "module pattern" without too much
loss of generality -- just promote a couple of utility functions and stuff?

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:523: var match =
/^url[(]["'](.*)["'][)]/.exec(url);
Surprised the regex doesn't end in "$" for max para-know-ya.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:542: * @param {function(string,
boolean)} cont
inuation

Pun intended? :)

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:543: *     Support external
stylesheets loaded via @import
Can you please be more specific about the function's API?

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:586: continuation(sanitized.result,
sanitized.moreToCome);
Ok ... the claim being that this mechanism alone guarantees proper ordering of
the CSS declarations in both time and space (lexically earlier => inserted into
the page earlier in wall-clock time *and* in a <style> node previous to what is
lexically later). I guess that depends on the implementation of "continuation".

https://codereview.appspot.com/8778046/diff/3002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-css-imports-guest.html (right):

https://codereview.appspot.com/8778046/diff/3002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:58:
pass('testCssImports');
Nice tests! I had to "macro-expand" the rulesets into a single file in order to
understand them, but now that I do, it looks really good.

[Jasvir, 2013-04-23 17:48:23]

Supports the loading of external stylesheets with @import.

[Jasvir, 2013-04-23 17:48:27]

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:457: // TODO: Why is this setting
mime type to image/*?
On 2013/04/22 22:58:46, ihab.awad wrote:
> Because, until this CL, that was the only envisioned use case for CSS URIs?

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:458: function
makeCssUriSanitizer(baseUri) { return makeCssUriHandler(baseUri, 'cssUri',
'image/*'); }
On 2013/04/22 22:58:46, ihab.awad wrote:
> Long lines.

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:464: function
continuation(sanitizeStyle, moreToCome) {
I did it that way initially but given that I want to apply the inline css only
after applying all the imports, the construction seemed easier to understand
with the repetition.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:465: safeCss.push(sanitizeStyle);
The ordering is for a given CSS text, I apply all @imports contained in it first
in order, followed by the rest of the CSS.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:473: cssText,
domicile.suffixStr.replace(/^-/, ''),
On 2013/04/22 22:58:46, ihab.awad wrote:
> Nit: Outdent 1 space.

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/sanitizecss.js (right):

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:121: sanitizeCssProperty = (function
() {
Unless you insist, I'd rather defer that kind of refactoring to a different CL.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:523: var match =
/^url[(]["'](.*)["'][)]/.exec(url);
On 2013/04/22 22:58:46, ihab.awad wrote:
> Surprised the regex doesn't end in "$" for max para-know-ya.

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:542: * @param {function(string,
boolean)} cont
On 2013/04/22 22:58:46, ihab.awad wrote:
> inuation
> 
> Pun intended? :)

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:542: * @param {function(string,
boolean)} cont
On 2013/04/22 22:58:46, ihab.awad wrote:
> inuation
> 
> Pun intended? :)

Done.

https://codereview.appspot.com/8778046/diff/3002/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/sanitizecss.js:543: *     Support external
stylesheets loaded via @import
On 2013/04/22 22:58:46, ihab.awad wrote:
> Can you please be more specific about the function's API?

Done.

https://codereview.appspot.com/8778046/diff/3002/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/es53-test-css-imports-guest.html (right):

https://codereview.appspot.com/8778046/diff/3002/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/es53-test-css-imports-guest.html:58:
pass('testCssImports');
On 2013/04/22 22:58:46, ihab.awad wrote:
> Nice tests! I had to "macro-expand" the rulesets into a single file in order
to
> understand them, but now that I do, it looks really good.

Done.

[ihab.awad, 2013-04-23 20:12:41]

Jasvir and I chatted and we concluded that this algorithm does not *always* work
for all possible interleavings of @import-ed CSS. However, it works well enough
for most simple cases, so we will add a big TODO, check it in, and then submit
an improvement CL later.

On that basis, lgtm++.

[Jasvir, 2013-04-24 11:43:01]

Supports the loading of external stylesheets with @import.