User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13 Build Identifier: 1) Enable mod_expires via apache See: http://httpd.apache.org/docs/2.0/mod/mod_expires.html 1) Turn on the AllowOverride Limit via the apache configuration for the specific bugzilla directory 2) Add the following to the .htaccess files: # Enable Expirations ExpiresActive On # Expire all JS ExpiresByType application/javascript "access plus 10 seconds" ExpiresByType application/x-javascript "access plus 10 seconds" ExpiresByType application/javascript "access plus 10 seconds" ExpiresByType text/css "access plus 1 hours" This should be document for specific uses, since expiring css and javascript too soon will result in WAY more network traffic, but expiring too long will mean users will never get updates to javascript or css files. Reproducible: Always

this sounds bad. we have two kinds of js, static js, and json. giving the same expires policy to both would be disastrous.

I guess this is more intended for CSS files than JS, this is only what I have implemented for myself. Perhaps is should just be a documentation update.

We don't have any JSON. This should probably go in our <Directory> block rather than in the .htaccess.

Sure we do: http://bugzilla.mozilla.org/buglist.cgi?product=Bugzilla&resolution=---&ctype=js it might not be true json, but apache won't know the difference.

This should be a documentation bug, for optional additional configuration. please review :-)

Instead of making this an optional additional configuration, or even making it part of the configuration at all, what we should do is put ?mtime after all the CSS and JS files, where ?mtime is the integer modification time of the file returned by "stat". The webserver already has to stat all the files to see if they're there, anyway, so I don't think we're adding any overhead if we do this. This is a 4.0 blocker because, as it turns out, if you still have the old JS for the attachment details page, your patch review comment will get eaten and discarded.

All right, this solves the bug and still lets browsers cache every JS and CSS file without having to go back to the server to determine if they've been modified. I moved all the logic for determining the stylesheets to use into Bugzilla::Template. Also, if a stylesheet doesn't exist on disk, we no longer add it in the header. This should generally improve first-page-load performance, particularly for people with high-latency connections. To assure that upgraded Bugzillas get this improved performance, I have checksetup.pl remove old "placeholder" CSS files if they really are just the placeholders that checksetup.pl created. Because we now use query strings on the CSS and JS files, browsers won't cache them without an explicit Expires header. So, I've modified our default .htaccess to properly send this Expires header for Apache. This also requires a change in the recommended AllowOverride directive, meaning that most people will have to adjust their Apache configuration if they are using mod_cgi, after this patch lands. I provide (and use) a "new" mtime_url filter, which extensions can also use. I made some slight adjustments to the CSS code that will now allow extensions to use skins if they want, depending on how they are putting their CSS into the header. I removed the ability to have single-file skins. I'm not aware of anybody using that ability, and it would have some code complexity to continue supporting it.

v1 failed the 008filter.t test, this one doesn't.

Comment on attachment 458324 [details] [diff] [review] v2 breaking on sites that don't have mod_expires installed or available doesn't sit right with me. i'm leaning more towards the mod_expires modifications should be a strongly worded installation note only. we should also send a cache-control:public header to get better caching over ssl. the documentation needs to point out that adding the AllowOverride Indexes directive is for allowing Expires* as this isn't very obvious.

(In reply to comment #9) > breaking on sites that don't have mod_expires installed or available doesn't > sit right with me. i'm leaning more towards the mod_expires modifications > should be a strongly worded installation note only. RHEL5 has it enabled by default. It ships with Apache. The lack of it causes a tremendous, noticeable slowdown on every page even when the server is localhost. If it is just a "strongly-worded installation note", then what will happen is that the vast majority of Bugzilla installations will suddenly become slow, people will think that Bugzilla is slow, and usage will drop significantly. > we should also send a cache-control:public header to get better caching over > ssl. Agreed. You tested it and it's necessary? > the documentation needs to point out that adding the AllowOverride Indexes > directive is for allowing Expires* as this isn't very obvious. We don't point out the reasoning for the Limit AllowOverride, though.

Comment on attachment 458324 [details] [diff] [review] v2 Encapsulate either the entire <FilesMatch> or the ExpiresDefault with <IfModule mod_expires.c>...</IfModule>. That addresses glob's concern. >+<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" >+</FilesMatch>

(In reply to comment #7) > I removed the ability to have single-file skins. I'm not aware of anybody using > that ability, and it would have some code complexity to continue supporting it. I have one that I wrote, but no one uses it. I'm sure if the desire came, I could easily convert it from Style.css to Style/global.css. If you are dropping a supported feature, perhaps the install should check for that and convert it for the user?

(In reply to comment #11) > Encapsulate either the entire <FilesMatch> or the ExpiresDefault with <IfModule > mod_expires.c>...</IfModule>. That addresses glob's concern. But not mine. See, I actually *want* Bugzilla to fall over if mod_expires isn't installed. I do *not* want people to run a Bugzilla where every page request has to send 10-20 requests to check if CSS has been modified before it can even display the page!

(In reply to comment #12) > If you are > dropping a supported feature, perhaps the install should check for that and > convert it for the user? Oh, that's a reasonable idea, I could modify the patch to do that, sure.

This adds "public" to Cache-Control and also adds code to convert old single-file stylesheets. Note that the "Header" command requires the FileInfo override, so I've added that to the docs and mod_perl.pl.

Oh, you know actually, I could just do everything with mod_headers, if we really want, since our expiration system is so simple.

Oh wait, nevermind. The "Expires" header is not in seconds--it's in header date format, so I can't just create it ad hoc. I have to use mod_expires, if we want to be RFC-compliant.

Comment on attachment 458796 [details] [diff] [review] v3 r=glob mod_expires has been a standard module for ages, if you don't have it compiled into apache, you've explicitly removed it. the performance difference without these headers is noticeable, so breaking without these headers is acceptable. for non-apache servers, well, they are used to being second-rate bugzilla citizens anyhow :( bug 580471 should help iis here. >+ <link href=[% "skins/standard/global.css" FILTER mtime_url FILTER html %] >+ rel="alternate stylesheet" >+ title="[% setting_descs.standard FILTER html %]"> the href needs to be quoted; please fix on check-in

Thanks glob!! I will fix the quoting on checkin, for sure.

I fixed the quoting and also one template comment, on checkin. Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified .htaccess modified mod_perl.pl modified Bugzilla/Template.pm modified Bugzilla/Install/Filesystem.pm modified docs/en/xml/installation.xml added skins/README modified template/en/default/global/header.html.tmpl modified template/en/default/setup/strings.txt.pl Committed revision 7391. Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/ modified .htaccess modified mod_perl.pl modified Bugzilla/Template.pm modified Bugzilla/Install/Filesystem.pm modified docs/en/xml/installation.xml modified template/en/default/global/header.html.tmpl modified template/en/default/setup/strings.txt.pl Committed revision 7336.

Can we please document the requirement for mod_expires? It doesn't seem to be enabled by default on my Ubuntu 10.04 machine. http://www.google.co.uk/search?hl=en&q=mod_expires%20site%3Abugzilla.org turns up no results, so I don't think it's documented yet. I started getting 500 Server Errors and I didn't know why; I had to track down this bug via the checkin to the .htaccess file with the relevant directives in it. Is there a "doc-me" keyword or something we can add? Or should I file a new bug? Gerv

(In reply to comment #21) > Can we please document the requirement for mod_expires? It doesn't seem to be > enabled by default on my Ubuntu 10.04 machine. Yes, we absolutely plan to. See also bug 586244. > Is there a "doc-me" keyword or something we can add? There's a documentation flag. > Or should I file a new bug? You can file a new bug.

Added to the release notes in bug 604256.