Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11825019:
a utility script to pngcrush png files (Closed)
|
Created:
7 years, 11 months ago by oshima Modified:
7 years, 11 months ago CC:
chromium-reviews, msw Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptiona utility script to pngcrush png files
BUG=150046
TEST=none
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176157
Patch Set 1 : #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #Patch Set 4 : fix trap #
Total comments: 2
Messages
Total messages: 13 (0 generated)
would you mind take a look at this?
lgtm, but my shell fu is pretty weak. +jrbarnette@
On 2013/01/09 18:40:39, stevenjb (chromium) wrote: > lgtm, but my shell fu is pretty weak. +jrbarnette@ I believe this will work as advertised. There are some style nits you may want to address.
On 2013/01/09 19:15:37, jrbarnette wrote: > On 2013/01/09 18:40:39, stevenjb (chromium) wrote: > > lgtm, but my shell fu is pretty weak. +jrbarnette@ > > I believe this will work as advertised. There are some style nits you may > want to address. I'm happy to address style issues. Please let me know what should be changed.
<sigh> I've gotten used to Gerrit; didn't realize I hadn't published my drafts w/ the nits I mentioned. https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:29: for f in `find $dir -name "*.png"`; do This will fail if any of the .png files have a space in their names. I trust you're confident that can't happen... Here and below, I'd recommend using $() rather than `` (see below). https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:48: TMP_DIR=`mktemp -d` There's inconsistency here and above regarding whether to use `` or $(). The Google bash code style guide calls for $(), so I'd recommend that. https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:51: trap "rm -rf $TMP_DIR; exit 255" SIGINT SIGTERM EXIT The 'exit 255' combined with 'trap ... EXIT' means that this script will always exit with a failure status. Usually, that's bad form.
https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:29: for f in `find $dir -name "*.png"`; do On 2013/01/09 20:44:19, jrbarnette wrote: > This will fail if any of the .png files have a space in their names. I trust > you're confident that can't happen... Yes, that shouldn't happen. > Here and below, I'd recommend using $() rather than `` (see below). done https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:48: TMP_DIR=`mktemp -d` On 2013/01/09 20:44:19, jrbarnette wrote: > There's inconsistency here and above regarding whether to use `` or $(). > The Google bash code style guide calls for $(), so I'd recommend that. Done. https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:51: trap "rm -rf $TMP_DIR; exit 255" SIGINT SIGTERM EXIT On 2013/01/09 20:44:19, jrbarnette wrote: > The 'exit 255' combined with 'trap ... EXIT' means that this script will always > exit with a failure status. Usually, that's bad form. I changed to simply exit for now. Do you have any suggestion what to do here? I couldn't figure out how to pass the right exit code.
LGTM https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.sh File build/sanitize-png-files.sh (right): https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... build/sanitize-png-files.sh:51: trap "rm -rf $TMP_DIR; exit 255" SIGINT SIGTERM EXIT On 2013/01/10 05:36:34, oshima wrote: > On 2013/01/09 20:44:19, jrbarnette wrote: > > The 'exit 255' combined with 'trap ... EXIT' means that this script will > always > > exit with a failure status. Usually, that's bad form. > > I changed to simply exit for now. Do you have any suggestion what to do here? > I couldn't figure out how to pass the right exit code. You could have a global variable that you set with the exit code you want. However, that's awkward. I think your best bet to deal with it is just to handle normal exit specially: rm -rf $TMP_DIR trap '' EXIT exit 0
On 2013/01/10 18:34:33, jrbarnette wrote: > LGTM > > https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.sh > File build/sanitize-png-files.sh (right): > > https://codereview.chromium.org/11825019/diff/4001/build/sanitize-png-files.s... > build/sanitize-png-files.sh:51: trap "rm -rf $TMP_DIR; exit 255" SIGINT SIGTERM > EXIT > On 2013/01/10 05:36:34, oshima wrote: > > On 2013/01/09 20:44:19, jrbarnette wrote: > > > The 'exit 255' combined with 'trap ... EXIT' means that this script will > > always > > > exit with a failure status. Usually, that's bad form. > > > > I changed to simply exit for now. Do you have any suggestion what to do here? > > I couldn't figure out how to pass the right exit code. > > You could have a global variable that you set with the exit code > you want. However, that's awkward. I think your best bet to deal > with it is just to handle normal exit specially: > rm -rf $TMP_DIR > trap '' EXIT > exit 0 I think I misunderstood the behavior of trap and EXIT. I believe all I have to do is trap "rm -rf $TMP_DIR" EXIT and it takes care of exit code as well. Could you please take another look?
On 2013/01/10 20:49:56, oshima wrote: [ ... ] > I think I misunderstood the behavior of trap and EXIT. I believe all I have to > do is > > trap "rm -rf $TMP_DIR" EXIT > > and it takes care of exit code as well. > > Could you please take another look? LGTM. Yes, that works quite well.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oshima@chromium.org/11825019/7002
Message was sent while issue was closed.
Change committed as 176157
Message was sent while issue was closed.
Nice!! https://chromiumcodereview.appspot.com/11825019/diff/7002/build/sanitize-png-... File build/sanitize-png-files.sh (right): https://chromiumcodereview.appspot.com/11825019/diff/7002/build/sanitize-png-... build/sanitize-png-files.sh:23: -rem mkTS $file > /dev/null I asked leiz/msw about the best way to crunch PNG files, and he wrote: """PNGCrush alone isn't ideal. You'll also want to strip iTXt, tEXt, zTXt, and tIME chunks *and run PNGSlim* for non-trivial additional lossless size reduction. I've been meaning to write up these instructions for a while, but you can get the gist from: crbug.com/76282 and codereview.chromium.org/7782022 and cmd: pngcrush.exe -reduce -brute -rem iTXt -rem tEXt -rem zTXt -rem tIME""" It looks like you are not using all these flags. Also, the bug mentioned also running pngslim. Finally, should this be a python script, so that windows people can run it? Or is this supposed to run automatically on some bot?
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/11825019/diff/7002/build/sanitize-png-... File build/sanitize-png-files.sh (right): https://chromiumcodereview.appspot.com/11825019/diff/7002/build/sanitize-png-... build/sanitize-png-files.sh:23: -rem mkTS $file > /dev/null On 2013/01/10 23:47:37, Nico wrote: > I asked leiz/msw about the best way to crunch PNG files, and he wrote: > > """PNGCrush alone isn't ideal. You'll also want to strip iTXt, tEXt, zTXt, and > tIME > chunks *and run PNGSlim* for non-trivial additional lossless size reduction. > I've been meaning to write up these instructions for a while, but you can get > the gist from: > > crbug.com/76282 and codereview.chromium.org/7782022 and cmd: > pngcrush.exe -reduce -brute -rem iTXt -rem tEXt -rem zTXt -rem tIME""" > > It looks like you are not using all these flags. Also, the bug mentioned also > running pngslim. I took the command from chrome/app/theme/README but looks like it wasn't up to date. I'll fix them in follow up CL. > Finally, should this be a python script, so that windows people can run it? Or > is this supposed to run automatically on some bot? I was planning to just run this periodically for the time being. I'd rather want to automate someone and will look into it first. IIRC, there was some discussion about it and decided not to, but I'll look into it again. |
