We should have some reftests for using variables in external style sheets, to support the new "CSS3 variables" feature.

Comment on attachment 8367355 [details] [diff] [review] tests Review of attachment 8367355 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the tests. r=me with the change below. ::: layout/style/test/test_variables.html @@ +16,5 @@ > p { border-left-style: inset; padding: 1px; var-decoration: line-through; } > </style> > > +<style id="test4"> > +div { var-a: url("image.png"); background-image: var(a); } Can you put the var-a declaration in an external style sheet? I think we should be testing that the tokens from the external style sheet get interpreted as a url() and resolved against the base URL of the style sheet where you use it (i.e., the local style sheet here that declares background-image).

Also that external style sheet should be in the supports/ directory, so that the base URL is different.

Comment on attachment 8367891 [details] [diff] [review] updated tests Review of attachment 8367891 [details] [diff] [review]: ----------------------------------------------------------------- Oh, I'm sorry. I forgot the external variable was referenced from the mochitest rather than a reftest. So I think it would be better to put it off a subdirectory of layout/style/test/. You can call that directory "support" too. r=me if you move it there, and then I don't need to see the patch again before it lands.

Backed out for bustage. Please make sure this at least compiles locally before requesting checkin. A green Try run would be even better. https://hg.mozilla.org/integration/mozilla-inbound/rev/78bbd153072a https://tbpl.mozilla.org/php/getParsedLog.php?id=33888899&tree=Mozilla-Inbound

Mihaela will you look into this?

I'm not sure what I'm supposed to do for this. I get no errors when building the mozilla-central source + my tests locally (with ./mach build). I don't have access (yet) to build on Try. What else should I try?

I don't think you need to do those Makefile.in changes. Also, test_variables.html refers to external-variable-url.css, which should be supports/external-variable-url.css (it needs to be in a different directory for the test to make sense, as we're testing how URLs are resolved) and that file appears to be missing from the patch.

There were a few things that were causing the test to fail: 1. The background-image property needed to be set in the document itself, with the variable set in the external style sheet. 2. Some Makefile changes were needed to cause external-variable-url.css to be copied into the right place in the test environment. (And using MOCHITEST_FILES won't do, since that only copies files into layout/style/test/; it won't keep things in subdirectories.) 3. The <link> element needed to be inserted into the document after the pref has been set, just like all the <style> elements are populated after the pref has been set. r?gps for the Makefile.in change.

Comment on attachment 8383350 [details] [diff] [review] updated patch Review of attachment 8383350 [details] [diff] [review]: ----------------------------------------------------------------- Ted is currently neck deep converting mochitests to use .ini manifests. I wouldn't be surprised if he's already bit rotted this patch. Deferring to him for review. Hint: He'll probably tell you to move things to support-files in an .ini file.

Comment on attachment 8383350 [details] [diff] [review] updated patch Review of attachment 8383350 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/test/Makefile.in @@ +26,5 @@ > INSTALL_TARGETS += VISITED_REFTEST > > +EXTERNAL_VARIABLE_MOCHITEST_FILES = support/external-variable-url.css > +EXTERNAL_VARIABLE_MOCHITEST_DEST = $(DEPTH)/_tests/testing/mochitest/tests/$(relativesrcdir)/support/ > +INSTALL_TARGETS += EXTERNAL_VARIABLE_MOCHITEST Nope. You just want to stick this in mochitest.ini nowadays: http://mxr.mozilla.org/mozilla-central/source/layout/style/test/mochitest.ini#1 support-files = support/external-variable-url.css will do what you want.

Ah good, thanks.

This added a bunch of reftests without adding those tests to the reftest.list, which means they're not being run. The tests should be added, right?

Good catch.