[webkit-reviews] review canceled: [Bug 86022] Refactor layer-related logic out of RenderBoxModelObject : [Attachment 151471] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Florin Malita
<fmalita at chromium.org>'s request for review:
Bug 86022: Refactor layer-related logic out of RenderBoxModelObject
https://bugs.webkit.org/show_bug.cgi?id=86022

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=151471&action=review


All in all, it's a fine patch but I would like to see another round. The next
round will very likely get an r+ from me so if you want to nitpick, now is the
moment.

> Source/WebCore/ChangeLog:12
> +

I would like an explanation for what qualifies for your massive
s/RenderBoxModelObject/RenderBaseObject/. Maybe better would be function-level
comments about why groups of functions were touched.

> Source/WebCore/rendering/RenderBaseObject.cpp:33
> +};

Stray ';'

> Source/WebCore/rendering/RenderBaseObject.cpp:69
> +    // This must be done before we destroy the RenderObject.
> +    if (m_layer)
> +	   m_layer->clearClipRects();

I don't think this is needed.

> Source/WebCore/rendering/RenderBaseObject.h:29
> +class RenderBaseObject : public RenderObject {

Sincerely this name doesn't add much. RenderBaseObject as in? There is a <base>
element in HTML which doesn't have a renderer but such a naming would make me
think it does.

I would rather go with the other alternative (RenderTransformableObject) so
that everyone is happy :)

> Source/WebCore/rendering/RenderBaseObject.h:40
> +    virtual bool requiresLayer() const { return false; }

Nit: It would be better for now if this is pure virtual. Once SVG starts
inheriting from this class, it could help us catch bad use.

> Source/WebCore/rendering/RenderBaseObject.h:48
> +    virtual bool isLayerBase() const OVERRIDE { return true; }

I find |isLayerBase| to be difficult to read and understand at call sites.
Mostly because it's a very generic naming and using the full object name would
be better: isRenderBaseObject().

Arguably, canHaveALayer() could be better as it matches what we are interested
in (but we lose the consistency with the other is* flags and we could object
that we are tying the name to RenderLayer).

> Source/WebCore/rendering/RenderBox.cpp:3764
> +	   RenderLayer* layer = curr->hasLayer() && curr->isBox() ?
toRenderBaseObject(curr)->layer() : 0;

toRenderBox here.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:402
>      if (requiresLayer()) {

Some of this should go to the new class (at least the creation / deletion).
It's possible we want to defer that until SVG starts using it too as you would
have a better picture.

> Source/WebCore/rendering/RenderBoxModelObject.h:81
>      virtual bool requiresLayer() const { return isRoot() ||
isOutOfFlowPositioned() || isRelPositioned() || isTransparent() ||
hasTransform() || hasHiddenBackface() || hasMask() || hasReflection() ||
hasFilter() || style()->specifiesColumns(); }

OVERRIDE.

> Source/WebCore/rendering/svg/RenderSVGModelObject.h:56
> +    virtual LayoutRect clippedOverflowRectForRepaint(RenderBaseObject*
repaintContainer) const;
> +    virtual void computeFloatRectForRepaint(RenderBaseObject*
repaintContainer, FloatRect&, bool fixed = false) const;
> +    virtual LayoutRect outlineBoundsForRepaint(RenderBaseObject*
repaintContainer, LayoutPoint*) const;

Let's annotate all the functions touched that are overriding something with
OVERRIDE while at it.