concatenate and slightly minify css files

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

({bmo-goal, perf})

unspecified
Bugzilla 5.0
bmo-goal, perf
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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]-->
(Assignee)

Comment 1

3 years ago
to be clear: minifiation should only happen in production mode.

Comment 2

3 years ago
I already filed bug 956190 about decreasing the number of CSS files.
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
(In reply to Byron Jones ‹:glob› from comment #0)
> - remove IE specific css file

these were removed as part of bug 956190.
(Assignee)

Updated

3 years ago
Assignee: general → glob
(Assignee)

Comment 5

3 years ago
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.
Attachment #8415049 - Flags: review?(gerv)
(Assignee)

Updated

3 years ago
Keywords: bmo-goal, perf

Comment 6

3 years ago
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...
(Assignee)

Comment 7

3 years ago
(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

3 years ago
(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 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.
Attachment #8415049 - Flags: review?(gerv) → review-
(Assignee)

Comment 10

3 years ago
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8417444 [details] [diff] [review]
977969_2.patch

- fixes permissions of skins/assets
- no longer touches formatting of related code
Attachment #8415049 - Attachment is obsolete: true
Attachment #8417444 - Flags: review?(gerv)

Comment 12

3 years ago
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?
(Assignee)

Comment 13

3 years ago
(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

Updated

3 years ago
Depends on: 1006288
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 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?
Attachment #8417444 - Flags: review?(gerv) → review+
(Assignee)

Comment 16

3 years ago
(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.
Flags: approval?
Target Milestone: --- → Bugzilla 5.0
(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
(Assignee)

Updated

3 years ago
Flags: approval? → approval+
(Assignee)

Comment 18

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   fca0b6c..6d3857e  master -> master
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

3 years ago
fix for mod_perl:

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   6d3857e..33a29ba  master -> master
Blocks: 1013209
(Assignee)

Updated

3 years ago
Blocks: 1016199
Blocks: 1160598
You need to log in before you can comment on or make changes to this bug.