[webkit-reviews] review granted: [Bug 121828] [CSS Regions] Activate all regions to have layers, as CSS Regions create a new stacking context : [Attachment 213091] patch. incorporates the feedback.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Mihai Maerean <mmaerean at adobe.com>'s
request for review:
Bug 121828: [CSS Regions] Activate all regions to have layers, as CSS Regions
create a new stacking context
https://bugs.webkit.org/show_bug.cgi?id=121828

Attachment 213091: patch. incorporates the feedback.
https://bugs.webkit.org/attachment.cgi?id=213091&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213091&action=review


> Source/WebCore/rendering/RenderRegion.h:142
> +    virtual bool requiresLayer() const { return true; } // Because CSS
Regions create Stacking Contexts.

Should be marking this OVERRIDE.

Also, I think the comment is not clear. I’d rather you make this non-inline so
you could have a clearer comment inside the .cpp file. We won’t lose any
inlining from that. I would write this comment:

    // All regions create stacking contexts, as specified in the CSS standard.
Do that by allocating a separate RenderLayer for each.
    bool RenderRegion::requiresLayer() const
    {
	return true;
    }

The comment may not seem all that different to you, but I find it more specific
and easier to comprehend.

> Source/WebCore/rendering/RenderRegionSet.h:64
> +    virtual bool requiresLayer() const { return
RenderBlockFlow::requiresLayer(); }

Should be marking this OVERRIDE and FINAL.

But also, this needs a why comment. The change log simply says what the code is
doing, and not why. A brief comment explaining why is what we are looking for.

Why don’t all region sets require a layer? And given that, why do they derive
from region? It is possibly evidence that the class hierarchy is flawed. The
comment can be brief, but the subject is “why”, not what.