Code review - Issue 6865054: Combine and minifiy CSS files.https://codereview.appspot.com/2012-12-04T20:09:33+00:00rietveld
Message from unknown
2012-12-03T18:27:45+00:00benjiurn:md5:756022f60383bf85fcb0a34d89538330
Message from benji.york@gmail.com
2012-12-03T18:27:54+00:00benjiurn:md5:4a94a51e51f9fb185fd25a1ca05d50ab
Please take a look.
Message from matthew.scott@canonical.com
2012-12-03T21:57:20+00:00matthew.scotturn:md5:e5e78bbadb2f52923fa161e36fe80349
Request rereview (needs fixes).
I have one minor comment about the code, but was unable to complete a functional test or a more in-depth look at the branch. Running make build, server, or debug gave me four of these errors: http://pastebin.ubuntu.com/1408941/ - this happened both after first checking out the branch and also after make clean. When running the branch, I would get a 404 on all-static.css, all-yui.js, and app.js; cache flushed and force-reloaded, same errors.
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
I think that this regex could use some more explanation or a more descriptive variable name; it's a big one.
Message from bac@canonical.com
2012-12-04T14:50:11+00:00bacurn:md5:13703e1dad09b673d8eda700af1bd932
Thanks for the changes Benji. I've made some suggestions on making it more understandable. I have no need to re-review.
https://codereview.appspot.com/6865054/diff/1/Makefile
File Makefile (right):
https://codereview.appspot.com/6865054/diff/1/Makefile#newcode21
Makefile:21: $(BUILD_ASSETS_DIR)/combined-css/all-static.css
Thanks for making this sensible change.
https://codereview.appspot.com/6865054/diff/1/Makefile#newcode30
Makefile:30: cp -r --parents */assets "$(PWD)/$(BUILD_ASSETS_DIR)")
Why is $PWD required here but not at line 28? Goose/gander, etc.
https://codereview.appspot.com/6865054/diff/1/Makefile#newcode86
Makefile:86: ln -sf `pwd`/node_modules/yui app/assets/javascripts/
Should we standardize on `pwd` vs $(PWD) ? They are equivalent, no?
https://codereview.appspot.com/6865054/diff/1/bin/merge-files
File bin/merge-files (right):
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode9
bin/merge-files:9: * firewall without access to the internet.
Can we do this now? Are we not getting anything from the Yahoo CDN? Two possible culprits are gallery-markdown and gallery-ellipsis. I know they are handled specially below but have you tried running while disconnected?
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode17
bin/merge-files:17: * loader object. It means it wont even try to download modules. We cannot
typo: won't
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode18
bin/merge-files:18: * do it because the loader also manages the "use" property which defines
"We cannot do it" -- what is "it"? I cannot parse that sentence.
https://codereview.appspot.com/6865054/diff/1/grunt.js
File grunt.js (right):
https://codereview.appspot.com/6865054/diff/1/grunt.js#newcode1
grunt.js:1: 'use strict';
This file could you an introduction. (Sorry, I know you've just inherited it.)
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
Oh, I thought that was a caterpillar emoticon. Does it *do* something?
Message from benji.york@gmail.com
2012-12-04T17:22:23+00:00benjiurn:md5:be1e92350da030728a61cd98bfaf8709
Thanks for the good input guys. I have fixed all the issues pointed out (plus a couple more). See my replies to individual comments for more detail.
https://codereview.appspot.com/6865054/diff/1/Makefile
File Makefile (right):
https://codereview.appspot.com/6865054/diff/1/Makefile#newcode30
Makefile:30: cp -r --parents */assets "$(PWD)/$(BUILD_ASSETS_DIR)")
On 2012/12/04 14:50:11, bac wrote:
> Why is $PWD required here but not at line 28? Goose/gander, etc.
The $PWD is required here because we are running in a subshell (the parenthesis wrapping the commands invoke a subshell) and inside that shell we are issuing a "cd" command so the "cp -r --parents" will work the way we want (copying the subdirectory structure).
https://codereview.appspot.com/6865054/diff/1/Makefile#newcode86
Makefile:86: ln -sf `pwd`/node_modules/yui app/assets/javascripts/
On 2012/12/04 14:50:11, bac wrote:
> Should we standardize on `pwd` vs $(PWD) ? They are equivalent, no?
I was really just removing the unneccesary and silly "./" from the path, but yes, we can just use one or the other. I prefer $(PWD) because it does not invoke a shell each time it is rendered so it does less work. I have fixed these.
https://codereview.appspot.com/6865054/diff/1/bin/merge-files
File bin/merge-files (right):
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode9
bin/merge-files:9: * firewall without access to the internet.
On 2012/12/04 14:50:11, bac wrote:
> Can we do this now?
If "this" is "use the YUI combo loader", then no. The combo loader requires a special combo server and we just want to use simple static HTTP servers.
> Are we not getting anything from the Yahoo CDN? Two
> possible culprits are gallery-markdown and gallery-ellipsis. I know they are
> handled specially below but have you tried running while disconnected?
I haven't tried running while disconnected, but the Chrome network panel clearly shows them being loaded from an external site.
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode17
bin/merge-files:17: * loader object. It means it wont even try to download modules. We cannot
On 2012/12/04 14:50:11, bac wrote:
> typo: won't
Thanks, I didn't see that typo when I was editing this wall of text.
https://codereview.appspot.com/6865054/diff/1/bin/merge-files#newcode18
bin/merge-files:18: * do it because the loader also manages the "use" property which defines
On 2012/12/04 14:50:11, bac wrote:
> "We cannot do it" -- what is "it"? I cannot parse that sentence.
I don't know. I didn't write this text, I was just trying to fix it up as best I could.
https://codereview.appspot.com/6865054/diff/1/grunt.js
File grunt.js (right):
https://codereview.appspot.com/6865054/diff/1/grunt.js#newcode1
grunt.js:1: 'use strict';
On 2012/12/04 14:50:11, bac wrote:
> This file could you an introduction. (Sorry, I know you've just inherited it.)
I've added a one-liner description of what the file does. It's the best I have without digging more deeply into it.
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js
File lib/merge-files.js (right):
https://codereview.appspot.com/6865054/diff/1/lib/merge-files.js#newcode27
lib/merge-files.js:27: var regex = /\(["']?((\.\/|\.\.\/)[^)]*)["']\)/g;
On 2012/12/03 21:57:20, matthew.scott wrote:
> I think that this regex could use some more explanation or a more descriptive
> variable name; it's a big one.
This function has been rendered unnecessary and has been removed. You will just have to spend the rest of your life wondering what that regex did.
Message from unknown
2012-12-04T17:34:10+00:00benjiurn:md5:d1a8bf6c40b122c9d6a1652d70ac0c2a
Message from benji.york@gmail.com
2012-12-04T17:34:12+00:00benjiurn:md5:dce84ed7e81d8ed6be7a52859e1d7a11
Please take a look.
Message from matthew.scott@canonical.com
2012-12-04T19:18:19+00:00matthew.scotturn:md5:a539077340665be1cc3da9a3dcb2fb20
One minor, otherwise good to go.
Thanks for the fixes; definitely an overall win.
https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js
File lib/merge-files.js (right):
https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js#newcode26
lib/merge-files.js:26: function extractRelativePaths(s) {
Should be marked deprecated or removed (though it sounds like that's in the works).
Message from benji.york@gmail.com
2012-12-04T20:07:16+00:00benjiurn:md5:733246bf5017944d6595acf40109c231
Thanks for looking at this again.
https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js
File lib/merge-files.js (right):
https://codereview.appspot.com/6865054/diff/8001/lib/merge-files.js#newcode26
lib/merge-files.js:26: function extractRelativePaths(s) {
On 2012/12/04 19:18:19, matthew.scott wrote:
> Should be marked deprecated or removed (though it sounds like that's in the
> works).
Fixed.
Message from unknown
2012-12-04T20:07:53+00:00benjiurn:md5:c53d5076c55a9275b7e88474a36d2283
Message from benji.york@gmail.com
2012-12-04T20:09:33+00:00benjiurn:md5:cbe2524d92034e8e2896c09293d32325
*** Submitted:
Combine and minifiy CSS files.
R=matthew.scott, bac
CC=
https://codereview.appspot.com/6865054