Bug 237469 – Add hasTransformOrPerspective() helper to RenderObject

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

RenderGeometryMap contains a duplication of the 'hasTransform()' logic from RenderObject, just with an additional check. Avoid that.

[Nikolas Zimmermann]

Created attachment 453834 [details]
Patch, v1

[Darin Adler]

Comment on attachment 453834 [details]
Patch, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=453834&action=review

I wonder at what point these function implementations get long enough that we want to move them out of the class definition.  They can still be in the header if they need to be inlined or in the .cpp file if not. Probably not yet but they are certainly getting close.

> Source/WebCore/ChangeLog:3
> +        Add hasTransformOrPerspective() helper to RenderObject

The rationale in the change log explains in detail why we must use hasTransform here, but says nothing about why adding hasTransdormOrPerspective is good.

[alan]

These functions should really be on the RenderElement (RenderText has no business of dealing with transforms and alikes).

[Nikolas Zimmermann]

(In reply to zalan from comment #3)
> These functions should really be on the RenderElement (RenderText has no
> business of dealing with transforms and alikes).

I agree and do address that in a follow-up patch in the LBSE downstream branch, however this only aims to remove the code duplication, postponing cleanups to later. Ok with you?

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #2)
> Comment on attachment 453834 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453834&action=review
> 
> I wonder at what point these function implementations get long enough that
> we want to move them out of the class definition.  They can still be in the
> header if they need to be inlined or in the .cpp file if not. Probably not
> yet but they are certainly getting close.
> 
> > Source/WebCore/ChangeLog:3
> > +        Add hasTransformOrPerspective() helper to RenderObject
> 
> The rationale in the change log explains in detail why we must use
> hasTransform here, but says nothing about why adding
> hasTransdormOrPerspective is good.

Good point! In LBSE I'll have to add another condition to hasTransform(): checking for hasSVGTransform(). Prior to this patch I had to fix two call sites: the one in RenderObject.h and in RenderGeometryMap.cpp -- I want to avoid having to touch more than one place to add such a condition.

I'll amend the ChangeLog a bit to include that information. Thanks for the review, and sorry for the late reply: I was on sick leave.... More patches will follow today, cleaning up transform-box handling, individual bugs in perspective-origin, transform-box + compositing, etc.

[Nikolas Zimmermann]

Committed r290881 (248112@trunk): <https://commits.webkit.org/248112@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/89897134>