[webkit-reviews] review granted: [Bug 24396] Review use of ENABLE(3D_TRANSFORMS) macro : [Attachment 28674] Patch, changelogs

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 24396: Review use of ENABLE(3D_TRANSFORMS) macro
https://bugs.webkit.org/show_bug.cgi?id=24396

Attachment 28674: Patch, changelogs
https://bugs.webkit.org/attachment.cgi?id=28674&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Change looks great.

> +#if ENABLE(3D_RENDERING)
> +    if (value) {
> +	   float number;
> +	   return numberValue(value, number) && compareValue(1,
static_cast<int>(number), op);
> +    }
> +    return true;
> +#else
>      if (value) {
>	   float number;
>	   return numberValue(value, number) && compareValue(0,
static_cast<int>(number), op);
>      }
>      return false;
> +#endif

I would have made a named constant for the one and 0 and the default value
rather than copying the body of the function twice. Not your fault, but this
code is surprisingly awkward and wordy.

> +    // Throw away the non-affine parts of the matrix (lossy!)
> +    TransformationMatrix& makeAffine();

I think it's confusing to have these functions that both modify in place and
return the value of an object. It's too easy to call one of these and not
realize that it modifies "this". This is not just a theoretical issue -- I've
had to fix many bugs during the lifetime of WebKit due to this sort of
misunderstanding. Because of that, I'd prefer to have this return void. Your
thoughts?

> +#if !ENABLE(3D_RENDERING)
> +	   m_transform->makeAffine();
> +#endif

Should we perhaps have a function just for this purpose so we don't have to
have the #if everywhere? It would just be an inline that does exactly this. I'm
not sure exactly what to call it, but something about removing the parts of the
transform that we would be unable to render.

> +// Accelerated compositing (also needs to be set in WebCore/config.h)
> +#if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> +#define WTF_USE_ACCELERATED_COMPOSITING 0
> +#endif

I'm worried about this. Why do we have to set this up both in config.h and in
the prefix? This should be in exactly one place. It's OK to have that one place
be included both by the prefix and config.h, but it's not OK to repeat things
like this. I know this configuration stuff is a mess, but I would like to keep
it from getting even worse. Maybe there's nothing you can easily improve here.
We could talk about this in person at some point if you like.

Despite these 4 comments, I'm going to say r=me as is. If you'd like to clear
the review flag and post a new patch for review withe improvements, that would
be OK. Or landing this as-is also would be. I think the one I feel most
strongly about is the return value from makeAffine.