Bug 428313: Properly expire the browser's CSS and JS cache when there

are new versions of those files. This also eliminates single-file skins
and should also allow Extensions to have skins.
r=glob, a=mkanat

8 files changed, 267 insertions, 150 deletions

diff --git a/.htaccess b/.htaccess
index 72a96e064..d4436e563 100644
--- a/.htaccess
+++ b/.htaccess
@@ -2,3 +2,14 @@
 <FilesMatch ^(.*\.pm|.*\.pl|.*localconfig.*)$>
   deny from all
 </FilesMatch>
+<FilesMatch (\.js|\.css)$>
+  ExpiresActive On
+  # According to RFC 2616, "1 year in the future" means "never expire".
+  # We change the name of the file's URL whenever its modification date
+  # changes, so browsers can cache any individual JS or CSS URL forever.
+  # However, since all JS and CSS URLs involve a ? in them (for the changing
+  # name) we have to explicitly set an Expires header or browsers won't
+  # *ever* cache them.
+  ExpiresDefault "now plus 1 years"
+  Header append Cache-Control "public"
+</FilesMatch>
diff --git a/Bugzilla/Install/Filesystem.pm b/Bugzilla/Install/Filesystem.pm
index db55576a4..20bd021ef 100644
--- a/Bugzilla/Install/Filesystem.pm
+++ b/Bugzilla/Install/Filesystem.pm
@@ -284,20 +284,6 @@ sub FILESYSTEM {
                                         contents => '' },
     );
 
-    # Each standard stylesheet has an associated custom stylesheet that
-    # we create. Also, we create placeholders for standard stylesheets
-    # for contrib skins which don't provide them themselves.
-    foreach my $skin_dir ("$skinsdir/custom", <$skinsdir/contrib/*>) {
-        next if basename($skin_dir) =~ /^cvs$/i;
-        foreach my $base_css (<$skinsdir/standard/*.css>) {
-            _add_custom_css($skin_dir, basename($base_css), \%create_files);
-        }
-        foreach my $dir_css (<$skinsdir/standard/*/*.css>) {
-            $dir_css =~ s{.+?([^/]+/[^/]+)$}{$1};
-            _add_custom_css($skin_dir, $dir_css, \%create_files);
-        }
-    }
-
     # Because checksetup controls the creation of index.html separately
     # from all other files, it gets its very own hash.
     my %index_html = (
@@ -455,20 +441,53 @@ EOT
         print "Removing duplicates directory...\n";
         rmtree("$datadir/duplicates");
     }
+
+    _remove_empty_css_files();
+    _convert_single_file_skins();
 }
 
