Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10699034:
Add presubmit script to verify that scaled resources are correct dimensions. (Closed)
|
Created:
8 years, 5 months ago by flackr Modified:
8 years, 5 months ago CC:
chromium-reviews, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd presubmit script to verify that scaled resources are correct dimensions.
BUG=133573
TEST=Add an incorrectly sized resource, run presubmit checks and see error with details about incorrect size.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=147742
Patch Set 1 #
Total comments: 11
Patch Set 2 : Use tuples. #Patch Set 3 : Include and use pypng library to get image sizes. #Patch Set 4 : Add license and readme. #Patch Set 5 : TEST - Always include the library to test on try bots. #
Total comments: 2
Patch Set 6 : Read PNG header directly to avoid third party library. #Patch Set 7 : Merge with master. #
Messages
Total messages: 19 (0 generated)
Hi, please take a look, thanks!
ping
Did I miss this? I'm very sorry about that. I'm not (no longer) familiar with pyhon, so please add someone who knows python and its style guide as well. http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py File chrome/app/theme/PRESUBMIT.py (right): http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... chrome/app/theme/PRESUBMIT.py:13: keep 1 new lines. same for other places. http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... chrome/app/theme/PRESUBMIT.py:33: ] This must be same for all resources directory, so isn't it better to put this in the common place?
http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py File chrome/app/theme/PRESUBMIT.py (right): http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... chrome/app/theme/PRESUBMIT.py:33: ] On 2012/07/16 19:23:20, oshima1 wrote: > This must be same for all resources directory, so isn't it better to put this in > the common place? I guess this can be different for webui?
On 2012/07/17 20:20:49, oshima wrote: > http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py > File chrome/app/theme/PRESUBMIT.py (right): > > http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... > chrome/app/theme/PRESUBMIT.py:33: ] > On 2012/07/16 19:23:20, oshima1 wrote: > > This must be same for all resources directory, so isn't it better to put this > in > > the common place? > > I guess this can be different for webui? Yes, although to generalize for WebUI too we should separate identifying groups of images to compare with comparing them as in this case a 2x directory can exist in any subdirectory.
I guess we should add a presubmit check for WebUI image scale factors too even though they're not technically required to be the same size by the UI, if you think it should all be done in one CL I can do this now, or I can refactor it later. http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py File chrome/app/theme/PRESUBMIT.py (right): http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... chrome/app/theme/PRESUBMIT.py:13: On 2012/07/16 19:23:20, oshima1 wrote: > keep 1 new lines. same for other places. Just following the style guide. Not sure if that means two blank lines after the top-level comment or not, but the rest should be correct. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... chrome/app/theme/PRESUBMIT.py:33: ] On 2012/07/16 19:23:20, oshima1 wrote: > This must be same for all resources directory, so isn't it better to put this in > the common place? If you think this makes more sense, I thought with the fact that these directories are defined separately by the resource grd files that it made sense to define them separately for the presubmit scripts too.
lgtm On 2012/07/19 20:24:50, flackr wrote: > I guess we should add a presubmit check for WebUI image scale factors too even > though they're not technically required to be the same size by the UI, if you > think it should all be done in one CL I can do this now, or I can refactor it > later. > > http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py > File chrome/app/theme/PRESUBMIT.py (right): > > http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... > chrome/app/theme/PRESUBMIT.py:13: > On 2012/07/16 19:23:20, oshima1 wrote: > > keep 1 new lines. same for other places. > > Just following the style guide. Not sure if that means two blank lines after the > top-level comment or not, but the rest should be correct. I'm no expert on python and I believe you. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... > > http://codereview.chromium.org/10699034/diff/1/chrome/app/theme/PRESUBMIT.py#... > chrome/app/theme/PRESUBMIT.py:33: ] > On 2012/07/16 19:23:20, oshima1 wrote: > > This must be same for all resources directory, so isn't it better to put this > in > > the common place? > > If you think this makes more sense, I thought with the fact that these > directories are defined separately by the resource grd files that it made sense > to define them separately for the presubmit scripts too. Technically true, but they shouldn't be different in practice. At some point of (near) future, we'll have more directories, and it may be better to keep it in one place, but I think it's ok to revisit this when we need it.
lgtm http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... File ui/resources/resource_check/resource_scale_factors.py (right): http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... ui/resources/resource_check/resource_scale_factors.py:27: [[1, 'default_100_percent'], [2, 'default_200_percent']] Nit: I'd prefer tuples: [(1, '...'), ...]. You could extract the scale factor from the context name instead (the context is guaranteed to have the form *_###_percent). You could even drop this parameter entirely in favor of os.listdir. I can't decide whether this would actually be an improvement. http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... ui/resources/resource_check/resource_scale_factors.py:45: return round(base_width * scale), round(base_height * scale) This doesn't need to be fixed right now, but when the scale factor isn't exactly representable as a float this code rounds three times -- first in the scale itself, then the multiplication, then round(). I think it would be better to use integer math. Also, is an exact mathematical round(base*scale) the correct answer? At 100%, three images of 4px each and four images of 3px each have the same total size, but at 140% with this formula they don't. Should we check only that the size is between floor(base*scale) and ceil(base*scale)?
http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... File ui/resources/resource_check/resource_scale_factors.py (right): http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... ui/resources/resource_check/resource_scale_factors.py:27: [[1, 'default_100_percent'], [2, 'default_200_percent']] On 2012/07/19 21:47:56, benrg wrote: > Nit: I'd prefer tuples: [(1, '...'), ...]. Done. > You could extract the scale factor from the context name instead (the context is > guaranteed to have the form *_###_percent). You could even drop this parameter > entirely in favor of os.listdir. I can't decide whether this would actually be > an improvement. Yes I wasn't sure if we were always going to follow the _###_percent or not. There's certainly a question of how much or little magic to put into this check, and I'll gladly iterate on this. http://codereview.chromium.org/10699034/diff/1/ui/resources/resource_check/re... ui/resources/resource_check/resource_scale_factors.py:45: return round(base_width * scale), round(base_height * scale) On 2012/07/19 21:47:56, benrg wrote: > This doesn't need to be fixed right now, but when the scale factor isn't exactly > representable as a float this code rounds three times -- first in the scale > itself, then the multiplication, then round(). I think it would be better to use > integer math. > > Also, is an exact mathematical round(base*scale) the correct answer? At 100%, > three images of 4px each and four images of 3px each have the same total size, > but at 140% with this formula they don't. Should we check only that the size is > between floor(base*scale) and ceil(base*scale)? Yes, I fully expected this to change when we get non-integer scale factors. Probably something like between the floor and ceil makes sense but I think it's worth giving more thought later when we want to start to support that. I'll leave a comment to that effect.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/10699034/10005
Presubmit check for 10699034-10005 failed and returned exit status 1.
Running presubmit commit checks ...
Traceback (most recent call last):
File "/b/commit-queue/verification/presubmit_shim.py", line 43, in <module>
sys.exit(presubmit_support.Main(argv))
File "/b/depot_tools/presubmit_support.py", line 1257, in Main
rietveld_obj)
File "/b/depot_tools/presubmit_support.py", line 1105, in DoPresubmitChecks
results += executer.ExecPresubmitScript(presubmit_script, filename)
File "/b/depot_tools/presubmit_support.py", line 1022, in ExecPresubmitScript
result = eval(function_name + '(*__args)', context)
File "<string>", line 1, in <module>
File "<string>", line 19, in CheckChangeOnCommit
File "<string>", line 44, in _CommonChecks
File
"/b/commit-queue/workdir/chromium/chrome/app/theme/../../../ui/resources/resource_check/resource_scale_factors.py",
line 43, in RunChecks
from PIL import Image
ImportError: No module named PIL
Hi, can you take another look. I've added the pypng library (a pure python png library). I've followed the instructions at http://dev.chromium.org/developers/adding-3rd-party-libraries running licenses.py scan and as far as I know this is good but I've never added a third party library before so let me know if I've missed something. Thanks.
https://chromiumcodereview.appspot.com/10699034/diff/1/ui/resources/resource_... File ui/resources/resource_check/resource_scale_factors.py (right): https://chromiumcodereview.appspot.com/10699034/diff/1/ui/resources/resource_... ui/resources/resource_check/resource_scale_factors.py:27: [[1, 'default_100_percent'], [2, 'default_200_percent']] On 2012/07/20 13:53:34, flackr wrote: > Yes I wasn't sure if we were always going to follow the _###_percent or not. > There's certainly a question of how much or little magic to put into this check, > and I'll gladly iterate on this. Grit (currently) requires the _###_percent suffix. Of course, if you extract the scale from there, you have to deal with non-integer scale factors in this CL. Unless you want to hack on it more, I think it's fine as is. https://chromiumcodereview.appspot.com/10699034/diff/1/ui/resources/resource_... ui/resources/resource_check/resource_scale_factors.py:45: return round(base_width * scale), round(base_height * scale) On 2012/07/20 13:53:34, flackr wrote: > Yes, I fully expected this to change when we get non-integer scale factors. > Probably something like between the floor and ceil makes sense but I think it's > worth giving more thought later when we want to start to support that. I'll > leave a comment to that effect. Sounds good. https://chromiumcodereview.appspot.com/10699034/diff/7003/ui/resources/resour... File ui/resources/resource_check/resource_scale_factors.py (right): https://chromiumcodereview.appspot.com/10699034/diff/7003/ui/resources/resour... ui/resources/resource_check/resource_scale_factors.py:46: return (image[0], image[1]) I think this reads and decodes the whole file, unlike the PIL code. You could eliminate the library dependency by doing this instead: def ImageSize(filename): with open(filename, 'rb', buffering=0) as f: data = f.read(24) assert data[:8] == '\x89PNG\r\n\x1A\n' and data[12:16] == 'IHDR' return struct.unpack('>ii', data[16:24]) (The PNG standard guarantees that the IHDR chunk comes first.) Will we need other features of the library in the future? We might want to rescale PNGs in Grit, but doing that in Python would probably be too slow.
https://chromiumcodereview.appspot.com/10699034/diff/7003/ui/resources/resour... File ui/resources/resource_check/resource_scale_factors.py (right): https://chromiumcodereview.appspot.com/10699034/diff/7003/ui/resources/resour... ui/resources/resource_check/resource_scale_factors.py:46: return (image[0], image[1]) On 2012/07/20 19:51:25, benrg wrote: > I think this reads and decodes the whole file, unlike the PIL code. > > You could eliminate the library dependency by doing this instead: > > def ImageSize(filename): > with open(filename, 'rb', buffering=0) as f: > data = f.read(24) > assert data[:8] == '\x89PNG\r\n\x1A\n' and data[12:16] == 'IHDR' > return struct.unpack('>ii', data[16:24]) > > (The PNG standard guarantees that the IHDR chunk comes first.) > > Will we need other features of the library in the future? We might want to > rescale PNGs in Grit, but doing that in Python would probably be too slow. Ah okay, I didn't realize the standard guaranteed a consistent order. This is probably much better to do for now until we actually need an image library at which point we may want to use PIL since I believe it uses C++ code for the heavy processing. Thanks!
(lgtm)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/10699034/19001
Try job failure for 10699034-19001 (retry) on mac_rel for step "update". It's a second try, previously, step "update" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/flackr@chromium.org/10699034/12006
Try job failure for 10699034-12006 on mac_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
