Add support for non-integer scaling factors in resource_scale_factors presubmit script

Add support for non-integer scaling factors in resource_scale_factors presubmit script


BUG=None
TEST=tested locally by huangs

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153826

[benrg, 2012-08-27 20:43:46]

flackr, oshima, does this look reasonable? (Note: not tested yet so there may be
silly bugs.)

[flackr, 2012-08-28 16:51:28]

LGTM

Returning a validator function may make the code read more nicely, i.e. a check
for width and height being within floor and ceil of "exact" scaled width/height,
but not necessary.

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
File ui/resources/resource_check/resource_scale_factors.py (right):

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
ui/resources/resource_check/resource_scale_factors.py:56: (base_size *
scale_percent + 99) / 100]))
This isn't exactly the same as ceil(base_size * scale_percent), though should be
fine for most cases.

[flackr, 2012-08-28 16:52:30]

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
File ui/resources/resource_check/resource_scale_factors.py (right):

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
ui/resources/resource_check/resource_scale_factors.py:28: [(1,
'default_100_percent'), (2, 'default_200_percent')]
Update this comment please.

[benrg, 2012-08-28 18:03:09]

Oshima says we need to talk to the UI people about the correct constraint, so
this CL is on hold for now.

> Returning a validator function may make the code read more
> nicely, i.e. a check for width and height being within floor
> and ceil of "exact" scaled width/height,
> but not necessary.

I was going to do it as a validator, but it seemed like a good idea to display
the legal values in the error message, which meant I had to compute them anyway,
so a direct membership test was easier.

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
File ui/resources/resource_check/resource_scale_factors.py (right):

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
ui/resources/resource_check/resource_scale_factors.py:28: [(1,
'default_100_percent'), (2, 'default_200_percent')]
On 2012/08/28 16:52:30, flackr wrote:
> Update this comment please.

Fixed.

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
ui/resources/resource_check/resource_scale_factors.py:56: (base_size *
scale_percent + 99) / 100]))
On 2012/08/28 16:51:28, flackr wrote:
> This isn't exactly the same as ceil(base_size * scale_percent)

I think it is (when base_size and scale_percent are integers).

[oshima, 2012-08-28 18:09:30]

On Tue, Aug 28, 2012 at 11:03 AM, <benrg@chromium.org> wrote:

> Oshima says we need to talk to the UI people about the correct constraint,
> so
> this CL is on hold for now.


I believe this CL won't affect 140/180 assets, correct? If so, you can go
ahead and check it in.
Just don't add checks for 140/180 yet.


>
>
>  Returning a validator function may make the code read more
>> nicely, i.e. a check for width and height being within floor
>> and ceil of "exact" scaled width/height,
>> but not necessary.
>>
>
> I was going to do it as a validator, but it seemed like a good idea to
> display
> the legal values in the error message, which meant I had to compute them
> anyway,
> so a direct membership test was easier.
>
>
>
> http://codereview.chromium.**org/10875076/diff/1/ui/**
>
resources/resource_check/**resource_scale_factors.py<http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/resource_scale_factors.py>
> File ui/resources/resource_check/**resource_scale_factors.py (right):
>
> http://codereview.chromium.**org/10875076/diff/1/ui/**
>
resources/resource_check/**resource_scale_factors.py#**newcode28<http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/resource_scale_factors.py#newcode28>
> ui/resources/resource_check/**resource_scale_factors.py:28: [(1,
> 'default_100_percent'), (2, 'default_200_percent')]
> On 2012/08/28 16:52:30, flackr wrote:
>
>> Update this comment please.
>>
>
> Fixed.
>
>
> http://codereview.chromium.**org/10875076/diff/1/ui/**
>
resources/resource_check/**resource_scale_factors.py#**newcode56<http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/resource_scale_factors.py#newcode56>
> ui/resources/resource_check/**resource_scale_factors.py:56: (base_size *
> scale_percent + 99) / 100]))
> On 2012/08/28 16:51:28, flackr wrote:
>
>> This isn't exactly the same as ceil(base_size * scale_percent)
>>
>
> I think it is (when base_size and scale_percent are integers).
>
>
http://codereview.chromium.**org/10875076/<http://codereview.chromium.org/108...
>

[flackr, 2012-08-28 21:45:25]

Yes, this doesn't check 140 or 180, but it also wouldn't change anything for a
scale factor of 200.

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
File ui/resources/resource_check/resource_scale_factors.py (right):

http://codereview.chromium.org/10875076/diff/1/ui/resources/resource_check/re...
ui/resources/resource_check/resource_scale_factors.py:56: (base_size *
scale_percent + 99) / 100]))
On 2012/08/28 18:03:09, benrg wrote:
> On 2012/08/28 16:51:28, flackr wrote:
> > This isn't exactly the same as ceil(base_size * scale_percent)
> 
> I think it is (when base_size and scale_percent are integers).

Right, was thinking of floats but these are both ints. My mistake.

[benrg, 2012-08-28 22:50:26]

Oshima, I need your owner approval.

[oshima, 2012-08-28 22:54:46]

LGTM

[commit-bot: I haz the power, 2012-08-28 22:57:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/10875076/3002

[commit-bot: I haz the power, 2012-08-28 22:57:49]

Failed to apply patch for chrome/app/theme/PRESUBMIT.py:
While running patch -p1 --forward --force;
patching file chrome/app/theme/PRESUBMIT.py
Hunk #1 FAILED at 28.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/app/theme/PRESUBMIT.py.rej

[commit-bot: I haz the power, 2012-08-28 23:05:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benrg@chromium.org/10875076/5003

[commit-bot: I haz the power, 2012-08-29 02:17:20]

Change committed as 153826