-# A simple helper for creating "empty" CSS files.
-sub _add_custom_css {
-    my ($skin_dir, $path, $create_files) = @_;
-    $create_files->{"$skin_dir/$path"} = { perms => WS_SERVE, contents => <<EOT
+sub _remove_empty_css_files {
+    my $skinsdir = bz_locations()->{'skinsdir'};
+    foreach my $css_file (glob("$skinsdir/custom/*.css"),
+                          glob("$skinsdir/contrib/*/*.css"))
+    {
+        _remove_empty_css($css_file);
+    }
+}
+
+# A simple helper for the update code that removes "empty" CSS files.
+sub _remove_empty_css {
+    my ($file) = @_;
+    my $basename = basename($file);
+    my $empty_contents = <<EOT;
 /*
- * Custom rules for $path.
+ * Custom rules for $basename.
  * The rules you put here override rules in that stylesheet.
  */
 EOT
+    if (length($empty_contents) == -s $file) {
+        open(my $fh, '<', $file) or warn "$file: $!";
+        my $file_contents;
+        { local $/; $file_contents = <$fh>; }
+        if ($file_contents eq $empty_contents) {
+            print install_string('file_remove', { name => $file }), "\n";
+            unlink $file or warn "$file: $!";
+        }
     };
 }
 
+# We used to allow a single css file in the skins/contrib/ directory
+# to be a whole skin.
+sub _convert_single_file_skins {
+    my $skinsdir = bz_locations()->{'skinsdir'};
+    foreach my $skin_file (glob "$skinsdir/contrib/*.css") {
+        my $dir_name = $skin_file;
+        $dir_name =~ s/\.css$//;
+        mkdir $dir_name or warn "$dir_name: $!";
+        _rename_file($skin_file, "$dir_name/global.css");
+    }
+}
+
 sub create_htaccess {
     _create_files(%{FILESYSTEM()->{htaccess}});
 
@@ -492,7 +511,7 @@ sub create_htaccess {
 
 sub _rename_file {
     my ($from, $to) = @_;
-    print "Renaming $from to $to...\n";
+    print install_string('file_rename', { from => $from, to => $to }), "\n";
     if (-e $to) {
         warn "$to already exists, not moving\n";
     }
diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm
index 2c2d402a3..ffd702e62 100644
--- a/Bugzilla/Template.pm
+++ b/Bugzilla/Template.pm
@@ -358,6 +358,121 @@ sub get_bug_link {
     return qq{$pre<a href="$linkval" title="$title">$link_text</a>$post};
 }
 
+#####################
+# Header Generation #
+#####################
+
+# Returns the last modification time of a file, as an integer number of
+# seconds since the epoch.
+sub _mtime { return (stat($_[0]))[9] }
+
+sub mtime_filter {
+    my ($file_url) = @_;
+    my $cgi_path = bz_locations()->{'cgi_path'};
+    my $file_path = "$cgi_path/$file_url";
+    return "$file_url?" . _mtime($file_path);
+}
+
+# Set up the skin CSS cascade:
+#
+#  1. YUI CSS
+#  2. Standard Bugzilla stylesheet set (persistent)
+#  3. Standard Bugzilla stylesheet set (selectable)
+#  4. All third-party "skin" stylesheet sets (selectable)
+#  5. Page-specific styles
+#  6. Custom Bugzilla stylesheet set (persistent)
+#
+# "Selectable" skin file sets may be either preferred or alternate.
+# Exactly one is preferred, determined by the "skin" user preference.
+sub css_files {
+    my ($style_urls, $yui, $yui_css) = @_;
+    
+    # global.css goes on every page, and so does IE-fixes.css.
+    my @requested_css = ('skins/standard/global.css', @$style_urls,
+                         'skins/standard/IE-fixes.css');
+
+    my @yui_required_css;
+    foreach my $yui_name (@$yui) {
+        next if !$yui_css->{$yui_name};
+        push(@yui_required_css, "js/yui/assets/skins/sam/$yui_name.css");
+    }
+    unshift(@requested_css, @yui_required_css);
+    
+    my @css_sets = map { _css_link_set($_) } @requested_css;
+    
+    my %by_type = (standard => [], alternate => {}, skin => [], custom => []);
+    foreach my $set (@css_sets) {
+        foreach my $key (keys %$set) {
+            if ($key eq 'alternate') {
+                foreach my $alternate_skin (keys %{ $set->{alternate} }) {
+                    my $files = $by_type{alternate}->{$alternate_skin} ||= [];
+                    push(@$files, $set->{alternate}->{$alternate_skin});
+                }
+            }
+            else {
+                push(@{ $by_type{$key} }, $set->{$key});
+            }
+        }
+    }
+    
+    return \%by_type;
+}
+
+sub _css_link_set {
+    my ($file_name) = @_;
+
+    my $standard_mtime = _mtime($file_name);
+    my %set = (standard => $file_name . "?$standard_mtime");
+    
+    # We use (^|/) to allow Extensions to use the skins system if they
+    # want.
+    if ($file_name !~ m{(^|/)skins/standard/}) {
+        return \%set;
+    }
+    
+    my $user = Bugzilla->user;
+    my $cgi_path = bz_locations()->{'cgi_path'};
+    my $all_skins = $user->settings->{'skin'}->legal_values;    
+    my %skin_urls;
+    foreach my $option (@$all_skins) {
+        next if $option eq 'standard';
+        my $skin_file_name = $file_name;
+        $skin_file_name =~ s{(^|/)skins/standard/}{skins/contrib/$option/};
+        if (my $mtime = _mtime("$cgi_path/$skin_file_name")) {
+            $skin_urls{$option} = $skin_file_name . "?$mtime";
+        }
+    }
+    $set{alternate} = \%skin_urls;
+    
+    my $skin = $user->settings->{'skin'}->{'value'};
+    if ($skin ne 'standard' and defined $set{alternate}->{$skin}) {
+        $set{skin} = delete $set{alternate}->{$skin};
+    }
+    
+    my $custom_file_name = $file_name;
+    $custom_file_name =~ s{(^|/)skins/standard/}{skins/custom/};
+    if (my $custom_mtime = _mtime("$cgi_path/$custom_file_name")) {
+        $set{custom} = $custom_file_name . "?$custom_mtime";
+    }
+    
+    return \%set;
+}
+
+# YUI dependency resolution
+sub yui_resolve_deps {
+    my ($yui, $yui_deps) = @_;
+    
+    my @yui_resolved;
+    foreach my $yui_name (@$yui) {
+        my $deps = $yui_deps->{$yui_name} || [];
+        foreach my $dep (reverse @$deps) {
+            push(@yui_resolved, $dep) if !grep { $_ eq $dep } @yui_resolved;
+        }
+        push(@yui_resolved, $yui_name) if !grep { $_ eq $yui_name } @yui_resolved;
+    }
+    return \@yui_resolved;
+}
+
 ###############################################################################
 # Templatization Code
 
@@ -647,6 +762,8 @@ sub create {
             html_light => \&Bugzilla::Util::html_light_quote,
 
             email => \&Bugzilla::Util::email_filter,
+            
+            mtime_url => \&mtime_filter,
 
             # iCalendar contentline filter
             ics => [ sub {
@@ -769,6 +886,9 @@ sub create {
                     Bugzilla->fields({ by_name => 1 });
                 return $cache->{template_bug_fields};
             },
+            
+            'css_files' => \&css_files,
+            yui_resolve_deps => \&yui_resolve_deps,
 
             # Whether or not keywords are enabled, in this Bugzilla.
             'use_keywords' => sub { return Bugzilla::Keyword->any_exist; },
diff --git a/docs/en/xml/installation.xml b/docs/en/xml/installation.xml
index 2b8e1b878..b498acb81 100644
--- a/docs/en/xml/installation.xml
+++ b/docs/en/xml/installation.xml
@@ -1044,7 +1044,7 @@ max_allowed_packet=4M
     AddHandler cgi-script .cgi
     Options +Indexes +ExecCGI
     DirectoryIndex index.cgi
-    AllowOverride Limit
+    AllowOverride Limit FileInfo Indexes
     &lt;/Directory&gt;
                 </programlisting>
     
diff --git a/mod_perl.pl b/mod_perl.pl
index a21d5d725..32fe82ccf 100644
--- a/mod_perl.pl
+++ b/mod_perl.pl
@@ -74,7 +74,7 @@ PerlChildInitHandler "sub { srand(); }"
     $sizelimit
     PerlOptions +ParseHeaders
     Options +ExecCGI
-    AllowOverride Limit
+    AllowOverride Limit FileInfo Indexes
     DirectoryIndex index.cgi index.html
 </Directory>
 EOT
diff --git a/skins/README b/skins/README
new file mode 100644
index 000000000..111c00f03
--- /dev/null
+++ b/skins/README
@@ -0,0 +1,20 @@
+There are three directories here, standard/, custom/, and contrib/.
+
+standard/ holds the standard stylesheets. These are used no matter
+what skin the user selects. If the user selects the "Classic" skin,
+then *only* the standard/ stylesheets are used.
+
+contrib/ holds "skins" that the user can select in their preferences.
+skins are in directories, and they contain files with the same names
+as the files in skins/standard/. Simply putting a new directory
+into the contrib/ directory adds a new skin as an option in users'
+preferences.
+
+custom/ allows you to locally override the standard/ and contrib/ CSS.
+If you put files into the custom/ directory with the same names as the CSS
+files in skins/standard/, you can override the standard/ and contrib/
+CSS. For example, if you want to override some CSS in
+skins/standard/global.css, then you should create a file called "global.css"
+in custom/ and put some CSS in it. The CSS you put into files in custom/ will
+be used *in addition* to the CSS in skins/standard/ or the CSS in
+skins/contrib/. It will apply to every skin.
diff --git a/template/en/default/global/header.html.tmpl b/template/en/default/global/header.html.tmpl
index 721afd7af..549fd538f 100644
--- a/template/en/default/global/header.html.tmpl
+++ b/template/en/default/global/header.html.tmpl
@@ -64,6 +64,14 @@
   datatable    => ['json', 'connection', 'datasource', 'element'],
 } %]
 
+[%# These are JS URLs that are *always* on the page and come before
+  # every other JS URL.
+  #%]
+[% SET starting_js_urls = [
+    "js/yui/yahoo-dom-event/yahoo-dom-event.js",
+    "js/yui/cookie/cookie-min.js",
+] %]
+
 
 [%# We should be able to set the default value of the header variable
   # to the value of the title variable using the DEFAULT directive,
@@ -87,118 +95,33 @@
 
     [% PROCESS 'global/setting-descs.none.tmpl' %]
 
-    [%# Set up the skin CSS cascade:
-      #  0. YUI CSS
-      #  1. Standard Bugzilla stylesheet set (persistent)
-      #  2. Standard Bugzilla stylesheet set (selectable)
-      #  3. All third-party "skin" stylesheet sets (selectable)
-      #  4. Page-specific styles
-      #  5. Custom Bugzilla stylesheet set (persistent)
-      # "Selectable" skin file sets may be either preferred or alternate.
-      # Exactly one is preferred, determined by the "skin" user preference.
-      #%]
-    [% IF user.settings.skin.value != 'standard' %]
-      [% user_skin = user.settings.skin.value %]
-    [% END %]
-    [% style_urls.unshift('skins/standard/global.css') %]
-
-    [%# YUI dependency resolution %]
-    [%# We have to do this in a separate array, because modifying the
-      # existing array by unshift'ing dependencies confuses FOREACH.
-      #%]
-    [% SET yui_resolved = [] %]
-    [% FOREACH yui_name = yui %]
-      [% FOREACH yui_dep = yui_deps.${yui_name}.reverse %]
-        [% yui_resolved.push(yui_dep) IF NOT yui_resolved.contains(yui_dep) %]
-      [% END %]
-      [% yui_resolved.push(yui_name) IF NOT yui_resolved.contains(yui_name) %]
-    [% END %]
-    [% SET yui = yui_resolved %]
-
-    [%# YUI CSS %]
-    [% FOREACH yui_name = yui %]
-      [% IF yui_css.$yui_name %]
-        <link rel="stylesheet" type="text/css"
-              href="js/yui/assets/skins/sam/[%- yui_name FILTER html %].css">
-      [% END %]
-    [% END %]
+    [% SET yui = yui_resolve_deps(yui, yui_deps) %]
+    [% SET css_sets = css_files(style_urls, yui, yui_css) %]
 
     [%# CSS cascade, part 1: Standard Bugzilla stylesheet set (persistent).
       # Always present.
       #%]
-    [% FOREACH style_url = style_urls %]
-      <link href="[% style_url FILTER html %]"
-            rel="stylesheet"
-            type="text/css">
+    [%# This allows people to switch back to the "Classic" skin if they
+      # are in another skin. 
+      #%]
+    <link href="[% 'skins/standard/global.css' FILTER mtime_url FILTER html %]"
+          rel="alternate stylesheet" 
+          title="[% setting_descs.standard FILTER html %]">
+    [% FOREACH style_url = css_sets.standard %]
+      [% PROCESS format_css_link css_set_name = 'standard' %]
     [% END %]
-    <!--[if lte IE 7]>
-      [%# Internet Explorer treats [if IE] HTML comments as uncommented.
-        # Use it to import CSS fixes so that Bugzilla looks decent on IE 7
-        # and below.
-        #%]
-      <link href="skins/standard/IE-fixes.css"
-            rel="stylesheet"
-            type="text/css">
-    <![endif]-->
 
-    [%# CSS cascade, part 2: Standard Bugzilla stylesheet set (selectable)
-      # Present if skin selection is enabled.
+    [%# CSS cascade, part 2 & 3: Third-party stylesheet set (selected and
+      # selectable). All third-party skins are present as alternate
+      # stylesheets, even if they are not currently in use.
       #%]
-    [% IF user.settings.skin.is_enabled %]
-      [% FOREACH style_url = style_urls %]
-        <link href="[% style_url FILTER html %]"
-              rel="[% 'alternate ' IF user_skin %]stylesheet"
-              title="[% setting_descs.standard FILTER html %]"
-              type="text/css">
-      [% END %]
-      <!--[if lte IE 7]>
-      [%# Internet Explorer treats [if IE] HTML comments as uncommented.
-        # Use it to import CSS fixes so that Bugzilla looks decent on IE 7
-        # and below.
-        #%]
-        <link href="skins/standard/IE-fixes.css"
-              rel="[% 'alternate ' IF user_skin %]stylesheet"
-              title="[% setting_descs.standard FILTER html %]"
-              type="text/css">
-      <![endif]-->
+    [% FOREACH style_url = css_sets.skin %]
+      [% PROCESS format_css_link css_set_name = user.settings.skin.value %]
     [% END %]
 
-    [%# CSS cascade, part 3: Third-party stylesheet set (selectable).
-      # All third-party skins are present if skin selection is enabled.
-      # The admin-selected skin is always present.
-      #%]
-    [% FOREACH contrib_skin = user.settings.skin.legal_values %]
-      [% NEXT IF contrib_skin == 'standard' %]
-      [% NEXT UNLESS contrib_skin == user_skin
-                  OR user.settings.skin.is_enabled %]
-      [% contrib_skin = contrib_skin FILTER url_quote %]
-      [% IF contrib_skin.match('\.css$') %]
-        [%# 1st skin variant: single-file stylesheet %]
-        <link href="[% "skins/contrib/$contrib_skin" %]"
-              rel="[% 'alternate ' UNLESS contrib_skin == user_skin %]stylesheet"
-              title="[% contrib_skin FILTER html %]"
-              type="text/css">
-      [% ELSE %]
-        [%# 2nd skin variant: stylesheet set %]
-        [% FOREACH style_url = style_urls %]
-          [% IF style_url.match('^skins/standard/') %]
-            <link href="[% style_url.replace('^skins/standard/',
-                                             "skins/contrib/$contrib_skin/") %]"
-                  rel="[% 'alternate ' UNLESS contrib_skin == user_skin %]stylesheet"
-                  title="[% contrib_skin FILTER html %]"
-                  type="text/css">
-          [% END %]
-        [% END %]
-        <!--[if lte IE 7]>
-          [%# Internet Explorer treats [if IE] HTML comments as uncommented.
-            # Use it to import CSS fixes so that Bugzilla looks decent on IE 7
-            # and below.
-            #%]
-          <link href="skins/contrib/[% contrib_skin FILTER html %]/IE-fixes.css"
-                rel="[% 'alternate ' UNLESS contrib_skin == user_skin %]stylesheet"
-                title="[% contrib_skin FILTER html %]"
-                type="text/css">
-        <![endif]-->
+    [% FOREACH alternate_skin = css_sets.alternate.keys %]
+      [% FOREACH style_url = css_sets.alternate.$alternate_skin %]
+        [% PROCESS format_css_link css_set_name = alternate_skin %]
       [% END %]
     [% END %]
 
@@ -214,33 +137,19 @@
       # Always present. Site administrators may override all other style
       # definitions, including skins, using custom stylesheets.
       #%]
-    [% FOREACH style_url = style_urls %]
-      [% IF style_url.match('^skins/standard/') %]
-        <link href="[% style_url.replace('^skins/standard/', "skins/custom/")
-                       FILTER html %]" rel="stylesheet" type="text/css">
-      [% END %]
+    [% FOREACH style_url = css_sets.custom %]
+      [% PROCESS format_css_link css_set_name = 'standard' %]
     [% END %]
-    <!--[if lte IE 7]>
-      [%# Internet Explorer treats [if IE] HTML comments as uncommented.
-        # Use it to import CSS fixes so that Bugzilla looks decent on IE 7
-        # and below.
-        #%]
-      <link href="skins/custom/IE-fixes.css"
-            rel="stylesheet"
-            type="text/css">
-    <![endif]-->
 
     [%# YUI Scripts %]
-    <script src="js/yui/yahoo-dom-event/yahoo-dom-event.js"
-            type="text/javascript"></script>
-    <script src="js/yui/cookie/cookie-min.js" type="text/javascript"></script>
     [% FOREACH yui_name = yui %]
-      <script type="text/javascript" 
-              src="js/yui/[% yui_name FILTER html %]/
-                   [%- yui_name FILTER html %]-min.js"></script>
+      [% starting_js_urls.push("js/yui/$yui_name/${yui_name}.js") %]
     [% END %]
+    [% starting_js_urls.push('js/global.js') %]
 
-    <script src="js/global.js" type="text/javascript"></script>
+    [% FOREACH javascript_url = starting_js_urls %]
+      [% PROCESS format_js_link %]
+    [% END %]
 
     <script type="text/javascript">
     <!--
@@ -291,10 +200,8 @@
     // -->
     </script>
 
-    [% IF javascript_urls %]
-      [% FOREACH javascript_url = javascript_urls %]
-        <script src="[% javascript_url FILTER html %]" type="text/javascript"></script>
-      [% END %]
+    [% FOREACH javascript_url = javascript_urls %]
+      [% PROCESS format_js_link %]
     [% END %]
 
     [%# this puts the live bookmark up on firefox for the Atom feed %]
@@ -380,3 +287,41 @@
 [% IF message %]
 <div id="message">[% message %]</div>
 [% END %]
+
+[% BLOCK format_css_link %]
+  [% IF style_url.match('/IE-fixes\.css') %]
+    <!--[if lte IE 7]>
+      [%# Internet Explorer treats [if IE] HTML comments as uncommented.
+        # We use it to import CSS fixes so that Bugzilla looks decent on IE 7
+        # and below.
+        #%]
+  [% END %]
+
+  [% IF css_set_name == 'standard'
+       OR css_set_name == user.settings.skin.value
+  %]
+    [% SET css_rel = 'stylesheet' %]
+    [% SET css_set_display_name = setting_descs.${user.settings.skin.value}
+                                  || user.settings.skin.value %]
+  [% ELSE %]
+    [% SET css_rel = 'alternate stylesheet' %]
+    [% SET css_set_display_name = setting_descs.$css_set_name || css_set_name %]
+  [% END %]
+
+  [% IF css_set_name == 'standard' %]
+    [% SET css_title_link = '' %]
+  [% ELSE %]
+    [% css_title_link = BLOCK ~%]
+      title="[% css_set_display_name FILTER html %]"
+    [% END %]
+  [% END %]
+
+  <link href="[% style_url FILTER html %]" rel="[% css_rel FILTER none %]"
+        type="text/css" [% css_title_link FILTER none %]>
+
+  [% '<![endif]-->' IF style_url.match('/IE-fixes\.css') %]
+[% END %]
+
+[% BLOCK format_js_link %]
+  <script type="text/javascript" src="[% javascript_url FILTER mtime_url FILTER html %]"></script>
+[% END %]
diff --git a/template/en/default/setup/strings.txt.pl b/template/en/default/setup/strings.txt.pl
index 7e590cb3e..62b645672 100644
--- a/template/en/default/setup/strings.txt.pl
+++ b/template/en/default/setup/strings.txt.pl
@@ -71,6 +71,8 @@ END
     feature_updates           => 'Automatic Update Notifications',
     feature_xmlrpc            => 'XML-RPC Interface',
 
+    file_remove => 'Removing ##name##...',
+    file_rename => 'Renaming ##from## to ##to##...',
     header => "* This is Bugzilla ##bz_ver## on perl ##perl_ver##\n"
             . "* Running on ##os_name## ##os_ver##",
     install_all => <<EOT,