We need to clarify the use of ENABLE(3D_TRANSFORMS). Maybe it should be more like ENABLE(3D_RENDERING).
Should fix along with 24165
Created attachment 28674 [details] Patch, changelogs The thrust of this patch is to change the 3d flag from "has 3d transforms" to "has 3d rendering". "has 3d transforms" no longer makes sense, since we parse and keep 3d transforms in RenderStyles on all platforms. What we need to know, instead, is whether the platform supports 3d rendering, and if it does not, we need to flatten to affine for rendering and hit testing. We also need to make sure that the media query gives the correct answer.
Comment on attachment 28674 [details] Patch, changelogs 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.
(In reply to comment #3) > (From update of attachment 28674 [details] [review]) > 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. I did that in one version of the patch, but it means having another variable for the return value. I'll do it. > > + // 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? Actually, most of the "operations" on TransformationMatrix modify |this|, like multiply(), because it reduces the amount of copying, and TransformationMatrix is 128 bytes (see bug 24395). They return a reference to this so you can chain them. However, I'm fine returning void to reduce the chance of confusion. > > +#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. Yeah, but I'd rather not put ENABLE(3D_RENDERING) in TransformationMatrix. Maybe a static method on some rendering-related class, like: static RenderObject::makeRenderableMatrix(TransformationMatrix&) ? > > +// 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. Agreed. I asked Mark Rowe about this and he didn't have any suggestions. We need a header that is included by all code in WebCore, WebKit and tools (DRT), and gets preprocessed after the platform defines. I think this should be done in a separate patch. I filed bug 24647. > 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.
better name: makeMatrixRenderable() as a free function somewhere.
(In reply to comment #4) > Actually, most of the "operations" on TransformationMatrix modify |this|, like > multiply(), > because it reduces the amount of copying, and TransformationMatrix is 128 bytes > (see bug 24395). They return a reference to this so you can chain them. As we discussed on IRC, the "return reference so you can chain" idiom doesn't seem to work very well. > Yeah, but I'd rather not put ENABLE(3D_RENDERING) in TransformationMatrix. I agree. > Maybe a static method on some rendering-related class, like: > > static RenderObject::makeRenderableMatrix(TransformationMatrix&) ? As we discussed on IRC, I like: void makeMatrixRenderable(TransformationMatrix&); And I think it would be fine to put it in RenderObject.h.
I filed bug 24649 on the TransformationMatrix mutibility issue.
http://trac.webkit.org/changeset/41780