Last Comment Bug 977969 - concatenate and slightly minify css files
: concatenate and slightly minify css files
Status: RESOLVED FIXED
: bmo-goal, perf
Product: Bugzilla
Classification: Server Software
Component: Bugzilla-General (show other bugs)
: unspecified
: All All
-- enhancement (vote)
: Bugzilla 5.0
Assigned To: Byron Jones ‹:glob›
: default-qa
:
Mentors:
Depends on: 1006288
Blocks: 1013209 1016199 1160598
  Show dependency treegraph
 
Reported: 2014-02-27 20:34 PST by Byron Jones ‹:glob›
Modified: 2015-05-01 14:57 PDT (History)
1 user (show)
glob: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
977969_1.patch (11.77 KB, patch)
2014-04-30 00:34 PDT, Byron Jones ‹:glob›
gerv: review-
Details | Diff | Splinter Review
977969_2.patch (11.04 KB, patch)
2014-05-05 08:56 PDT, Byron Jones ‹:glob›
gerv: review+
Details | Diff | Splinter Review

Description User image Byron Jones ‹:glob› 2014-02-27 20:34:22 PST
we send a lot of css files. too many.
instead we should concatenate and slightly minify css.

broad outline:
- add a developer/production mode switch to localconfig
  - this should only happen in developer mode
- css_files in Template.pm should build concatenated and minified files using a hash of the requested css files
  - needs two files to retain the current ordering around inline styles
  - i suggest only slight minification for now (remove comments, trim lines, remove empty lines)
  - save the unified css file and return the path to that
  - delete all unified files during checksetup
