On 25 December 2012 20:05, Maxim Dounin <mdounin@mdounin.ru> wrote:

> Hello!
>
> On Thu, Dec 20, 2012 at 09:01:38PM +0400, ivan babrou wrote:
>
> > I've send patch for configuration semantics in separate thread.
> >
> > Here's lastest version of my patch that may be applied after. Is there
> > something else that I should fix?
>
> I would like to see clarification on how are you going to handle
> just bare numbers specified. As of now the code seems to don't do
> any special processing and seems to assume all values are correct,
> which might not be true and will result in some mostly arbitray
> crop offset selection. It might make sense to actually support
> percents as in Maxim Bublis's patch[1].
>
> [1] http://mailman.nginx.org/pipermail/nginx-devel/2012-April/002156.html


I assume that ngx_atoi will return NGX_ERROR if it's not number. If number
is ok, (ngx_uint_t) n will be returned. If it's not a number, string checks
will be performed. If even crop offset value is wrong then
NGX_HTTP_IMAGE_OFFSET_CENTER
will be returned which is default behaviour and zero at the same time. So
if you pass incorrect number and expect a number then you'll just get zero.
Just like it was before.

Cropping with offsets that is specified in percents seems more specific
task. I don't think this is required.

[...]
>
> > @@ -1151,7 +1188,24 @@ ngx_http_image_filter_value(ngx_str_t *value)
> >
> > n = ngx_atoi(value->data, value->len);
> >
> > - if (n > 0) {
> > + if (n == NGX_ERROR) {
> > + if (ngx_strncmp(value->data, "left", value->len) == 0) {
> > + return NGX_HTTP_IMAGE_OFFSET_LEFT;
> > +
> > + } else if (ngx_strncmp(value->data, "right", value->len) == 0) {
> > + return NGX_HTTP_IMAGE_OFFSET_RIGHT;
> > +
> > + } else if (ngx_strncmp(value->data, "top", value->len) == 0) {
> > + return NGX_HTTP_IMAGE_OFFSET_TOP;
> > +
> > + } else if (ngx_strncmp(value->data, "bottom", value->len) == 0)
> {
> > + return NGX_HTTP_IMAGE_OFFSET_BOTTOM;
> > +
> > + } else {
> > + return NGX_HTTP_IMAGE_OFFSET_CENTER;
> > + }
> > +
> > + } else if (n > 0) {
> > return (ngx_uint_t) n;
> > }
> >
>
> The ngx_strncmp() checks are incorrect, see here:
> http://mailman.nginx.org/pipermail/nginx-devel/2012-December/003065.html
>
> You may also move them below "if (n > 0)" the check for better
> readability.
>
> > @@ -1175,6 +1229,8 @@ ngx_http_image_filter_create_conf(ngx_conf_t *cf)
> > conf->angle = NGX_CONF_UNSET_UINT;
> > conf->transparency = NGX_CONF_UNSET;
> > conf->buffer_size = NGX_CONF_UNSET_SIZE;
> > + conf->crop_offset_x = NGX_CONF_UNSET_UINT;
> > + conf->crop_offset_y = NGX_CONF_UNSET_UINT;
>
> The "crop_" prefix probably should be dropped here for better
> readability (and to match other names in the code like "oxcv").
>
> [...]
>
> --
> Maxim Dounin
> http://nginx.com/support.html
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>



--
Regards, Ian Babrou
http://bobrik.name http://twitter.com/ibobrik skype:i.babrou
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel