Conditional loading of module.css files

I really like this patch a lot -- it definitely makes conditional load of CSS files make sense.

And it works *perfectly* with the CSS aggregation patch for smaller aggregate files and less of them.

Seems like a win all around.

I'd say this is RTBC but I'm a bit tired so I'll leave that for someone else in case I overlooked something small :-)

Steven - I wouldn't include the comment "Add CSS" - the name of the function call has these two words in it already, and it's a pretty self-explanable one-liner. Just wasted space and makes for ugly whitespace valleys. I also don't see watchdog in this patch? (For onlookers, color.css isn't included in the patch because it's already conditionally sound.)

I love you guys. ^^ I'll test it as soon as the preprocessor & this needs review.

The comments still bug me, but certainly not a showstopper. This first patch is ready to go in.

Patch looks good. I agree with Morbus on the comment. Maybe: "Conditionally include the CSS to reduce overhead:" would be better?

Some performance benchmarks would be useful here. :)

OK, So I did some benchmarking of #100563 and #100516. I used the same method as in my original bug report.
I am posting these results in both places, since they are both looking at fixing the same issue.

Here are the results, see below for explanation:

          HEAD
          Total       Transfer Duration       Page Duration
Test 1    12798       8756                    8454
Test 2    12984       8907                    8162
Test 3    12730       8718                    7966
Average   12837       8794                    8194
Baseline  100%        100%                    100%
                      
          conditional_css_include_2.patch
          Total       Transfer Duration       Page Duration
Test 1    10633       7111                    6828
Test 2    11225       7332                    7530
Test 3    11047       7639                    7357
Average   10968.33    7361                    7238
Faster By 15%         16%                     12%
                      
          cache_19.patch
          Total       Transfer Duration       Page Duration
Test 1    9160        5453                    5175
Test 2    9417        5712                    4961
Test 3    9056        5306                    5290
Average   9211        5490                    5142
Faster By 28%         38%                     37%

• Testing was done locally, to eliminate the variable latency you get on live networks. The Charles web debugging proxy was used to apply a consistent throttle and latency equivalent to a typical 64Kbps connection.
• Browser caching was disabled completely, to simulate an initial page load. I can repeat, with caching enabled (to test http cache freshness checks) if that would be useful.
• Drupal page caching (css caching for #100516) was enabled, and the test was done as an anonymous user. All Drupal modules were enabled (a few of these would more likely be contrib modules in reality). The page cache was cleared before each set of tests, and 2 dummy reloads were made before starting timing.
• The first column 'Total' is the total time spent transferring data, as reported by Charles. This does not include the time saved by browser pipelining of http requests.
• The second column 'Transfer Duration' is the Duration between the first byte transfer and the last byte transfer, as reported by Charles
• The third column 'Page Duration' is the time between the end user hitting refresh and Firefox finishing building the page. This is occasionally less that the transfer duration, which is a little odd, but perhaps certain page graphics (favicons maybe?) are not included in the page build time.

- As you can see, conditionally loading CSS is approximately 15% faster than the current situation, whilst a preprocessing cache is almost 40% faster.

In real time conditionally loading CSS is saves about 1 second from the time taken to initially load the page for the user. This is certainly worth committing in my opinion.

Adding a preprocessing cache saves about 3 seconds from the time taken to initially load the page for the user. This is, in my opinion an enormous benefit (compared to the effort spent whittling 50ms off a database query!) and is well worth the added complexity.

Grugnog2, is it possible to conduct a test with both patches combined using the same criteria? Thanks in advance :)

These results are totally impressive! Im speechless.
Changed priority to critical to put it in line with the CSS preprocessor patch.

OK, here are the combined results of both patches.

As I expected, there isn't any net benefit to using both approaches, suggesting that the effect of network latency with a large number of small files is (as we have been trying to point out many many times) a much more important factor than just the net file transfer size. Newbies who don't turn on CSS caching will still benefit from the conditional loading patch, however, so it is still worth committing IMHO - unless we decide to turn CSS caching on by default.

          conditional_css_include_2.patch AND cache_19.patch
          Total       Transfer Duration       Page Duration
Test 1    9595        5615                    5333
Test 2    9909        5709                    5425
Test 3    9461        5824                    5537
Average   9655        5716                    5432
Faster By 25%         35%                     34%

Note: The numbers report that they are very slightly slower, but I think this difference is probably a red herring and not statistically significant. I could run more tests if people thought this could be a real issue (or could think of a reason why there might be a real issue here).

Also of note, both of these patches when combined, will create *smaller* aggregated CSS files which will use up less bandwidth.

@Steven - thanks! I meant to explain this with my post, but ran out of time. Since I was hitting the front page we are simply seeing the result of a slightly smaller cache file (i.e. just cache hits). For other pages there is the delicate balance between hits and misses as you describe. As Robert suggests, we may eventually want to allow a contrib to provide an interface to configure cached vs. non-cached CSS (and even custom group them), since the balance will obviously vary between sites.

Another important note:
If we actually applied both patches, we would of course need to adjust the calls to drupal_add_css() that the 'less frequent' CSS files are not included in the cache ($cache = FALSE). Right now the caching would generate a new complete cache for each combination, resulting in unnecessary multiple downloads of the same CSS code (see http://drupal.org/node/98819#comment-161026 for a full explanation).

the initial post here is the only description i know of clearly describes how to use the $preprocess parameter on drupal_add_css(). would be nice if we added very descriptive doxygen comment to that function.

Here's a proposed patch to enhance the comments.

The Doxygen will need to be wrapped me thinks.

I wrapped the lines and committed the patch ... BUT ... I think it needs more and better documentation.

1. The current phpDoc code assumes that a developer what the benefits of preprocessing are. I think that's problematic so I suggest that we add a short introduction so people can actually understand what this is about.

2. The documentation mentions what to preprocess but it doesn't provide a clue as what NOT to preprocess. All in all, it's not very clear.

Keeping the status as 'code needs work'.

Updated patch to clarify things.

Revised edition, even clearer.