- remove IE specific css file
  - the IE-only stylesheets make this difficult, as it's client-side css selection
  - verify that the hacks are still required (most likely not) and remove
  - if they are still required, use a class on the html tag to identify IE8 (IE9 and above shouldn't require nasty hacks)
    <!--[if IE 8]> <html class="lt-ie8"> <![endif]-->
Comment 1 User image Byron Jones ‹:glob› 2014-02-27 20:37:39 PST
to be clear: minifiation should only happen in production mode.
Comment 2 User image Frédéric Buclin 2014-02-28 01:06:39 PST
I already filed bug 956190 about decreasing the number of CSS files.
Comment 3 User image Byron Jones ‹:glob› 2014-04-10 00:40:05 PDT
(In reply to Frédéric Buclin from comment #2)
> I already filed bug 956190 about decreasing the number of CSS files.

that bug helps a lot, however that doesn't help with extensions, and there's limits to what that approach can do.
Comment 4 User image Byron Jones ‹:glob› 2014-04-10 01:00:25 PDT
(In reply to Byron Jones ‹:glob› from comment #0)
> - remove IE specific css file

these were removed as part of bug 956190.
Comment 5 User image Byron Jones ‹:glob› 2014-04-30 00:34:07 PDT
Created attachment 8415049 [details] [diff] [review]
977969_1.patch

i ended up not implementing the 'development/production' toggle. css development tools work on the parsed css code so there's no harm in working on minified css during development.

- adds a skins/assets/ directory
- when css files are requested:
  - a minified version of each file is created in assets/
  - a concatenated file with all the requested files is created in assets/
- all assets/ files are deleted when checksetup is executed

on an unmodified bugzilla installation, this reduces the number of css requests from 5 to 1 on show_bug.

on bugzila.mozilla.org, which has a large number of extensions, the number of css requests drops from 17 to 2 on show_bug (dropping in size from 54kb to 43kb).

as the mtime of the file is included as part of the key, the workflow of editing stylesheets then reloading a page to pick up the changes remains unchanged.

one part that was interesting is urls in css need to be rewritten to account for the change of location of the stylesheet.
Comment 6 User image Frédéric Buclin 2014-04-30 02:38:55 PDT
Comment on attachment 8415049 [details] [diff] [review]
977969_1.patch

>diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm

>-    # global.css goes on every page.
>-    my @requested_css = ('skins/standard/global.css', @$style_urls);
>+    # global.css belongs on every page
>+    my @requested_css = ( 'skins/standard/global.css', @$style_urls );

I don't see the point to mess with existing code just to add extra whitespaces, and potentially break custom code.


>-        next if !$yui_css->{$yui_name};
>-        push(@yui_required_css, "js/yui/assets/skins/sam/$yui_name.css");
>+        next unless $yui_css->{$yui_name};
>+        push @yui_required_css, "js/yui/assets/skins/sam/$yui_name.css";

Same here...



>-    unshift(@requested_css, @yui_required_css);
>-    
>+    unshift @requested_css, @yui_required_css;

And here, etc...
Comment 7 User image Byron Jones ‹:glob› 2014-04-30 06:11:53 PDT
(In reply to Frédéric Buclin from comment #6)
> I don't see the point to mess with existing code just to add extra
> whitespaces, and potentially break custom code.

i'm fixing formatting in related code.  that's perfectly reasonable.
Comment 8 User image Frédéric Buclin 2014-04-30 08:51:20 PDT
(In reply to Byron Jones ‹:glob› from comment #7)
> i'm fixing formatting in related code.  that's perfectly reasonable.

Formatting what? Each developer/reviewer has his own preference, and in a year or two, another developer/reviewer is going to revert that because he doesn't like extra whitespaces or the lack of parens?? The Bugzilla developer's guide doesn't say anything about this.
Comment 9 User image Gervase Markham [:gerv] 2014-05-02 07:20:38 PDT
Comment on attachment 8415049 [details] [diff] [review]
977969_1.patch

Review of attachment 8415049 [details] [diff] [review]:
-----------------------------------------------------------------

Are the size gains you mention actually relevant if we are using gzip transfer compression (which I'd hope we are)?

r- for issues noted, but this generally looks good. If we find we do need a switch to turn off minification for debugging, we can surely add one later.

Gerv

::: Bugzilla/Install/Filesystem.pm
@@ +272,4 @@
>          # Directories that contain content served directly by the web server.
>          "$skinsdir/custom"      => DIR_WS_SERVE,
>          "$skinsdir/contrib"     => DIR_WS_SERVE,
> +        "$skinsdir/assets"      => DIR_WS_SERVE,

This permissions setting prevents the webserver writing to the directory, so Bugzilla won't even load a page. I think you want DIR_CGI_WRITE | DIR_ALSO_WS_SERVE, but for some reason that doesn't work for me either :-(. chmodding the dir to "g+w" does.

::: Bugzilla/Template.pm
@@ +441,4 @@
>      my @yui_required_css;
>      foreach my $yui_name (@$yui) {
> +        next unless $yui_css->{$yui_name};
> +        push @yui_required_css, "js/yui/assets/skins/sam/$yui_name.css";

"Along the same lines, just because you CAN omit parentheses in many places doesn't mean that you ought to" says http://search.cpan.org/dist/perl/pod/perlstyle.pod :-) 

Unless you are making something match the Perl style guide, please don't make formatting changes.
Comment 10 User image Byron Jones ‹:glob› 2014-05-04 21:23:03 PDT
(In reply to Gervase Markham [:gerv] from comment #9)
> Are the size gains you mention actually relevant if we are using gzip
> transfer compression (which I'd hope we are)?

yes, it's relevant because bugzilla doesn't enforce gzip transfer compression.

that said, the main point of this change is to reduce the number of requests, which has much more of an impact on sites with lots of extensions.
Comment 11 User image Byron Jones ‹:glob› 2014-05-05 08:56:58 PDT
Created attachment 8417444 [details] [diff] [review]
977969_2.patch

- fixes permissions of skins/assets
- no longer touches formatting of related code
Comment 12 User image Frédéric Buclin 2014-05-05 09:07:39 PDT
Comment on attachment 8417444 [details] [diff] [review]
977969_2.patch

>diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm


>+use File::Slurp;

File::Slurp is not a core module, so you will have to list it as a mandatory module. Do you really need it to read/write a file?
Comment 13 User image Byron Jones ‹:glob› 2014-05-05 21:37:30 PDT
(In reply to Frédéric Buclin from comment #12)
> File::Slurp is not a core module, so you will have to list it as a mandatory
> module.

ah, we already use File::Slurp in Bugzilla::JobQueue (via bug 832893) and i thought i'd added it to the dependencies there.  as it needs to be fixed on the 4.4 branch, i've file bug 1006288 to track that.

> Do you really need it to read/write a file?

it improves the readability of the code, and is faster than a traditional "undef $/" slurp.
http://search.cpan.org/~drolsky/File-Slurp-9999.13/extras/slurp_article.pod
Comment 14 User image Gervase Markham [:gerv] 2014-05-06 02:30:59 PDT
Comment on attachment 8417444 [details] [diff] [review]
977969_2.patch

Review of attachment 8417444 [details] [diff] [review]:
-----------------------------------------------------------------

::: skins/README
@@ +17,5 @@
> +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.
> +
> +assets/ holds the minified and concatenated files which are created by
> +checksetup.pl and Bugzilla::Template.  Do no edit the files in this directory.

s/no/not/.
Comment 15 User image Gervase Markham [:gerv] 2014-05-06 02:36:47 PDT
Comment on attachment 8417444 [details] [diff] [review]
977969_2.patch

Review of attachment 8417444 [details] [diff] [review]:
-----------------------------------------------------------------

r=gerv. Feel free to address the comments if you feel they have merit, or not otherwise.

Gerv

::: Bugzilla/Template.pm
@@ +488,5 @@
>      return \%set;
>  }
>  
> +sub _css_unified {
> +    my @sources = map { @$_ } @_;

Is there a better name for this function? _unify_css ?

Also, I don't know what this first line does exactly, but is it worth sorting @sources, so that two pages using the same CSS files will use the same cached file, even if they specify them in a different order?
Comment 16 User image Byron Jones ‹:glob› 2014-05-06 07:25:30 PDT
(In reply to Gervase Markham [:gerv] from comment #15)
> Is there a better name for this function? _unify_css ?

how about _concatenate_css

> Also, I don't know what this first line does exactly

this sub is passed arrayrefs.  the first line merges them into a single array.

> but is it worth sorting @sources, so that two pages using the same CSS files will use the
> same cached file, even if they specify them in a different order?

no, the css files need to be included in the same order as the array.  changing that impacts which rules end are applied to elements.
Comment 17 User image Gervase Markham [:gerv] 2014-05-06 08:33:09 PDT
(In reply to Byron Jones ‹:glob› from comment #16)
> how about _concatenate_css

Sure.

> no, the css files need to be included in the same order as the array. 
> changing that impacts which rules end are applied to elements.

Good point!

Gerv
Comment 18 User image Byron Jones ‹:glob› 2014-05-13 22:36:45 PDT
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   fca0b6c..6d3857e  master -> master
Comment 19 User image Byron Jones ‹:glob› 2014-05-13 23:48:05 PDT
fix for mod_perl:

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   6d3857e..33a29ba  master -> master

Note You need to log in before you can comment on or make changes to this bug.