Was noticing an excessive number of calls to drupal_add_css() and discovered that the logic in views_plugin_cache::gather_headers() is a bit broken. The array it's checking against to get the diffs is cleared right before it, which results in the entire $css and $js array being cached.

Attached patch simplifies the logic and appears to work well in my testing.

Files: 
CommentFileSizeAuthor
#4 1530740-cache-storage.patch5.77 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 1,408 pass(es).
[ View ]
#2 1530740-test-cache-storage.patch3.47 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 1,403 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#2 1530740-cache-storage.patch5.18 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 1,404 pass(es).
[ View ]
views_fix_cssjs_cache_add.patch1.71 KBmsonnabaum
PASSED: [[SimpleTest]]: [MySQL] 1,398 pass(es).
[ View ]

Comments

Status:Active» Needs review

Applies cleanly and seems to reduce the time spent in drupal_add_css in brief xhprof testing.

Let's see what testbot thinks of it.

StatusFileSize
new5.18 KB
PASSED: [[SimpleTest]]: [MySQL] 1,404 pass(es).
[ View ]
new3.47 KB
FAILED: [[SimpleTest]]: [MySQL] 1,403 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Here is a test for this issue. There is one expected test fail.

Status:Needs review» Needs work

Overall this looks great, and it's wonderful to see a test!
Coding/grammar nitpicks:

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // @FIXME: The cache plugin expects settings['data'] to be set

Use @todo, and no :, and a trailing full stop. Unless this is to be fixed before committing?

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // Take sure that this css is not added when running the cached view.

s/Take sure/Make sure

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    $this->assertFalse(isset($css[$system_css_path]), 'Take sure that unrelated css is not added.');
+    $this->assertFalse(isset($js[$system_js_path]), 'Take sure that unrelated js is not added.');

s/Take sure/Make sure

+++ b/plugins/views_plugin_cache.incundefined
@@ -201,32 +201,18 @@ class views_plugin_cache extends views_plugin {
+    // Get javascript after/before views renders

Missing trailing full stop.

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // @FIXME: The cache plugin expects settings['data'] to be set

Use @todo, and no :, and a trailing full stop. Unless this is to be fixed before committing?

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    // Take sure that this css is not added when running the cached view.

s/Take sure/Make sure

+++ b/tests/views_cache.testundefined
@@ -148,4 +148,66 @@ class ViewsCacheTest extends ViewsSqlTest {
+    $this->assertFalse(isset($css[$system_css_path]), 'Take sure that unrelated css is not added.');
+    $this->assertFalse(isset($js[$system_js_path]), 'Take sure that unrelated js is not added.');

s/Take sure/Make sure

Status:Needs work» Needs review
StatusFileSize
new5.77 KB
PASSED: [[SimpleTest]]: [MySQL] 1,408 pass(es).
[ View ]

dawehner-- for doing the same stupid thing over and over again.

I'm confused you posted the same review twice, how does dreditor allow you to do that.

Version:7.x-3.x-dev» 6.x-3.x-dev
Status:Needs review» Patch (to be ported)

Let's get it in, the tests pass and the code itself looks fine.

This maybe is worth to backport.